-
Notifications
You must be signed in to change notification settings - Fork 138
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
refactor: transition from open_ethereum primatives (types/rlp) to Reth's Alloy (types/rlp) #1231
Conversation
3788a50
to
0f1631c
Compare
cb9783f
to
e011600
Compare
3ff205d
to
dd830d4
Compare
In general, I like the idea and the initiative. However, I have some concerns. There are several crates that are referencing your private repo, meaning they wouldn't get any updates / bug fixes. I checked few and they don't have only trivial changes. Is there a plan for those crates to migrate to Maybe update the PR description with each such crate and plan for it for future. |
The only one that doesn't have trivial changes is eth-trie.rs which we maintain. That is because it also required me to switch updated its RLP. The rest of the libraries mentioned are maintained by sigma prime and the changes are trival just changing openethereum_types to alloy-primative types The libraries maintained by sigma prime are
I made PR's for all 4, it took less then an hour combined to convert all of them from ethereum-types to Reth's alloy types. So yeah the only "private repo" with "don't have only trivial changes" is eth-trie.rs which we maintain. So I am assuming few means 1.
To my knowledge sigp is going to upgrade sigp/(ethereum_ssz/ethereum_serde_utils/tree_hash/ssz_types) I made PR's for it. They already upgraded sig/(enr/discv5). sigp/(ethereum_ssz/ethereum_serde_utils/tree_hash/ssz_types) are very stable and aren't in active development either, so I think we are good in that regard. I will reach out to sigp to get a more definitive answer, and update here on what I receive. I don't think the answer will be a blocker for this PR though. |
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 have a few questions:
- What is the motivation behind migrating from
ethereum_types
toalloy-primitives
?
sigp in the process of updating from open_ethereum primatives (types/rlp) to Reth's Alloy (types/rlp). Currently sigp/(enr/discv) are updated, but lighthouse is planning to upgrade as well.
-
Could you link to PR/issue about that. What I found is this but they migrate only
rlp
toalloy-rlp
. -
Is it possible to migrate only the rlp crate and at a later stage to decide what to do with the primitive types?
Also so we can use the reth's REVM crate and json-rpc types seemlessly instead of a bunch of type conversion boilerplate.
Discv5 doesn't use ethereum-types. I want to emphasize even if lighthouse aligns with this change or not that shouldn't impact the decision. We are an execution layer focused project and being able to use a lot of Reth's crates going forward will be a massive help. The 4 sigp libraries are very small, drops in a bucket compared to the REVM, even the json-rpc-types crate we want to use. I asked and here is the response I got
The truth being Parity hasn't in the Ethereum space for 4 years now. So I think it is clear we want to use Reth's primatives expessially with our growing interest in the REVM I see this as invaluable. To answer the question directly |
I agree that this is a good direction to go forward but I think the switch should be done more carefully. This is a core change to our codebase and we should be more cautious relying on custom forks for core libraries like What problem are we trying to solve here? We want to fix an era1 decoding bug. Is this the simplest solution to this problem with the least friction? Is the alloy types migration at the top list of our priorities? I would wait for sgp to update their crates before moving with this change and in the meantime, I'm more comfortable applying a quick fix for the era1 decoding issue. |
I would say I'm with @ogenev on this one. I noticed that Would it be possible to migrate in smaller chunks? Crate-by-crate would probably not work, but maybe structure-by-structure, i.e. each subnetwork separately. Another related topic |
Those libraries don't really change, they maybe add 1 thing every year or 6 months, and normally that is a new "feature" we don't need. The only change I made in those 4 libraries is cntr-alt-r open-ethereum types with reth's alloy types. This isn't the first time we have relied on custom forks well we are waiting for changes to be implemented on the main repo's. PR's are made on all the repos with the changes to reth's alloy types. On Thursday call if we want we can go over sigp's crates commit history to look at the changes. No meaningful changes have been made to those repos in years. ssz is simple serialization there isn't much to work on or change. I will take on the responsibility to work with sigp and maintain our forks for the forseeable future if required, till alloy is merged into them.
This is the simplest solution as the work is already done and tested
After we launch history I will be going to work on the state network assuming everything goes to plan. REVM uses reth's alloy types/rlp not open-ethereums. This PR is the setup to work with Reth's crates. A majority of their work is in modular crates and switching to their types allows us to easily taken advantage of them.
This is the quick fix as the work is already done and tested. As stated above the sigp crates we rely on hardly change and when they do it is often a new "feature" we don't need. I am going to work with Sigp to resolve this for us, but I don't think this should block this PR. TLDR
That is to name a few examples, but this PR opens doors beyond the examples I gave today, parity hasn't been in the ethereum ecosystem for 4 years, we are an execution aligned project, it makes sense to change to these types so we can reuse Reth's execution layer code. I know this PR is "big", but it is unavoidable and this technical debt only grows overtime. The work is done. |
Are we not able to use this https://github.com/paradigmxyz/reth/tree/main/crates/trie ? Reth has crates for all things execution layer, which is exactly why I think we should make this change. So we can reuse work already done instead of reinventing the wheel. |
I wasn't aware of that crate, probably because it's not published. I will look into it today. |
It seems not as optimized yet so I would prefer the forks |
I looked into it, and we can likely use it. But, it's not really created for our use case and it comes with extra cost. Currently, and I'm not sure this will change any time soon, we only need to traverse certain path in a trie and verify that result is what it needs to be. And also, I'm not sure if we want to use They all use alloy types, so transition would be useful anyway. All I'm saying is that we might not need to be so strongly integrated with |
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.
More of an initial pass-through. Mostly some comments and suggestions for using a different api. Imo, it cuts down the diff and improves readability if we're using the same api for the same functionality throughout the codebase. As to the status of this pr, we can discuss it during sync tm to see if we can reach consensus on how to handle it.
} | ||
|
||
#[derive(Clone, Debug, PartialEq, Eq, Deserialize)] | ||
#[derive(Clone, Debug, PartialEq, Eq, Deserialize, RlpEncodable, RlpDecodable)] |
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 do remember that we needed to explicitly impl Encodable
/ Decodable
here. Unfortunately, I can't remember exactly the reason why 😅 ... But this is another example of a change that seems like it makes sense, but unless it's explicitly covered in testing (and I don't immediately see any BlockBodyMerge
tests down below) then it could lead to some rather tedious debugging down the road
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 is changed in the era1 rlp fix PR
The only "non-trivial" change is in 1 function We can review the changes in this 1 function. I already tested the changes with our Portal-Hive state tests and the update to alloy/rlp passes all successfully.
I think writing a whole new merkle trie library is far more "non-trivial" then updating the rlp library eth-trie.rs uses and just reviewing it.
I am very confused how 1 function changing from open-ethereum/rlp to alloy/rlp is a more complex change then writing a brand new library. Verkle is coming in a year? no? Won't that make a merkle based state network obsolete? So why does a library not designed for our specific temporary usecase, but works for our specific usecase justify building a special solution. My main point is I don't think this blocks this PR. And I am not sure why the complexity of the changes in The changes in |
We already/will use
If we don't want to use the
Reth is modular. I am not sure how using a crate is the same thing as Crates are just tools, Reth just happens to build a lot of tools. If there is a tool we can use to get something done faster I see that as a win. We don't want to rebuild everything from scratch unless there is a reason for it. Portal is a mainly execution layer aligned project. So it makes sense an ethereum execution layer client (Reth) with a modular design would build a lot of tools we can use |
…h's Alloy (types/rlp)
dd830d4
to
9a23c94
Compare
9a23c94
to
5f0c00c
Compare
I implemented the era1 change using open-ethereum/rlp I don't think it makes sense to use open-ethereum rlp, for all the reasons above. 3.64 times slower is no joke. |
@@ -248,19 +211,23 @@ impl ssz::Decode for BlockBodyLegacy { | |||
let uncles: Vec<u8> = decoder.decode_next()?; | |||
let txs: Vec<Transaction> = txs | |||
.iter() | |||
.map(|bytes| rlp::decode(bytes)) | |||
.map(|bytes| Decodable::decode(&mut bytes.as_slice())) |
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.
Well, personally I'd like to see alloy_rlp::decode()
here, but that doesn't seem to be an available method, which is an odd api choice imo....
@@ -475,15 +393,15 @@ mod tests { | |||
#[case(TX6, 41942)] | |||
fn encode_and_decode_txs(#[case] tx: &str, #[case] expected_nonce: u32) { | |||
let tx_rlp = hex_decode(tx).unwrap(); | |||
let tx = rlp::decode(&tx_rlp).expect("error decoding tx"); | |||
let tx = Transaction::decode(&mut tx_rlp.as_slice()).expect("error decoding tx"); |
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 Decodable
over Transaction
here
rlp::decode(&hex_decode(TX17).unwrap()).unwrap(), | ||
rlp::decode(&hex_decode(TX18).unwrap()).unwrap(), | ||
rlp::decode(&hex_decode(TX19).unwrap()).unwrap(), | ||
Transaction::decode(&mut hex_decode(TX1).unwrap().as_slice()).unwrap(), |
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.
Decodable
> Transaction
} else { | ||
s.begin_list(stream_length_without_seal); | ||
impl Encodable for Header { | ||
fn encode(&self, out: &mut dyn bytes::BufMut) { |
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'm not sure we want to get rid of the with_seal
bool, why was it removed?
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.
It wasn't used
Receipt::decode(&hex_decode(RECEIPT_16).unwrap()).unwrap(), | ||
Receipt::decode(&hex_decode(RECEIPT_17).unwrap()).unwrap(), | ||
Receipt::decode(&hex_decode(RECEIPT_18).unwrap()).unwrap(), | ||
Receipt::decode(&mut hex_decode(RECEIPT_0).unwrap().as_slice()).unwrap(), |
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.
Decodable
> Receipt
in the following 2 lists, and for all the following uses of Receipt::decode()
in these tests
use rlp_derive::{RlpDecodable, RlpEncodable}; | ||
use alloy_primitives::{keccak256, Address, B256, U256, U64}; | ||
use alloy_rlp::{ | ||
length_of_length, Decodable, Encodable, Error as RlpError, Header, RlpDecodable, RlpEncodable, |
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.
Header as RlpHeader
keccak_hash::keccak(rlp::encode(self)) | ||
pub fn hash(&self) -> B256 { | ||
let mut buf = vec![]; | ||
self.encode(&mut buf); |
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.
alloy_rlp::encode()
fn rlp_append(&self, s: &mut RlpStream) { | ||
fn encode(&self, out: &mut dyn bytes::BufMut) { | ||
// we don't wrap versioned transactions with a string header | ||
let with_header = false; |
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.
then why do we have an if with_header {}
clause later? with_header
will never evaluate to true
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.
We use it in the era 1 fix PR #1231 (comment)
fn decode(buf: &mut &[u8]) -> alloy_rlp::Result<Self> { | ||
if let Some(&first) = buf.first() { | ||
if first == EMPTY_STRING_CODE { | ||
buf.advance(1); |
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.
Why are we advancing the buf
here? I'm just not sure that there's a test for this code path, or did you find one and this was required to get it passing?
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 thought it was cleaner then doing &mut &buf[1..]
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 it is less error prone as well
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.
Ok let's give this a go, main main comment is that I would like to see alloy-primitives
version updated to the latest minor 0.7.0
on all crates and see some tests for u256_from_dec_str.rs. ⛵
Cargo.toml
Outdated
@@ -13,12 +13,13 @@ categories = ["cryptography::cryptocurrencies"] | |||
description = "A Rust implementation of the Ethereum Portal Network" | |||
|
|||
[dependencies] | |||
alloy-primitives = "0.6.4" |
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.
There is a new latest 0.7.0 version, let's update it in this repo, and all forked repos.
Cargo.toml
Outdated
@@ -41,7 +41,7 @@ tempfile = "3.3.0" | |||
tokio = { version = "1.14.0", features = ["full"] } | |||
tracing = "0.1.36" | |||
tracing-subscriber = "0.3.15" | |||
tree_hash = "0.5.2" | |||
tree_hash = { git = "https://github.com/KolbyML/tree_hash.git", rev = "c402373ce2b430551eb8debd85512ceac9567210" } |
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 will comment on the tree_hash
fork code here.
On line 122, I think it is more efficient if we do the following:
let mut result = [0; HASHSIZE];
result.copy_from_slice(self.as_le_slice());
instead of:
let mut result = [0; HASHSIZE];
for (index, i) in self.as_le_slice().iter().enumerate() {
result[index] = i.clone();
}
ethportal-api/Cargo.toml
Outdated
ethereum_ssz_derive = "0.5.3" | ||
discv5 = { version = "0.4.1", features = ["serde"] } | ||
eth_trie = { git = "https://github.com/kolbyml/eth-trie.rs.git", rev = "a09b2365f1457cb83bae5ebfa2cf133a9dd1657f" } | ||
ethereum_serde_utils = { git = "https://github.com/KolbyML/ethereum_serde_utils.git", rev = "25b69f5dbd3ac871217ad24fc42304a93000d8da" } |
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 see that you added a new file u256_from_dec_str.rs in the ethereum_serde_utils
fork. I would like to see some tests added for this functionality before depending on it.
ethportal-api/Cargo.toml
Outdated
@@ -29,23 +30,21 @@ lazy_static = "1.4.0" | |||
nanotemplate = "0.3.0" | |||
quickcheck = "1.0.3" | |||
rand = "0.8.5" | |||
reth-rpc-types = { tag = "v0.1.0-alpha.10", git = "https://github.com/paradigmxyz/reth.git"} | |||
reth-rpc-types = { tag = "v0.2.0-beta.4", git = "https://github.com/paradigmxyz/reth.git"} |
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.
Did reth switch to beta.5
? Let's update it here too.
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.
Reth bumped the alloy version after the release of beta.5 so I updated it to the commit for the bump commit
What was wrong?
How was it fixed?
update all our code to use Reth alloy types and rlp instead of open ethereum's
why do we want this?
Open ethereum has been unmaintained for 4 years now. Switching to alloy (lighthouse is also planning to switch), will allow us to much more easily use libraries from Reth
A big reason is we can has a ton of crates we are going to want to use, this transition makes using Reth crates a billion percent easier.