Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

common/cbor: Add debug.strict_cbor #2258

Closed
wants to merge 2 commits into from

Conversation

Yawning
Copy link
Contributor

@Yawning Yawning commented Oct 14, 2019

We might as well add round-trip sanity checking, at least when we are
testing things. I'm not sure if this should be enabled all the time,
and it is written under the assumption that it is not, as round-trip
failures will crash the node if the flag is enabled.

Inspired by #2020

@Yawning Yawning added c:testing Category: testing c:common Category: common libraries c:security Category: security sensitive labels Oct 14, 2019
@Yawning Yawning self-assigned this Oct 14, 2019
@Yawning Yawning force-pushed the yawning/feature/checked-cbor branch from bb1f4f5 to 5b0fb39 Compare October 14, 2019 09:47
@@ -56,3 +89,9 @@ func MustUnmarshal(data []byte, dst interface{}) {
panic(err)
}
}

func init() {
Flags.Bool(CfgDebugStrictCBOR, false, "(DEBUG) Enforce that CBOR blobs roundtrip")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe make the flag hidden from --help?

@Yawning Yawning force-pushed the yawning/feature/checked-cbor branch from 5b0fb39 to 8e8625d Compare October 14, 2019 09:59
@Yawning
Copy link
Contributor Author

Yawning commented Oct 14, 2019

I'm not sure if this is workable.

A lot of things fail because we have omitempty annotations everywhere on the go side. This is required for Option (or enum?) s11n to match what serde does. On the other hand, we try to use the annotations everywhere on the go side to save space, but serde's idea of this is #[serde(skip_serializing_if = "path")] which we have approximately 0 of.

We might as well add round-trip sanity checking, at least when we are
testing things.  I'm not sure if this should be enabled all the time,
and it is written under the assumption that it is not, as round-trip
failures will crash the node if the flag is enabled.
@Yawning Yawning force-pushed the yawning/feature/checked-cbor branch from 030b670 to 837a9c4 Compare October 14, 2019 10:44
@Yawning
Copy link
Contributor Author

Yawning commented Oct 14, 2019

Yeah, I'm going to give up on this for now.

@Yawning Yawning closed this Oct 14, 2019
@Yawning Yawning deleted the yawning/feature/checked-cbor branch October 14, 2019 10:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c:common Category: common libraries c:security Category: security sensitive c:testing Category: testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants