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

Cleanly fix TestBlockSync #208

Open
karlb opened this issue Aug 26, 2024 · 0 comments
Open

Cleanly fix TestBlockSync #208

karlb opened this issue Aug 26, 2024 · 0 comments

Comments

@karlb
Copy link

karlb commented Aug 26, 2024

Upstream geth recently introduced a new test (go test ./beacon/blsync -run TestBlockSync) that fails after 087af14 is applied. The workaround in with 8efa066 makes it pass, but it would be preferable to fix the issue in a way that does not require modifications to the test.

Comment from @piersy:

The reason for the failure is the different block encoding we use for pre-gingerbread blocks. We identify pre-gingerbread blocks by looking for a zero gas limit, and in theory all ethereum (I.E. non celo) blocks should never have a zero gas limit, however many tests in the geth codebase fail to set correct parameters for blocks when and subsequently do not have a gas limit set. In this case the tests only set the block number and so the blocks are encoded as pre gingerbread blocks, which results in a different hash because those blocks have a different rlp encoding since they lack certain fields.

This is a problem that seems to be recurring, initially when I changed the block encoding I had to modify a number of tests which was not nice, and now new tests have been added which also require modifying.

So I'm starting to think that using the nilness/zeroness or not of a field to determine how to encode the block may not be the most future proof approach. The alternative would be to add a private field to the block struct to indicate that it is pre-gingerbread, we can do this easily when loading blocks from the db by introspecting the rlp data but I can't see any way to consistently set that when creating new blocks. So if we wanted to do this I think we would have to mandate that new block creating is not supported in a pre-gingerbread context. I.E. It's invalid to start a new chain where the gingerbread block is not set to zero.

@celo-org celo-org deleted a comment Aug 26, 2024
@lvpeschke lvpeschke added this to the 5 - Celo MVP Mainnet milestone Oct 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants
@karlb @lvpeschke and others