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.

Sign up for GitHub

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

Keep Worldstate Storage open for Bonsai archive latest layer #5039

Merged
merged 6 commits into from
Feb 4, 2023
Merged

Keep Worldstate Storage open for Bonsai archive latest layer #5039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
faf0180
bonsai layered worldstate subscription
garyschulte Feb 1, 2023
f6a8900
* remove unnecessary worldstate copy in txpool
garyschulte Feb 1, 2023
2b265e9
refactor to use copy and close rather than explicit storage sub/unsub
garyschulte Feb 1, 2023
4780b25
re-add worldstate copy to txpool
garyschulte Feb 2, 2023
3e92f02
add changelog
garyschulte Feb 3, 2023
410e034
Merge branch 'main' into bugfix/latest-layered-worldstate
garyschulte Feb 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ tests are updated to use EC private keys instead of RSA keys.

### Bug Fixes
- Mitigation fix for stale bonsai code storage leading to log rolling issues on contract recreates [#4906](https://github.com/hyperledger/besu/pull/4906)
- Ensure latest cached layered worldstate is subscribed to storage, fix problem with RPC calls using 'latest' [#5039](https://github.com/hyperledger/besu/pull/5039)


## 23.1.0-RC1
Expand Down
7 changes: 6 additions & 1 deletion ...eum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/BonsaiInMemoryWorldState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,12 @@ public void persist(final BlockHeader blockHeader) {
final Hash newWorldStateRootHash = rootHash(localUpdater);
archive
.getTrieLogManager()
.saveTrieLog(archive, localUpdater, newWorldStateRootHash, blockHeader, this);
.saveTrieLog(
archive,
localUpdater,
newWorldStateRootHash,
blockHeader,
(BonsaiPersistedWorldState) this.copy());
Copy link
Contributor

Choose a reason for hiding this comment

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

is this copy can impact the performance ? I see some synchronized method , call to DB . if @ahamlat can confirm that it is safe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, an alternate implementation is to directly subscribe and unsubscribe to the underlying worldstate storage. This is cleaner code-wise, but if there is a performance impact we can directly sub/unsub to the storage itself.

worldStateRootHash = newWorldStateRootHash;
worldStateBlockHash = blockHeader.getBlockHash();
isPersisted = true;
Expand Down
20 changes: 20 additions & 0 deletions ...reum/core/src/main/java/org/hyperledger/besu/ethereum/bonsai/BonsaiLayeredWorldState.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,29 @@ public Optional<BonsaiWorldView> getNextWorldView() {
}

public void setNextWorldView(final Optional<BonsaiWorldView> nextWorldView) {
maybeUnSubscribe();
Copy link
Contributor

Choose a reason for hiding this comment

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

If we close the nextview, how will the layeredworldstate be able to ask it to retrieve the different values? Is it going to recreate it when needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is to close/unsubscribe from the worldstate this layer was initially created with, so we do not leak any snapshots. since this is a mutator, this is the last opportunity we will have to unsubscribe, typically when the BonsaiWorldStateArchive is responding to a BlockAddedEvent, and changes the nextWorldView.

garyschulte marked this conversation as resolved.
Show resolved Hide resolved
this.nextWorldView = nextWorldView;
}

private void maybeUnSubscribe() {
nextWorldView
.filter(WorldState.class::isInstance)
.map(WorldState.class::cast)
.ifPresent(
ws -> {
try {
ws.close();
} catch (final Exception e) {
// no-op
}
});
}

@Override
public void close() throws Exception {
maybeUnSubscribe();
}

public TrieLogLayer getTrieLog() {
return trieLog;
}
Expand Down
6 changes: 5 additions & 1 deletion .../hyperledger/besu/ethereum/eth/transactions/sorter/AbstractPendingTransactionsSorter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,7 +428,11 @@ private TransactionAddedStatus addTransaction(
LOG,
"Transaction {} not added because nonce too far in the future for sender {}",
transaction::toTraceLog,
maybeSenderAccount::toString);
() ->
maybeSenderAccount
.map(Account::getAddress)
.map(Address::toString)
.orElse("unknown"));
return NONCE_TOO_FAR_IN_FUTURE_FOR_SENDER;
}

Expand Down