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.

Sign up for GitHub

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 encoding of access_lists #1263

Closed
wants to merge 3 commits into from
Closed

Fix rlp encoding of access_lists #1263

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dd52376
Fix RLP encoding for access lists
Nashtare Sep 30, 2023
1c76d15
Derive Clone for Rlp structs
Nashtare Sep 30, 2023
7dcf03b
Clippy
Nashtare Oct 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
45 changes: 38 additions & 7 deletions evm/src/generation/mpt.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,13 +33,44 @@ impl Default for AccountRlp {
}
}

#[derive(RlpEncodable, RlpDecodable, Debug)]
#[derive(Clone, Debug)]
pub enum AccessListInner {
List(Vec<AccessListItemRlp>),
Item(AccessListItemRlp),
}

impl Encodable for AccessListInner {
fn rlp_append(&self, s: &mut RlpStream) {
match self {
AccessListInner::List(list) => s.append_list(list),
AccessListInner::Item(item) => s.append(item),
};
}
}

impl Decodable for AccessListInner {
fn decode(rlp: &Rlp) -> Result<Self, DecoderError> {
if rlp.is_list() {
Copy link
Contributor

Choose a reason for hiding this comment

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

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
}

let bytes = rlp.at(0)?;
let access_list: Vec<AccessListItemRlp> = bytes.as_list()?;
return Ok(AccessListInner::List(access_list));
}
if rlp.is_data() {
let bytes = rlp.at(0)?;
let access_list = bytes.as_val::<AccessListItemRlp>()?;
return Ok(AccessListInner::Item(access_list));
}
Err(DecoderError::RlpExpectedToBeData)
}
}

#[derive(Clone, RlpEncodable, RlpDecodable, Debug)]
pub struct AccessListItemRlp {
pub address: Address,
pub storage_keys: Vec<U256>,
}

#[derive(Debug)]
#[derive(Clone, Debug)]
pub struct AddressOption(pub Option<Address>);

impl Encodable for AddressOption {
Expand Down Expand Up @@ -67,7 +98,7 @@ impl Decodable for AddressOption {
}
}

#[derive(RlpEncodable, RlpDecodable, Debug)]
#[derive(Clone, RlpEncodable, RlpDecodable, Debug)]
pub struct LegacyTransactionRlp {
pub nonce: U256,
pub gas_price: U256,
Expand All @@ -80,7 +111,7 @@ pub struct LegacyTransactionRlp {
pub s: U256,
}

#[derive(RlpEncodable, RlpDecodable, Debug)]
#[derive(Clone, RlpEncodable, RlpDecodable, Debug)]
pub struct AccessListTransactionRlp {
pub chain_id: u64,
pub nonce: U256,
Expand All @@ -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>,
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good idea 👍

pub y_parity: U256,
pub r: U256,
pub s: U256,
}

#[derive(RlpEncodable, RlpDecodable, Debug)]
#[derive(Clone, RlpEncodable, RlpDecodable, Debug)]
pub struct FeeMarketTransactionRlp {
pub chain_id: u64,
pub nonce: U256,
Expand All @@ -105,7 +136,7 @@ pub struct FeeMarketTransactionRlp {
pub to: AddressOption,
pub value: U256,
pub data: Bytes,
pub access_list: Vec<AccessListItemRlp>,
pub access_list: Vec<AccessListInner>,
pub y_parity: U256,
pub r: U256,
pub s: U256,
Expand Down
Loading