Make access event keys eip 6800 compatible by lu-pinto · Pull Request #7594 · besu-eth/besu

lu-pinto · 2024-09-10T10:21:19Z

PR description

This PR encodes balance, nonce, version and code size into a single leaf in Verkle, while code hash has its own leaf, as mandated by EIP-6800.
I'm not doing fixes to gas costs for EIP-4762 yet, most changes are just to superficially not charge for the same leaf twice and to make code compile. I will implement EIP-4762 gas costs in a subsequent PR.

Fixed Issue(s)

This is needed for fixing gas costs for EIP-4762

Thanks for sending a pull request! Have you done the following?

Checked out our contribution guidelines?
Considered documentation and added the doc-change-required label to this PR if updates are required.
Considered the changelog and included an update if required.
For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Locally, you can run these tests to catch failures early:

unit tests: ./gradlew build
acceptance tests: ./gradlew acceptanceTest
integration tests: ./gradlew integrationTest
reference tests: ./gradlew ethereum:referenceTests:referenceTests

Signed-off-by: Luis Pinto <luis.pinto@consensys.net>

matkt

I like the proposed modification, I just have some remarks and we must not forget to add the equals method in VerkleAccount.

matkt · 2024-09-17T08:04:14Z

-        }
-      }
+    if (accountUpdate.isUnchanged()) {
+      return;


we cannot use if, else if , else if ? I think it will be better

matkt · 2024-09-17T08:08:53Z

-  }
-
-  private void updateAccountStorageState(
+  private void generateStorageValue(


generateStorageValue-S ?

matkt · 2024-09-17T08:09:45Z

-        Address.ZERO); // TODO REMOVE is just to preload the native library for performance check
  }

  public List<Bytes32> generateAccountKeyIds() {


we still need a list for this one ?

Signed-off-by: Luis Pinto <luis.pinto@consensys.net>

minor change and method renames Signed-off-by: Luis Pinto <luis.pinto@consensys.net>

use 0.0.3-SNAPSHOT with timestamp Signed-off-by: Luis Pinto <luis.pinto@consensys.net>

garyschulte · 2024-09-19T00:03:43Z

  private Hash storageRoot; // TODO REMOVE AS USELESS
+  private int hashCode;


should we remove storageRoot while we are here

good shout. @matkt mentioned he will remove it in future PR so I didn't want to create conflicts unnecessarily

matkt

Added comment but approve to unblock

matkt · 2024-09-20T07:26:31Z

+        .getStorageKeyValuesForUpdate()
+        .forEach(
+            (storageSlotKey, pair) -> {
+              var storageAccountUpdate = worldStateUpdater.getStorageToUpdate().get(accountKey);


I don't think this one is still needed as we already checked here https://github.com/hyperledger/besu/pull/7594/files#diff-a8c0068954d44abe33b85a9a6c2b70309adb959255e417350abe54c858574b62R267 but maybe I missing something ?

Not sure I understand. So the link you posted is when the key values are generated. Here we commit them to the trie in one go.
On generation we only have the storageSlotKey and corresponding key/value. We don't have the storageUpdate ref where we should save the prior value

I was talking about this check (==null)
https://github.com/hyperledger/besu/pull/7594/files/2d12b659a7478f155f9a7be52e1dfed6dfcb3c7d#diff-a8c0068954d44abe33b85a9a6c2b70309adb959255e417350abe54c858574b62R341

when we are generating the key we already check if the collection is empty, so imo when we enter this loop we cannot have a null

yes you're right that we if there are elements we can't have null. I added it based on a defensive approach since we don't know how the programmer will generate keys and values. What if it's based on another object?
If someone changes the code above, then we will not break here. I would prefer to leave it

matkt · 2024-09-20T07:29:31Z

+    final Bytes priorValue =
+        accountUpdate.getPrior() == null ? null : accountUpdate.getPrior().serializeAccount();
+    final Bytes accountValue = accountUpdate.getUpdated().serializeAccount();
+    if (!accountValue.equals(priorValue)) {


do we still need this check if we have now https://github.com/hyperledger/besu/pull/7594/files#diff-a8c0068954d44abe33b85a9a6c2b70309adb959255e417350abe54c858574b62R208 ?

lu-pinto · 2024-09-20T16:30:45Z

I noticed that adding VerkleAccount.equals/hashcode changes the state root in LogRollingTests. I'm going to remove that commit for now and should be investigated next week

Signed-off-by: Karim Taam <karim.t2am@gmail.com>

Signed-off-by: Luis Pinto <luis.pinto@consensys.net>

ahamlat · 2024-09-23T15:44:50Z

+        new HashSet<>(accumulator.getAccountsToUpdate().keySet()); // accountsToUpdate
+    mergedAddresses.addAll(accumulator.getCodeToUpdate().keySet()); // codeToUpdate
+    mergedAddresses.addAll(accumulator.getStorageToClear()); // storageToClear
+    mergedAddresses.addAll(accumulator.getStorageToUpdate().keySet()); // storageToUpdate


We need to check the performances of these merges. Depending on the number of addresses, it can have a non negligeable overhead, as it is on the critical path of calculating the root hash

it's something similar we are doing during // but clearly we will have to check the performance impact of this when will do benchmark for verkle

matkt

LGTM

lu-pinto changed the base branch from main to verkle September 10, 2024 10:23

lu-pinto force-pushed the make-access-event-keys-eip-6800-compatible branch from a12ee48 to 7a26729 Compare September 10, 2024 10:36

garyschulte mentioned this pull request Sep 11, 2024

Add Luis Pinto as a maintainer #7603

Merged

lu-pinto requested review from garyschulte and matkt September 11, 2024 10:41

lu-pinto added 2 commits September 16, 2024 11:09

Prepare for EIP-4762: make access event keys in line with eip-6800

dd4ea70

Signed-off-by: Luis Pinto <luis.pinto@consensys.net>

refactor VerkleWorldState::internalCalculateRootHash

8558033

Signed-off-by: Luis Pinto <luis.pinto@consensys.net>

lu-pinto mentioned this pull request Sep 16, 2024

Add SuffixTreeEncoder for encoding BasicDataLeaf values besu-eth/besu-stateless#68

Merged

Move SuffixTrieEncoder to besu-verkle-trie project

755f58c

Signed-off-by: Luis Pinto <luis.pinto@consensys.net>

lu-pinto force-pushed the make-access-event-keys-eip-6800-compatible branch from 7a26729 to 755f58c Compare September 16, 2024 15:38

matkt reviewed Sep 17, 2024

View reviewed changes

add missing VerkleAccount equals and hashcode

764ce3a

Signed-off-by: Luis Pinto <luis.pinto@consensys.net>

lu-pinto force-pushed the make-access-event-keys-eip-6800-compatible branch from 657a18e to 764ce3a Compare September 17, 2024 14:57

lu-pinto added 2 commits September 17, 2024 16:17

fixup! refactor VerkleWorldState::internalCalculateRootHash

b4983e6

minor change and method renames Signed-off-by: Luis Pinto <luis.pinto@consensys.net>

fixup! Move SuffixTrieEncoder to besu-verkle-trie project

2d12b65

use 0.0.3-SNAPSHOT with timestamp Signed-off-by: Luis Pinto <luis.pinto@consensys.net>

garyschulte reviewed Sep 19, 2024

View reviewed changes

matkt approved these changes Sep 20, 2024

View reviewed changes

lu-pinto force-pushed the make-access-event-keys-eip-6800-compatible branch from 2d12b65 to 1a07247 Compare September 20, 2024 22:05

matkt and others added 2 commits September 23, 2024 10:27

fix persist when unchanged account

2e3ba71

Signed-off-by: Karim Taam <karim.t2am@gmail.com>

Remove redundant update != prior account check

e8ed0e7

Signed-off-by: Luis Pinto <luis.pinto@consensys.net>

lu-pinto force-pushed the make-access-event-keys-eip-6800-compatible branch from afa6cce to e8ed0e7 Compare September 23, 2024 15:01

ahamlat reviewed Sep 23, 2024

View reviewed changes

matkt approved these changes Sep 24, 2024

View reviewed changes

matkt merged commit d64829e into besu-eth:verkle Sep 24, 2024

matkt added the stateless label Dec 20, 2024

		private Hash storageRoot; // TODO REMOVE AS USELESS
		private int hashCode;

Conversation

lu-pinto commented Sep 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR description

Fixed Issue(s)

Thanks for sending a pull request! Have you done the following?

Locally, you can run these tests to catch failures early:

Uh oh!

matkt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matkt Sep 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matkt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lu-pinto commented Sep 20, 2024

Uh oh!

ahamlat Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matkt Sep 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

matkt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

lu-pinto commented Sep 10, 2024 •

edited

Loading

matkt Sep 17, 2024 •

edited

Loading

ahamlat Sep 23, 2024 •

edited

Loading

matkt Sep 23, 2024 •

edited

Loading