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

Fix rlp for blocks with no withdrawals. #37

Merged
merged 1 commit into from
Jan 25, 2023

Conversation

spencer-tb
Copy link
Collaborator

@spencer-tb spencer-tb commented Jan 20, 2023

Regarding #36

It looks like before RLP serialization for blocks:

image

the Block class (block) doesn't set withdrawals to the empty list [ ] for Shanghai, and instead this remains as a None type. This

This PR contains one solution to the latter. As withdrawals are within the Environment class, and it is updated according to the fork type before RLP serialization:

image

we can use it instead, as the withdrawals None type here is set to the empty list [ ] within set_fork_requirements.

@spencer-tb
Copy link
Collaborator Author

spencer-tb commented Jan 20, 2023

@marioevz

Some other alternative options:

  1. To keep the withdrawals field consistent between the Environment class and the Block class we could add block as another input within set_fork_requirements -> set_fork_requirements(env, block, fork), and update the withdrawals for block to [ ] if it has a none type for Shanghai alongside env.
  2. Achieve a similar outcome with a new function called set_block_requirements(block, fork).
  3. Make the default type for withdrawals in the Block class the empty list as opposed to the None type.

@marioevz
Copy link
Member

A couple of things:

  • Looking at my code on make_block in BlockchainTests it seems like was really easy to make this mistake in the first place because of how the block and env are just mixed all over the place. I think it would be nice to refactor this a bit on a different PR to make a clear distinction that block structure is the source of the test information, but env is the structure that should contain all the correct information at some point.
  • I think this same mistake could be present in the StateTests, it's just that we don't have any withdrawals tests written in the StateTests format.

Copy link
Member

@marioevz marioevz left a comment

Choose a reason for hiding this comment

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

LGTM

@marioevz marioevz merged commit b4cfe83 into ethereum:main Jan 25, 2023
@spencer-tb spencer-tb deleted the empty-withdrawals-list branch January 25, 2023 18:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants