celo-org / op-geth

GNU Lesser General Public License v3.0
0 stars 0 forks source link

Handle migrated celo headers #143

Closed piersy closed 3 months ago

piersy commented 3 months ago

This PR updates the header encoding and decoding for celo blocks to be able to support data migrated from celo-blockchain.

Our post gingerbread headers actually decode into the op-geth header structure without any changes. Since adding fields to the end of a struct does not cause rlp decoding to fail for previously encoded values. Our pre-gingerbread don't decode into the op-geth header structure because the pre-gingerbread headers miss fields from the middle of the struct so we need to handle those explicitly.

The decision on how to decode is made based on introspecting the rlp and looking at the second field which is a hash (length 32 bytes) post-gingerbread, but an address (length 20 bytes) pre-gingerbread.

The decision on how to encode is made based on the value of gasLimit if zero then it's a pre-gingerbread header.

Additionally we needed to allow genesis blocks in a CeL2 migrated chain context (by checking if Cel2Time is set and non zero) that lack GasLimit and Difficulty because those fields were considered required by op-geth but not present in early Celo.

Finally some tests that used blocks with zero gas limits needed to be updated since with a zero gas limit the different encoding was now breaking them, and adding a non zero gas limit meant the block hash changed.

palango commented 3 months ago

The decision on how to encode is made based on the value of gasLimit if zero then it's a pre-gingerbread header.

I thought we would set that to the correct value, so that the apis return something useful. Or is this done somewhere in the API layer?

Additionally we needed to allow genesis blocks in a CeL2 context (by checking if Cel2Time is set) that lack GasLimit and Difficulty because those fields were considered required by op-geth but not present in early Celo.

Is this requires so that the genesis hash stays the same, or why can't we just add those values to the celo genesis block?

Many tests using TestChainConfig failed because it set Cel2Time and that caused genesis specs without gas limit or difficulty set to not have a default applied in Genesis.ToBlock

For this you can use TestChainConfigNoCel2, so that you don't need to touch those tests.

piersy commented 3 months ago

The decision on how to encode is made based on the value of gasLimit if zero then it's a pre-gingerbread header.

I thought we would set that to the correct value, so that the apis return something useful. Or is this done somewhere in the API layer?

So we decided that we wouldn't add this to the encoded representation since it then breaks the assumption that hash(rlp(header)) == header.Hash(). In fact we strip out the IstanbulAggregatedSeal during migration so that hash(rlp(header)) == header.Hash().

We can still return this at the api layer, but we are not going to put it in the rlp encoded header representation.

Additionally we needed to allow genesis blocks in a CeL2 context (by checking if Cel2Time is set) that lack GasLimit and Difficulty because those fields were considered required by op-geth but not present in early Celo.

Is this requires so that the genesis hash stays the same, or why can't we just add those values to the celo genesis block?

It is precisely for the genesis hash, if those fields are set then the hash changes and then you can't connect to the network due to a genesis hash mismatch.

Many tests using TestChainConfig failed because it set Cel2Time and that caused genesis specs without gas limit or difficulty set to not have a default applied in Genesis.ToBlock

For this you can use TestChainConfigNoCel2, so that you don't need to touch those tests.

Ok can have a look at how that works out.