Fix rlp encoding of access_lists #1263
Conversation
| @@ -89,13 +120,13 @@ pub struct AccessListTransactionRlp { | |||
| pub to: AddressOption, | |||
| pub value: U256, | |||
| pub data: Bytes, | |||
| pub access_list: Vec<AccessListItemRlp>, | |||
| pub access_list: Vec<AccessListInner>, | |||
There was a problem hiding this comment.
I'm a bit confused by this. EIP-2930 says that the format for access_list is [[{20 bytes}, [{32 bytes}...]]...], i.e., Vec<AccessListItemRlp>. But this change seems to indicate that it could also be something like Vec<Vec<AccessListItemRlp>>.
Are there txns with this access list format?
There was a problem hiding this comment.
I agree, this doesn't look right: like it stands this would be accepted: vec![AccessListInner::List(vec![AccessListItemRlp{address: …, storage_keys: …}, AccessListItemRlp{address: …, storage_keys: …}])], which seems odd.
There was a problem hiding this comment.
I agree that this isn't the regular encoding, but the EVM test suite has some weird block RLP encodings on some of its tests, which either need to be ignored when parsing or require this specific handling.
The other weird encoding is at the transactions section of the block, I've seen so far three versions:
- a list of txns
- a single txn
- a list of strings (ie doubly encoded)
There was a problem hiding this comment.
What I could do then, if just port these changes directly to evm-tests instead. I need some logic there anyway to handle the non-RLP prefix for those transactions, so it wouldn't add too much, and at least here we'd respect the specs.
There was a problem hiding this comment.
Yeah I think that's probably a better idea. These are public structs so I feel like we should have them follow the specs and handle the test suite's oddities in the evm-tests repo.
There was a problem hiding this comment.
Also they are public only for integration tests I believe, right? If so we may want to place them into a testing module to emphasize they aren't really aimed at being used.
|
|
||
| impl Decodable for AccessListInner { | ||
| fn decode(rlp: &Rlp) -> Result<Self, DecoderError> { | ||
| if rlp.is_list() { |
There was a problem hiding this comment.
If this is in the hot loop it can probably be optimized a bit by checking is_empty() once and then switch based on byte 0 instead, like:
if rlp.is_empty() { return DecoderError:RlpExpectedToBeData }
// Can only be list or data now and byte 0 is != 0xc0
if rlp.as_raw[0] > 0xc0 {
// do list stuff
} else {
// do data stuff
}
|
Porting the changes to evm-tests instead. |
Also derive
Cloneto be reused on the test-runner side.