eip4844: update tx_peek_blob_versioned_hashes to match tx type from fee market update#3027
Merged
eip4844: update tx_peek_blob_versioned_hashes to match tx type from fee market update#3027
Conversation
Inphi
reviewed
Oct 5, 2022
…ined in EIP PR 5707 (fee market update)
protolambda
force-pushed
the
eip-4844-fee-ssz-fix
branch
from
47a42af to
2d08dc5
Compare
Contributor
|
Maybe merge this PR after ethereum/EIPs#5707 is merged? |
Inphi
approved these changes
Oct 26, 2022
Contributor
Inphi
left a comment
Inphi
left a comment
There was a problem hiding this comment.
good to go as soon as ethereum/EIPs#5707 is merged.
Contributor
Author
|
This PR is ready for final review, the |
roberto-bayardo
approved these changes
Nov 1, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Also see fee market update PR to EIP, specifically the change to
BlobTransaction: ethereum/EIPs#5707This PR simply updates the offsets used in
tx_peek_blob_versioned_hashesto match again.If we pulled in the EIP type definition then we could have some test that the definition is correct.We actually do have a full definition of it in the testing code. Updated that, so now the tests should be able to pass.And added a unit test for the
tx_peek_blob_versioned_hashesfunction as a wholeFor now I updated the original gist file that computes the offset numbers: https://gist.github.com/protolambda/23bd106b66f6d4bb854ce46044aa3ca3
Thanks to Mofi and Roberto for reminding me of the possible tx type change effects on CL: the prysm code uses the full type definition instead of the optimized offset function, but either way the CL needs to reflect the EIP PR (when it's merged).