Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
test: anchor crypto and RLP tests to independent expectations #12712
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
base: master
Are you sure you want to change the base?
test: anchor crypto and RLP tests to independent expectations #12712
Changes from 7 commits
e2e6cf3dcf731afc4ee9f60409884a51ebab26b4348eb510472048129d41c5a82bda9eFile filter
Filter by extension
Conversations
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Uh oh!
There was an error while loading. Please reload this page.
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Low — this assert can't fail, and it reads like it can.
HeaderDecoder.cs:54setsHash = Keccak.Compute(headerRlp)from the raw incoming span, and the builder sets the original'sHashviaCalculateHash()over the same encoding (BlockHeaderBuilder.cs:19). So both sides hash the same bytes: no decode defect — symmetric or asymmetric — can move this assert. Its only real content is that the header bytes embedded byBlockDecodermatch what the standalone header encoder produces.The comment two lines up gestures at this ("The hash values also derive from the encoded bytes"), but the assert is then placed first, where a reader will take it as the strongest check in the block. Either drop it or say explicitly that it cross-checks the block-embedded header encoding against the standalone one — otherwise it's the same self-consistency pattern the PR is removing elsewhere.
The same caveat applies to the transaction loop below:
BaseTxDecoder.cs:68setsHashfrom the tx wire bytes, so comparing hashes catches ordering and asymmetric errors (good — that's real coverage) but not a symmetric field swap insideTxDecoder.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Medium — the field list stops short of the newest header fields.
HeaderDecoderround-trips 23 header items; this list covers 18. Missing:Bloom,ParentBeaconBlockRoot,RequestsHash,BlockAccessListHash,SlotNumber(seeHeaderDecoder.cs:52and:60-63). Since a symmetric encode/decode error is invisible to the re-encode comparison, those five fields keep exactly the blindness this PR is removing — and they're the newest, i.e. the likeliest to grow a bug.Bloomis set in every scenario (BlockHeaderBuilder.cs:34→Bloom.Empty) andBloomimplementsIEquatable<Bloom>, so it's a free one-liner. The four trailing optional fields are never set by any scenario inBuildScenarios(), so they're vacuous until a scenario sets them —BlockBuilderalready hasWithParentBeaconBlockRoot/WithRequestsHash/WithBlockAccessListHash, so one extra scenario would close the whole tail.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Low (carry-over, optional) — the body is still compared by count only, one level below the now-complete header comparison.
Uncles are the cheap half and they're already discriminating:
BuildScenarios()buildsuncles[0]withWithdrawalsRoot == nullanduncles[1]withKeccak.Compute("1")(line 39), sodecoded.Uncles[i].Hashvsblock.Uncles[i].Hashwould catch an order or content error the count assert can't see. If you add that loop, the length guard has to move outsideEnterMultipleScope()— the way the transaction guard sits at line 180 — otherwise a count mismatch throwsIndexOutOfRangeExceptioninside the scope instead of reporting.Withdrawals are weaker:
WithWithdrawals(8)fills the array with eight defaultnew Withdrawal()instances, so a content comparison would be vacuous until a scenario uses distinct ones (TestItem.WithdrawalA_1Eth…WithdrawalF_6Ethexist).Fine to leave for a later chunk in the series — this PR's stated scope is the header and the crypto/RLP anchors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Low (nit) — the test name
Spandescribed the old assertion (span overload vs array overload); now that both overloads assert a fixed vector, something likeComputes_known_hash_for_span_and_arraywould say what it checks. Optional, and it does grow the diff.I couldn't re-derive
0x5902e5…in this environment (no Python/dotnet execution available here), but a wrong constant fails both asserts in CI rather than passing silently, so the risk of a bad vector landing is nil.Uh oh!
There was an error while loading. Please reload this page.