Test vector metadata consideration

[fselmo]

In our test vectors right now we define blocks objects with both actual block body fields and some "metadata" / extra fields:

• block body fields: {"blockHeader", "transactions", "uncleHashes", "withdrawals"}
• extra fields: {"rlp", "blockNumber"}

For Amsterdam, we had blockAccessList as part of the block body, but recent changes to the spec have removed it from the block body. It is still useful to reference when debugging errors against clients, otherwise we would only have the BAL hash in the block header, which doesn't give us a whole lot of information. Also this blockNumber seems to be redundant since it exists in the blockHeader.

---

This issue is meant as a tracker to try to reconcile how these vectors are organized into an agreeable format. I would propose that we move the extra fields into some _metadata field (underscored so that it is clearly not a part of the block body) and to remove the blockNumber field entirely- the net change being:

• Move rlp and blockAccessList into a new blocks[i]["_metadata"] field
• Remove blockNumber entirely since it exists at blocks[i]["blockHeader"]["number"]

I believe that moving rlp, specifically, into _metadata may break something on the client side due to expectations of where this lives, so this may require some coordination with clients or sanity checking via hive, etc.

[fselmo]

Also, for BALs, we may now need a blockAccessListRlp to set up a sidecar for consume rlp since the BAL is no longer a part of the block body. Since consume rlp pulls the rlp field and sets up blocks/001.rlp (and so on...), we may need to pull out the blockAccessListRlp and set up a directory where clients can read this data as an rlp sidecar - perhaps at block_access_lists/001.rlp or something like this.

This would add yet another field to the object inside blocks. _metadata or a similar field would be a nice way to organize these extra fields that are not part of the block body.

[spencer-tb]

I think we should make clients regenerate for rlp imo - so no sidecar. As consume rlp is essentially testing historical sync (and afaik clients would regenerate BALs for historical sync in prod) then it seems appropriate to align them both. It feels like there is benefit to test that clients regenerate BALs:

• consume-rlp: RLP decode + sequential execution + BAL regeneration
• consume-engine: Engine API + BAL provided + parallel execution (enables)

To me it also makes sense to keep the BAL field in the block for debugging. I don't think we need to worry about adding a _metadata field currently, its a really nice idea but I think it would add more work to our plate, we should reconsider if we have extra fields. Given that we have rlp already in the block I see no reason why we cant add a blockAccessList field here as well. We'd just need to notify EL client teams of the format change (this seems like the less likely breaking change).

Wdyt?

[danceratopz]

I think we should make clients regenerate for rlp imo - so no sidecar. As consume rlp is essentially testing historical sync (and afaik clients would regenerate BALs for historical sync in prod) then it seems appropriate to align them both.

Agree this is the right approach. It could be worth getting input from other client teams nonetheless.

FYI, there is some explanation of the code paths that should be tested (it can't be enforced) here:
https://github.com/ethereum/execution-specs/blob/forks/amsterdam/docs/running_tests/running.md#code-path-choices

This topic has come up repeatedly so it's worth a discussion with client teams in its own right, tbh!

Given that we have rlp already in the block I see no reason why we cant add a blockAccessList field here as well.

This would be really nice for debugging, but is of limited use in isolation(?). However, if clients implement eth_getBlockAccessListByNumber, (ethereum/execution-apis#726) then we could improve the simulator and provide a full diff upon a block_access_list_hash mismatch.

[fselmo]

Thanks for the input here.

I think we should make clients regenerate for rlp imo - so no sidecar. As consume rlp is essentially testing historical sync (and afaik clients would regenerate BALs for historical sync in prod) then it seems appropriate to align them both. It feels like there is benefit to test that clients regenerate BALs

Yeah this sounds good to me too. I think this won't be an issue for clients as they still need to be able to execute blocks without BALs for historical blocks. We came up with this same proposal in chats with client teams. This is likely the path that we'll take.

Given that we have rlp already in the block I see no reason why we cant add a blockAccessList field here as well.

This is already the case for Amsterdam tests. This issue is more about asking if we should clean up the fields a bit. blocknumber seems unnecessary and, though blocks objects can be "anything" I suppose, there are only 2 deviations from an actual block body - so it's a bit confusing hence moving them over into a metadata field.

Overall this is a nit on the structure of the tests which would end up requiring a decent amount of coordination. I don't think it's time to do anything like this yet, especially since it doesn't look like we will need the blockAccessListRlp. We can let this sit for now.

[spencer-tb]

From @danceratopz:

This would be really nice for debugging, but is of limited use in isolation(?). However, if clients implement eth_getBlockAccessListByNumber, (ethereum/execution-apis#726) then we could improve the simulator and provide a full diff upon a block_access_list_hash mismatch.

Very much agree that the RPC calls would be beneficial for debugging! Besu has a PR for this: hyperledger/besu#9643

Overall this is a nit on the structure of the tests which would end up requiring a decent amount of coordination. I don't think it's time to do anything like this yet, especially since it doesn't look like we will need the blockAccessListRlp. We can let this sit for now.

After thinking on it if clients are putting in effort to read a different field anyway then a small format change is worth it.

[fselmo]

@danceratopz:

However, if clients implement eth_getBlockAccessListByNumber, (ethereum/execution-apis#726) then we could improve the simulator and provide a full diff upon a block_access_list_hash mismatch.

100%. This is the way 🙂.

@spencer-tb:

After thinking on it if clients are putting in effort to read a different field anyway then a small format change is worth it.

For sure. This was the whole reason I wrote this issue 😅. I could've been more clear here, that's my fault. Yeah the field was already added to the test vectors. The goal here was, since we are already making changes, instead of shoving new fields into this awkward "pseudo block body", what if we take this time to organize things a bit more? If we wanted to, what would that look like?

I'm not set on it, but put it up here as food for thought.