-
Notifications
You must be signed in to change notification settings - Fork 795
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
Change structure of Transactions
for faster block decoding
#6609
base: unstable
Are you sure you want to change the base?
Conversation
Paul the Optimiser returns 🤩 |
omg I did not mean to close that, sorry! |
This is currently blocked on sigp/tree_hash#18 |
Merged the tree_hash PR and a v0.9.0 release is building on CI now |
Updated ssz_types, but Milhouse will still need an update |
Merged latest unstable into this because I'd like to base some other changes off the latest Milhouse version. Oddly I'm now getting Clippy failures for |
@@ -29,6 +27,64 @@ pub struct Checkpoint { | |||
pub root: Hash256, | |||
} | |||
|
|||
/// Use a custom implementation of SSZ to avoid the overhead of the derive macro. |
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 can't spot the issue with this impl at a glance, but when I tried to sync a node with this change I got:
Jan 20 06:23:41.955 WARN BlockProcessingFailure outcome: DBError(SszDecodeError(OffsetIntoFixedPortion(156))), msg: unexpected condition in processing block
I'm fairly sure it's this change, because I tried reverting the transaction changes, and the error persisted. And then when I reverted this change, it went away.
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.
Ah, it's because we're missing the impl of ssz_fixed_len
:
https://docs.rs/ethereum_ssz/0.8.2/ssz/trait.Encode.html#method.ssz_fixed_len
Classic footgun, I think we've hit this multiple times. We should probably make that method mandatory, or combine it with is_ssz_fixed_len
like:
enum SszLength {
VariableLen,
FixedLen(usize),
}
pub fn ssz_len() -> SszLength {
SszLength::FixedLen(..)
}
Issue Addressed
NA
Proposed Changes
Reduces the time to decode a block by ~15% by reducing the effort needed to decode
execution_payload.transactions
.Previously we stored transactions as
VariableList<VariableList>
. Now we store them as(Vec<offsets>, Vec<values>)
, where eachoffset
points to the start of a new transaction invalues
. This is how the data is represented as SSZ.The prior implementation required
N+1
allocations, where this requires2
allocations.Additional Info
NA
Before
After