Make access event keys eip 6800 compatible #7594
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,7 +51,6 @@ | |
| import java.util.concurrent.ConcurrentHashMap; | ||
| import javax.annotation.Nonnull; | ||
|
|
||
| import org.apache.tuweni.bytes.Bytes; | ||
| import org.apache.tuweni.bytes.Bytes32; | ||
| import org.apache.tuweni.units.bigints.UInt256; | ||
|
|
@@ -125,8 +124,10 @@ protected Hash internalCalculateRootHash( | |
| accountUpdate -> { | ||
| final Address accountKey = accountUpdate.getKey(); | ||
| // generate account triekeys | ||
| final List<Bytes32> accountKeyIds = new ArrayList<>(); | ||
| if (!accountUpdate.getValue().isUnchanged()) { | ||
| accountKeyIds.add(trieKeyPreloader.generateAccountKeyId()); | ||
| } | ||
|
|
||
| // generate storage triekeys | ||
| final List<Bytes32> storageKeyIds = new ArrayList<>(); | ||
|
|
@@ -153,7 +154,7 @@ protected Hash internalCalculateRootHash( | |
| !codeUpdate.isUnchanged() | ||
| && !(codeIsEmpty(previousCode) && codeIsEmpty(updatedCode)); | ||
| if (isCodeUpdateNeeded) { | ||
| accountKeyIds.add(Parameters.CODE_HASH_LEAF_KEY); | ||
| codeKeyIds.addAll( | ||
| trieKeyPreloader.generateCodeChunkKeyIds( | ||
| updatedCode == null ? previousCode : updatedCode)); | ||
|
|
@@ -166,24 +167,13 @@ protected Hash internalCalculateRootHash( | |
| accountKey, accountKeyIds, storageKeyIds, codeKeyIds)); | ||
| }); | ||
|
|
||
| for (final Address accountKey : worldStateUpdater.getAccountsToUpdate().keySet()) { | ||
| updateState( | ||
| accountKey, | ||
| stateTrie, | ||
| maybeStateUpdater, | ||
| preloadedHashers.get(accountKey), | ||
| worldStateUpdater); | ||
| } | ||
|
|
||
| LOG.info("start commit "); | ||
|
|
@@ -206,124 +196,77 @@ protected Hash internalCalculateRootHash( | |
| return Hash.wrap(rootHash); | ||
| } | ||
|
|
||
| private static boolean codeIsEmpty(final Bytes value) { | ||
| return value == null || value.isEmpty(); | ||
| } | ||
|
|
||
| private void generateAccountValues( | ||
| final Address accountKey, | ||
| final VerkleEntryFactory verkleEntryFactory, | ||
| final Optional<VerkleWorldStateKeyValueStorage.Updater> maybeStateUpdater, | ||
| final DiffBasedValue<VerkleAccount> accountUpdate) { | ||
| if (accountUpdate.isUnchanged()) { | ||
| return; | ||
|
Contributor
There was a problem hiding this comment. we cannot use if, else if , else if ? I think it will be better |
||
| } | ||
| if (accountUpdate.getUpdated() == null) { | ||
| verkleEntryFactory.generateAccountKeysForRemoval(accountKey); | ||
| final Hash addressHash = hashAndSavePreImage(accountKey); | ||
| maybeStateUpdater.ifPresent( | ||
| bonsaiUpdater -> bonsaiUpdater.removeAccountInfoState(addressHash)); | ||
| return; | ||
| } | ||
| final Bytes priorValue = | ||
| accountUpdate.getPrior() == null ? null : accountUpdate.getPrior().serializeAccount(); | ||
| final Bytes accountValue = accountUpdate.getUpdated().serializeAccount(); | ||
| if (!accountValue.equals(priorValue)) { | ||
|
Contributor
There was a problem hiding this comment. do we still need this check if we have now https://github.com/hyperledger/besu/pull/7594/files#diff-a8c0068954d44abe33b85a9a6c2b70309adb959255e417350abe54c858574b62R208 ? |
||
| verkleEntryFactory.generateAccountKeyValueForUpdate( | ||
| accountKey, | ||
| accountUpdate.getUpdated().getNonce(), | ||
| accountUpdate.getUpdated().getBalance()); | ||
| maybeStateUpdater.ifPresent( | ||
| bonsaiUpdater -> | ||
| bonsaiUpdater.putAccountInfoState(hashAndSavePreImage(accountKey), accountValue)); | ||
| } | ||
| } | ||
|
|
||
| private void generateCodeValues( | ||
| final Address accountKey, | ||
| final VerkleEntryFactory verkleEntryFactory, | ||
| final Optional<VerkleWorldStateKeyValueStorage.Updater> maybeStateUpdater, | ||
| final DiffBasedValue<Bytes> codeUpdate) { | ||
| if (codeUpdate == null | ||
| || codeUpdate.isUnchanged() | ||
| || (codeIsEmpty(codeUpdate.getPrior()) && codeIsEmpty(codeUpdate.getUpdated()))) { | ||
| return; | ||
| } | ||
| if (codeUpdate.getUpdated() == null) { | ||
| final Hash priorCodeHash = Hash.hash(codeUpdate.getPrior()); | ||
| verkleEntryFactory.generateCodeKeysForRemoval(accountKey, codeUpdate.getPrior()); | ||
| final Hash accountHash = accountKey.addressHash(); | ||
| maybeStateUpdater.ifPresent( | ||
| bonsaiUpdater -> bonsaiUpdater.removeCode(accountHash, priorCodeHash)); | ||
| return; | ||
| } | ||
| final Hash accountHash = accountKey.addressHash(); | ||
| final Hash codeHash = Hash.hash(codeUpdate.getUpdated()); | ||
| verkleEntryFactory.generateCodeKeyValuesForUpdate( | ||
| accountKey, codeUpdate.getUpdated(), codeHash); | ||
| if (codeUpdate.getUpdated().isEmpty()) { | ||
| maybeStateUpdater.ifPresent(bonsaiUpdater -> bonsaiUpdater.removeCode(accountHash, codeHash)); | ||
| } else { | ||
| maybeStateUpdater.ifPresent( | ||
| bonsaiUpdater -> bonsaiUpdater.putCode(accountHash, codeHash, codeUpdate.getUpdated())); | ||
| } | ||
| } | ||
|
|
||
| private void generateStorageValues( | ||
| final Address accountKey, | ||
| final VerkleEntryFactory verkleEntryFactory, | ||
| final Optional<VerkleWorldStateKeyValueStorage.Updater> maybeStateUpdater, | ||
| final StorageConsumingMap<StorageSlotKey, DiffBasedValue<UInt256>> storageAccountUpdate) { | ||
| if (storageAccountUpdate == null || storageAccountUpdate.keySet().isEmpty()) { | ||
| return; | ||
| } | ||
| final Hash updatedAddressHash = accountKey.addressHash(); | ||
| // for manicured tries and composting, collect branches here (not implemented) | ||
| for (final Map.Entry<StorageSlotKey, DiffBasedValue<UInt256>> storageUpdate : | ||
|
|
@@ -333,30 +276,13 @@ private void updateAccountStorageState( | |
| if (!storageUpdate.getValue().isUnchanged()) { | ||
| final UInt256 updatedStorage = storageUpdate.getValue().getUpdated(); | ||
| if (updatedStorage == null) { | ||
| verkleEntryFactory.generateStorageKeysForRemoval(accountKey, storageUpdate.getKey()); | ||
| maybeStateUpdater.ifPresent( | ||
| diffBasedUpdater -> | ||
| diffBasedUpdater.removeStorageValueBySlotHash(updatedAddressHash, slotHash)); | ||
| } else { | ||
| verkleEntryFactory.generateStorageKeyValueForUpdate( | ||
| accountKey, storageUpdate.getKey(), updatedStorage); | ||
| maybeStateUpdater.ifPresent( | ||
| bonsaiUpdater -> | ||
| bonsaiUpdater.putStorageValueBySlotHash( | ||
|
|
@@ -366,6 +292,69 @@ private void updateAccountStorageState( | |
| } | ||
| } | ||
|
|
||
| private void updateState( | ||
| final Address accountKey, | ||
| final VerkleTrie stateTrie, | ||
| final Optional<VerkleWorldStateKeyValueStorage.Updater> maybeStateUpdater, | ||
| final HasherContext hasherContext, | ||
| final VerkleWorldStateUpdateAccumulator worldStateUpdater) { | ||
|
|
||
| final VerkleEntryFactory verkleEntryFactory = new VerkleEntryFactory(hasherContext.hasher()); | ||
|
|
||
| generateAccountValues( | ||
| accountKey, | ||
| verkleEntryFactory, | ||
| maybeStateUpdater, | ||
| worldStateUpdater.getAccountsToUpdate().get(accountKey)); | ||
|
|
||
| generateCodeValues( | ||
| accountKey, | ||
| verkleEntryFactory, | ||
| maybeStateUpdater, | ||
| worldStateUpdater.getCodeToUpdate().get(accountKey)); | ||
|
|
||
| generateStorageValues( | ||
| accountKey, | ||
| verkleEntryFactory, | ||
| maybeStateUpdater, | ||
| worldStateUpdater.getStorageToUpdate().get(accountKey)); | ||
|
|
||
| verkleEntryFactory | ||
| .getKeysForRemoval() | ||
| .forEach( | ||
| key -> { | ||
| System.out.println("remove key " + key); | ||
| stateTrie.remove(key); | ||
| }); | ||
| verkleEntryFactory | ||
| .getNonStorageKeyValuesForUpdate() | ||
| .forEach( | ||
| (key, value) -> { | ||
| System.out.println("add key " + key + " leaf value " + value); | ||
| stateTrie.put(key, value); | ||
| }); | ||
| verkleEntryFactory | ||
| .getStorageKeyValuesForUpdate() | ||
| .forEach( | ||
| (storageSlotKey, pair) -> { | ||
| var storageAccountUpdate = worldStateUpdater.getStorageToUpdate().get(accountKey); | ||
|
Contributor
There was a problem hiding this comment. 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 ?
Contributor
Author
There was a problem hiding this comment. 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.
Contributor
There was a problem hiding this comment. I was talking about this check (==null) 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
Contributor
Author
There was a problem hiding this comment. 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 (storageAccountUpdate == null) { | ||
| return; | ||
| } | ||
| System.out.println( | ||
| "add storage key " + pair.getFirst() + " value " + pair.getSecond()); | ||
| Optional<DiffBasedValue<UInt256>> storageUpdate = | ||
| Optional.ofNullable(storageAccountUpdate.get(storageSlotKey)); | ||
| stateTrie | ||
| .put(pair.getFirst(), pair.getSecond()) | ||
| .ifPresentOrElse( | ||
| bytes -> | ||
| storageUpdate.ifPresent( | ||
| storage -> storage.setPrior(UInt256.fromBytes(bytes))), | ||
| () -> storageUpdate.ifPresent(storage -> storage.setPrior(null))); | ||
| }); | ||
| } | ||
|
|
||
| @Override | ||
| public MutableWorldState freeze() { | ||
| this.worldStateConfig.setFrozen(true); | ||
|
|
||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
should we remove storageRoot while we are here
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.
good shout. @matkt mentioned he will remove it in future PR so I didn't want to create conflicts unnecessarily