-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: add support for Eth69 receipts representation #15619
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
base: main
Are you sure you want to change the base?
feat: add support for Eth69 receipts representation #15619
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great start, but we now must also duplicate the _inner functions without the bloom
see:
https://github.com/ethereum/EIPs/blob/master/EIPS/eip-7642.md#specification
} | ||
|
||
fn encode_2718(&self, out: &mut dyn BufMut) { | ||
self.rlp_encode_with_bloom(&self.bloom(), out); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this impl should use the bloom, instead we need to encode all of the fields without the bloom
basically just this without the bloom:
reth/crates/ethereum/primitives/src/receipt.rs
Lines 44 to 50 in 460f840
/// RLP-encodes receipt fields with the given [`Bloom`] without an RLP header. | |
pub fn rlp_encode_fields(&self, bloom: &Bloom, out: &mut dyn BufMut) { | |
self.success.encode(out); | |
self.cumulative_gas_used.encode(out); | |
bloom.encode(out); | |
self.logs.encode(out); | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@klkvr could you please check this, I can feel my IQ points drop just by looking at this -.-
|
||
impl Encodable2718 for Receipt { | ||
fn encode_2718_len(&self) -> usize { | ||
!self.tx_type.is_legacy() as usize + self.rlp_encoded_fields_length_without_bloom() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure this is correct because this doesn't include the rlp header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah this should be smth like
!self.tx_type.is_legacy() as usize + self.rlp_encoded_fields_length_without_bloom() | |
!self.tx_type.is_legacy() as usize + self.rlp_header_inner_without_bloom().length_with_payload() |
Self::Deposit(receipt) => { | ||
receipt.inner.status.length() + | ||
receipt.inner.cumulative_gas_used.length() + | ||
receipt.inner.logs.length() | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should encode deposit nonce and version, if those are present
receipt.inner.status.encode(out); | ||
receipt.inner.cumulative_gas_used.encode(out); | ||
receipt.inner.logs.encode(out); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
deposit_nonce: None, | ||
deposit_receipt_version: None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thiose should be decoded and not always set to None
@klkvr updated :) |
|
||
impl Encodable2718 for Receipt { | ||
fn encode_2718_len(&self) -> usize { | ||
self.rlp_header_inner_without_bloom().length_with_payload() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should still account for the type byte (!self.tx_type.is_legacy()
)
if let Some(nonce) = receipt.deposit_nonce { nonce.length() } else { 0 } + | ||
if let Some(version) = receipt.deposit_receipt_version { | ||
version.length() | ||
} else { | ||
0 | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's use map_or
here
let mut deposit_receipt_version = None; | ||
|
||
// For deposit receipts, try to decode nonce and version if they exist | ||
if tx_type == OpTxType::Deposit && buf.len() + header.payload_length < remaining { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should be buf.len() + header.payload_length > remaining
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer us to have some roundtrips here to catch such stuff
closes #15509