feat(revm): Integrate BundleBuilder #4647

alessandromazza98 · 2023-09-19T10:15:32Z

Not 100% sure everything is correct. Let me know WDYT, thanks!

codecov · 2023-09-19T10:24:20Z

Codecov Report

Attention: Patch coverage is 97.70992% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 67.26%. Comparing base (4dc15c3) to head (39f1d69).
Report is 1376 commits behind head on main.

❗ Current head 39f1d69 differs from pull request most recent head 17daaac. Consider uploading reports for the commit 17daaac to get more accurate results

Additional details and impacted files

Files	Coverage Δ
...der/src/bundle_state/bundle_state_with_receipts.rs	`92.13% <ø> (+80.04%)`	⬆️
crates/storage/provider/src/test_utils/blocks.rs	`80.58% <100.00%> (+80.58%)`	⬆️
bin/reth/src/init.rs	`97.41% <93.33%> (+49.05%)`	⬆️
...torage/provider/src/providers/database/provider.rs	`76.90% <96.66%> (+49.42%)`	⬆️

... and 478 files with indirect coverage changes

Flag	Coverage Δ
integration-tests	`15.36% <0.00%> (-10.71%)`	⬇️
unit-tests	`62.60% <97.70%> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
reth binary	`30.63% <93.33%> (+4.85%)`	⬆️
blockchain tree	`80.63% <ø> (+52.16%)`	⬆️
pipeline	`88.45% <ø> (+83.40%)`	⬆️
storage (db)	`74.36% <98.27%> (+44.39%)`	⬆️
trie	`94.48% <ø> (+71.94%)`	⬆️
txpool	`48.52% <ø> (+7.14%)`	⬆️
networking	`76.15% <ø> (+45.25%)`	⬆️
rpc	`58.45% <ø> (+31.97%)`	⬆️
consensus	`63.01% <ø> (+37.94%)`	⬆️
revm	`28.46% <ø> (+18.61%)`	⬆️
payload builder	`7.96% <ø> (-6.20%)`	⬇️
primitives	`85.42% <ø> (+56.24%)`	⬆️

gakonst

thx for taking this on! some questions and nits.

gakonst · 2023-09-22T16:59:27Z

bin/reth/src/init.rs


    for (address, account) in &genesis.alloc {
        let bytecode_hash = if let Some(code) = &account.code {
            let bytecode = Bytecode::new_raw(code.0.clone());
            let hash = bytecode.hash_slow();
-            contracts.insert(hash, bytecode);
+            bundle_builder = bundle_builder.contract(hash, bytecode.0);


nit (cc @rakita) we should make the fn contract on the bundle builder work with something like AsRef so that you don't need to do .0 here

should I create an issue on Revm for this?

created here: bluealloy/revm#782

It feels strange to do AsRef when we need a type. Would recommend adding Into for reth Bytecode.

thanks, I added a .into() and the related impl from Bytecode for RevmBytecode.

gakonst · 2023-09-22T16:59:43Z

bin/reth/src/init.rs

@@ -114,39 +111,30 @@ pub fn insert_genesis_state<DB: Database>(
                m.iter()
                    .map(|(key, value)| {
                        let value = U256::from_be_bytes(value.0);
-                        (*key, (U256::ZERO, value))
+                        (U256::from_be_bytes(key.0), (U256::ZERO, value))


won't be necessary post alloy transition

alloy transition is now done, can you check if you can get rid of this change now?

gakonst · 2023-09-22T17:01:00Z

crates/storage/provider/src/bundle_state/bundle_state_with_receipts.rs

@@ -56,44 +42,6 @@ impl BundleStateWithReceipts {
        Self { bundle, receipts, first_block }
    }

-    /// Create new bundle state with receipts.
-    pub fn new_init(


crates/storage/provider/src/bundle_state/bundle_state_with_receipts.rs

crates/storage/provider/src/providers/database/provider.rs

gakonst · 2023-09-22T17:05:31Z

crates/storage/provider/src/providers/database/provider.rs

+                    bundle_builder = bundle_builder.state_original_account_info(
+                        address,
+                        into_revm_acc(old_info.unwrap_or_default()),
+                    );
+                    bundle_builder = bundle_builder.state_present_account_info(
+                        address,
+                        into_revm_acc(new_info.unwrap_or_default()),


Given the introduction of these, shouldn't let mut state be unnecessary, and as a result all the match state.entry calls? I would think that the bundle builder would be able to recognize where to put these internally, otherwise we're now double counting

yes you're right. I am now working on it. I'll soon push the update

gakonst · 2023-09-22T17:05:53Z

crates/storage/provider/src/providers/database/provider.rs

+                address,
+                if let Some(old_info) = old_info {
+                    Some(Some(into_revm_acc(old_info)))
+                } else {
+                    Some(None)
+                },
+            );


still don't love the Option<Option> in this API, would really like to do something about it -- cc @rakita for more thoughts

for this one we should change the revert_account_info. I could try think something about it today

Can you simplify this with a old_info.map(into_revm_acc)?

Yes, this could be Some(old_info.map(into_revm_acc))

alessandromazza98 · 2023-09-26T10:53:51Z

Right now I simply solved the problem related to mut state which effectively was useless since the use of BundleBuilder.

gakonst

ok w/ me @rakita do you mind taking a look at this as well? @alessandromazza98 also pls rebase to main and check if we can get rid of type conversions

gakonst · 2023-10-06T18:14:12Z

bin/reth/src/init.rs

@@ -114,39 +111,30 @@ pub fn insert_genesis_state<DB: Database>(
                m.iter()
                    .map(|(key, value)| {
                        let value = U256::from_be_bytes(value.0);
-                        (*key, (U256::ZERO, value))
+                        (U256::from_be_bytes(key.0), (U256::ZERO, value))


alloy transition is now done, can you check if you can get rid of this change now?

rakita · 2023-10-08T14:53:10Z

crates/storage/provider/src/providers/database/provider.rs

-                    entry.get_mut().0 = old_storage.value;
-                }
-            };
+            let present_info = plain_accounts_cursor.seek_exact(address)?.map(|kv| kv.1);


Does this part seems okay, we now always override the present and original state, while previously we did it only if there is no account inside state let account_state = match state.entry(address) {

ok, should have solved it now. I added a Vec<Address> that saves all seen addresses so that it's possible to filter out accounts already previously seen.

alessandromazza98 · 2023-10-09T08:35:10Z

Hi, sorry for delay, I was off this weekend. Yes, I'm gonna work on final comments and reviews

alessandromazza98 · 2023-10-09T11:27:38Z

I rebased on main and deleted type conversions whenever possible.

I also created the issue for Revm in order to be able to use the fn contract without doing bytecode.0. I already created the PR for that and I'm waiting for its approval.

When merged (if it's all right) I can also update this PR to use only bytecode and not bytecode.0

rakita

Just so we dont merge it, and not check if there is a bug here #4647 (comment)

crates/storage/provider/src/providers/database/provider.rs

rakita · 2023-10-11T08:26:50Z

crates/storage/provider/src/providers/database/provider.rs

-                .push(old_storage);
+            if !addresses_seen.contains(&address) {
+                let present_info = plain_accounts_cursor.seek_exact(address)?.map(|kv| kv.1);
+                bundle_builder = bundle_builder.state_original_account_info(


And you would need to save the address as seen here:
addresses_seen.push(address);

I would try to get this information from BundleBuilder sa previously we would use state, although this approach with addresses_seen does the job

Trying to use the BundleBuilder I would like to use the states field that actually contains all addresses seen, but its fields are private and not accessible and there aren't getter functions for it. Should I add a getter for states to the BundleBuilder?

getter would be good

bluealloy/revm#792

ok, now is it better to wait for next release of Revm and update the release in the general Cargo.toml or should I use the last commit in the specific Cargo.toml of provider folder in Reth in order to use the newly added getter?

You can specify the commit.

rakita · 2023-10-11T08:43:06Z

crates/storage/provider/src/providers/database/provider.rs

+                let new_info = plain_accounts_cursor.seek_exact(address)?.map(|kv| kv.1);
+                bundle_builder = bundle_builder.state_present_account_info(
+                    address,
+                    into_revm_acc(new_info.unwrap_or_default()),


This line does not look good. Having Some and having AccountInfo::default are two different account states. Should probably be handled differently

Looked at the BundleBuilder and in this case it would create Some(AccountInfo::default()) and I assume we would expect None:
https://github.com/bluealloy/revm/blob/main/crates/revm/src/db/states/bundle_state.rs#L166-L167

yes makes sense. I think it should work if first checking if new_info (or old_info and present_info later in the code) is Some and only if it is so, then do bundle_builder.state_present_account_info().

alessandromazza98 · 2023-10-17T08:43:27Z

Hi @rakita I pushed my last two commits. In the second one I update the Cargo.toml globally in order to use the bundle_builder.get_states() function. The problem is that there are some breaking changes done for examples in this PR: bluealloy/revm#775

So right now the ci does not succeed for that reason. And I cannot use the update version of revm only in the provider crate since otherwise it does not recognize the correct revm-primitives.

So maybe would be better to use the addresses_seen (HashSet) approach for now and only when you release the new version of revm, update all of it and here update the approach using bundle_builder.get_states().

What do you think?

idatsy · 2023-11-26T11:46:23Z

Hows it looking @alessandromazza98? Happy to help out on this

rakita

Merged with main and left comments.

rakita · 2023-11-27T12:30:07Z

crates/storage/provider/src/providers/database/provider.rs

+            if let Some(old_info) = old_info {
+                bundle_builder =
+                    bundle_builder.state_original_account_info(address, into_revm_acc(old_info));


This seems not correct, the Option around old_info is not properly used here, the Option represents if account was Some or None. I need to check original_account_info inside builder, maybe logic is there that would make this okay.

Scratch the last sentence, there is a problem here as we don't remove old_info from original_state. In essence, leaving the previous value where it should be removed (Set to None).

rakita · 2023-11-27T16:31:40Z

crates/storage/provider/src/providers/database/provider.rs

-                    }
+            let state_changeset = bundle_state.clone().into_plain_state(OriginalValuesKnown::No);
+            // revert account if needed.
+            for (address, account) in state_changeset.accounts {


Is this okay, or is there a better way, doesn't the bundle builder have all the needed info so we could skip both the cloning and the into_plain_state?

I mean by is it okay what happens if previous bundle state does not have account it would be None, but present plain state would contain it (it should remove it)?

rakita · 2023-11-27T16:34:56Z

crates/storage/provider/src/providers/database/provider.rs

-                    } else if existing_entry.is_some() {
-                        plain_accounts_cursor.delete_current()?;
-                    }
+            let state_changeset = bundle_state.clone().into_plain_state(OriginalValuesKnown::No);


it is called state_changeset but function is into_plain_state

gakonst · 2024-01-09T21:49:09Z

Hey @alessandromazza98 given we have @rakita comments above, anything blocking? Else @idatsy feel free to take over if Ale is not able to complete this

alessandromazza98 · 2024-01-10T07:25:32Z

Hei @gakonst, thanks for pinging me. I can't finish this issue now, @idatsy please go ahead with it, thanks a lot.

onbjerg · 2024-04-18T11:16:30Z

Closing this as stale. Feel free to let us know on the original issue if you end up picking this up again 😊 Thank you for taking a stab at it

alessandromazza98 requested review from rakita, joshieDo and onbjerg as code owners September 19, 2023 10:15

gakonst requested changes Sep 22, 2023

View reviewed changes

gakonst approved these changes Oct 6, 2023

View reviewed changes

rakita reviewed Oct 8, 2023

View reviewed changes

alessandromazza98 added 5 commits October 9, 2023 10:41

add BundleBuilder

9ad8d33

delete obsolete methods and types

1108d48

fix doctest

ac96bbc

fix clippy

f8397f5

delete mut state

1f23247

alessandromazza98 mentioned this pull request Oct 9, 2023

fn contract in bundle_state.rs should accept AsRef bluealloy/revm#782

Closed

rebase to main and delete type conversions when possible

960f4c2

alessandromazza98 force-pushed the bundle-builder branch from 8b4b0d5 to 960f4c2 Compare October 9, 2023 11:26

alessandromazza98 added 2 commits October 9, 2023 13:34

delete type conversions

21a151e

fix doctests

39f1d69

rakita requested changes Oct 9, 2023

View reviewed changes

alessandromazza98 added 3 commits October 10, 2023 15:01

only override state if it's first time to meet that account

8ae12f9

add from Bytecode for RevmBytecode impl

db983d2

fmt

1865458

rakita reviewed Oct 11, 2023

View reviewed changes

crates/storage/provider/src/providers/database/provider.rs Outdated Show resolved Hide resolved

rakita reviewed Oct 11, 2023

View reviewed changes

alessandromazza98 mentioned this pull request Oct 11, 2023

getter for states field of BundleBuilder bluealloy/revm#792

Merged

alessandromazza98 added 2 commits October 11, 2023 12:27

add address to bundlebuilder only if account info is Some(..)

aca2d5f

use bundle_builder.get_states()

f470b89

github-actions bot added the S-stale This issue/PR is stale and will close with no further activity label Nov 23, 2023

rakita added 2 commits November 27, 2023 13:06

Merge remote-tracking branch 'origin/main' into builder

a755757

import missing

17daaac

rakita requested changes Nov 27, 2023

View reviewed changes

github-actions bot removed the S-stale This issue/PR is stale and will close with no further activity label Dec 1, 2023

rkrasiuk added the A-execution Related to the Execution and EVM label Dec 18, 2023

github-actions bot added the S-stale This issue/PR is stale and will close with no further activity label Jan 9, 2024

github-actions bot removed the S-stale This issue/PR is stale and will close with no further activity label Jan 10, 2024

github-actions bot added S-stale This issue/PR is stale and will close with no further activity and removed S-stale This issue/PR is stale and will close with no further activity labels Mar 9, 2024

github-actions bot added S-stale This issue/PR is stale and will close with no further activity and removed S-stale This issue/PR is stale and will close with no further activity labels Apr 2, 2024

onbjerg closed this Apr 18, 2024

feat(revm): Integrate BundleBuilder #4647

feat(revm): Integrate BundleBuilder #4647

Conversation

alessandromazza98 commented Sep 19, 2023

codecov bot commented Sep 19, 2023 • edited Loading

Codecov Report

gakonst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakita Oct 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessandromazza98 commented Sep 26, 2023

gakonst left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rakita Oct 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessandromazza98 commented Oct 9, 2023

alessandromazza98 commented Oct 9, 2023

rakita left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alessandromazza98 commented Oct 17, 2023

idatsy commented Nov 26, 2023

rakita left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gakonst commented Jan 9, 2024

alessandromazza98 commented Jan 10, 2024

onbjerg commented Apr 18, 2024

codecov bot commented Sep 19, 2023 •

edited

Loading

rakita Oct 11, 2023 •

edited

Loading

gakonst left a comment •

edited

Loading

rakita Oct 8, 2023 •

edited

Loading

rakita left a comment •

edited

Loading