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.

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

feat(consensus) implement RLP for Account information #789

Merged
merged 9 commits into from
May 29, 2024

Conversation

DoTheBestToGetTheBest
Copy link
Contributor

Closes #780

@DoTheBestToGetTheBest DoTheBestToGetTheBest changed the title Update account.rs feat(consensus) implement RLP for Account information May 27, 2024
Copy link
Member

@prestwich prestwich left a comment

Choose a reason for hiding this comment

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

Based on the reth struct here, we should be able to derive this implementation. However, we will need to re-order the fields. That seems fine to me

while we're here, I would also like to see a convenience function for trie_hash (similar to in Encodable2718

pub fn trie_hash_slow(&self) -> B256 {
    let mut buf = vec![];
    self.encode(&mut buf);
    keccak256(buf)
}

bonus points for memoizing the hash in a OnceLock

@Rjected @mattsse do you have any RLP encoding/trie hashing test vectors handy?

Copy link
Member

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

not sure, I don't think we use this type in reth because we don't store the account's storage root

maybe @rkrasiuk knows if we have some test vectors

but I wonder if we can use derive here?

@prestwich
Copy link
Member

prestwich commented May 28, 2024

not sure, I don't think we use this type in reth because we don't store the account's storage root

hmmm, this type is committed in the state MPT right? or am i misunderstanding?

@DoTheBestToGetTheBest
Copy link
Contributor Author

Based on the reth struct here, we should be able to derive this implementation. However, we will need to re-order the fields. That seems fine to me

while we're here, I would also like to see a convenience function for trie_hash (similar to in Encodable2718

pub fn trie_hash_slow(&self) -> B256 {
    let mut buf = vec![];
    self.encode(&mut buf);
    keccak256(buf)
}

bonus points for memoizing the hash in a OnceLock

@Rjected @mattsse do you have any RLP encoding/trie hashing test vectors handy?

Thank you very much for your review i added the trie_hash and trie_hash_slow methods

@DoTheBestToGetTheBest
Copy link
Contributor Author

but I wonder if we can use derive here?

should i directly derive ?

@prestwich
Copy link
Member

yes, please derive and reorder the fields to match reth as linked above

@DoTheBestToGetTheBest
Copy link
Contributor Author

DoTheBestToGetTheBest commented May 28, 2024

yes, please derive and reorder the fields to match reth as linked above

Thank you very much guys for the review of the PR.

I reorder the field similar to reth struct and i derived instead of manual impl

@prestwich
Copy link
Member

thank you. last few things:

  • after some thought, I think we're not clear on our memoization strategy yet. So let's go ahead and remove the trie_hash fn, and leave trie_hash_slow. This will give us some time to think about how we want to do this across the code base.

  • I would like a couple tests, because this is consensus critical code, and used in reth consensus at some point. You don't have to provide tests right now. I can open a follow up issue.

@prestwich
Copy link
Member

oh and for no_std support in CI, need to add a

#[cfg(not(feature = "std"))]
use alloc::{vec, vec::Vec};

@DoTheBestToGetTheBest
Copy link
Contributor Author

thank you. last few things:

Thank you very much for you review & your patience !

  • after some thought, I think we're not clear on our memoization strategy yet. So let's go ahead and remove the trie_hash fn, and leave trie_hash_slow. This will give us some time to think about how we want to do this across the code base.

No problem !

  • I would like a couple tests, because this is consensus critical code, and used in reth consensus at some point. You don't have to provide tests right now. I can open a follow up issue.

Thank you, when you open issue about it if it's possible to link any example of test like this i'd like to take that new issue :)

pub fn trie_hash_slow(&self) -> B256 {
let mut buf = vec![];
self.encode(&mut buf);
let mut hasher = Keccak256::new();
Copy link
Member

Choose a reason for hiding this comment

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

nit:
we usually write this using the conveniencealloy_primitives::keccak256 instead of invoking Keccak256::new() and update

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nit: we usually write this using the conveniencealloy_primitives::keccak256 instead of invoking Keccak256::new() and update

Thank you very much, i changed !

@prestwich
Copy link
Member

Thank you, when you open issue about it if it's possible to link any example of test like this i'd like to take that new issue :)

this is what i was asking mattsse and rjected about earlier :) I am not sure where to link for testvectors for account encoding

@DoTheBestToGetTheBest
Copy link
Contributor Author

Thank you, when you open issue about it if it's possible to link any example of test like this i'd like to take that new issue :)

this is what i was asking mattsse and rjected about earlier :) I am not sure where to link for testvectors for account encoding

Yep gotcha, ty very much for all support on this !

@prestwich prestwich merged commit 45c78c6 into alloy-rs:main May 29, 2024
24 checks passed
ben186 pushed a commit to ben186/alloy that referenced this pull request Jul 27, 2024
* Update account.rs

* fix clippy

* Update account.rs

* Update account.rs

* Update account.rs

* Update account.rs

* Update account.rs

* Update account.rs

* Update account.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature] Implement RLP for new account type
3 participants