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

Jump to bottom

Snap server rebase #6640

Merged
merged 16 commits into from Mar 30, 2024
Merged

Snap server rebase #6640

merged 16 commits into from Mar 30, 2024

Conversation

garyschulte
Copy link
Contributor

@garyschulte garyschulte commented Feb 28, 2024 •
edited

PR description

Initial implementation of snap server using streaming flat database. Obseletes #5887

This PR adds snap server functionality behind a feature flag --Xsnapsync-server-enabled which enables besu to serve snap/1 protocol fully.

Snap server has a specific set of storage requirements:

  • --data-storage-format=BONSAI, snap server only works with bonsai storage
  • --Xbonsai-code-using-code-hash-enabled, snap server nodes need to store code by code hash in order to serve snap code requests
  • --Xsnapsync-synchronizer-flat-db-healing-enabled, Snap server nodes must be snap synced with full flat db enabled, (or the node has to have been full-synced to have a fully-flattened db)

Given two of these requrirements (code-by-code-hash and flat-db-healing), most nodes will have to resync in order to serve snap data, with these parameters in the config file:

data-storage-format="BONSAI"
Xsnapsync-server-enabled=true
Xbonsai-code-using-code-hash-enabled=true
Xsnapsync-synchronizer-flat-db-healing-enabled=true

Performance Impact

Testing the initial implementation has shown serving snap data has slight impact to execution performance in normal network conditions, on the order of 10%

Screenshot 2024-03-22 at 5 12 25 PM

Load Test

To get an idea of the cost to execution performance in less than ideal conditions, besu was tested with the load of 2 static syncing peers (besu serving all of the sync load to both clients simulatenously). In this case besu showed roughly 2x the block execution times of besu nodes which were not serving snap data.

Normally besu would only serve some fraction of the load of syncing clients. We could push this further with more static syncing clients, but this was sufficient to get signal about the performance impact.

Once the snap clients completed their worldstate sync, besu performance returned to the mean immediately

2 client sync perf

Read vs write during this time:

Screenshot 2024-03-22 at 5 06 39 PM

Testing Clients

Besu 23.4.x
Geth 13.14
Nethermind 1.25.4

What's Next

  • Optimizations. This is an initial implementation of snap server and now that we are satisfied in its correctness, we will turn to optimizing to minimize its impact on block execution times.

  • A follow-on PR will add a storage subcommand to convert existing databases to fully flattened and code-by-code-hash storage, without the need for a resync.

Fixed Issue(s)

fixes #3165

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

Most advanced CI tests are deferred until PR approval, but you could:

  • locally run all unit tests via: ./gradlew build
  • locally run all acceptance tests via: ./gradlew acceptanceTest
  • locally run all integration tests via: ./gradlew integrationTest
  • locally run all reference tests via: ./gradlew ethereum:referenceTests:referenceTests

@garyschulte garyschulte force-pushed the snap-server-rebase branch 5 times, most recently from 6fb37f2 to 80eb1ac Compare March 16, 2024 00:10
@garyschulte garyschulte marked this pull request as ready for review March 16, 2024 00:27
@garyschulte garyschulte requested review from matkt and jflo March 16, 2024 00:27
@garyschulte garyschulte force-pushed the snap-server-rebase branch 5 times, most recently from b4fc969 to a0ec424 Compare March 16, 2024 03:35
@garyschulte garyschulte force-pushed the snap-server-rebase branch 5 times, most recently from f571c45 to b51f3c6 Compare March 22, 2024 23:20
matkt
matkt reviewed Mar 27, 2024
View reviewed changes
Copy link
Contributor

@matkt matkt left a comment

Choose a reason for hiding this comment

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

Added some questions but in general I like the implementation 💯

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java
worldStateStorageCoordinator.worldStateKeyValueStorage())
.getFlatDbStrategy();
if (!flatDbStrategy.isCodeByCodeHash()) {
LOGGER.warn("SnapServer requires code stored by codehash, but it is not enabled");
Copy link
Contributor

Choose a reason for hiding this comment

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

what we are doing in this case ? not sure to understand if we are stopping the snapserver ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

for now, snap server will operate as normal with just a warning. Essentially we will just serve empty code requests if the storage hasn't migrated. If we think it is necessary we can refuse to start snap server if we are not in code-by-code hash storage mode. I was thinking that we would address this corner case via the storage migration subcommand PR

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java Show resolved Hide resolved
...ices/kvstore/src/main/java/org/hyperledger/besu/services/kvstore/LayeredKeyValueStorage.java Outdated Show resolved Hide resolved
...th/src/main/java/org/hyperledger/besu/ethereum/eth/messages/snap/GetStorageRangeMessage.java Show resolved Hide resolved
...ices/kvstore/src/main/java/org/hyperledger/besu/services/kvstore/LayeredKeyValueStorage.java Show resolved Hide resolved
matkt
matkt reviewed Mar 27, 2024
View reviewed changes
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java
}
}

MessageData constructGetTrieNodesResponse(final MessageData message) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we need to limite the number of trie nodes to serve by request as Geth. Because it seems to be a possible dos vector

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will 👀 what geth is doing here. Just to clarify, what about trie nodes queries makes you think they are disproportionately expensive? I would think range requests would represent more resource consumption, especially considering we are not directly using an iterator but rather collecting in memory to serve the requests.

Copy link
Contributor

@matkt matkt Mar 29, 2024 •
edited

Choose a reason for hiding this comment

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

I ask you 30000 trie nodes that does not exist. You will still have to read in base to find out if the trie node exists. So I can spam you as much as I want. The Range is not the same I ask you for a range and the peers will control what it returns in this range according to the number of leaves. To test. it is an intuition that I have it could be wrong

@garyschulte garyschulte force-pushed the snap-server-rebase branch 3 times, most recently from 82a5de6 to 3560866 Compare March 29, 2024 19:41
macfarla
macfarla approved these changes Mar 29, 2024
View reviewed changes
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

few nits mostly for logging

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java Outdated Show resolved Hide resolved
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java
.map(DefaultSynchronizer.class::cast)
.ifPresentOrElse(
z -> this.listenerId.set(z.subscribeInitialSync(this)),
() -> LOGGER.warn("SnapServer created without reference to sync status"));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: would be nice for logs to be consistent with snap server vs SnapServer

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java Outdated Show resolved Hide resolved
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java

var resp = StorageRangeMessage.create(collectedStorages, proofNodes);
LOGGER.debug(
"returned in {} storage {} to {} range {} to {} with {} storages and {} proofs, resp size {} of max {}",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this log message is duplicated, could be refactored to a method

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am going to skip this one for now since all of the return messages at least subtly different.

ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java Outdated Show resolved Hide resolved
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java Outdated Show resolved Hide resolved
ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java Outdated

/** See https://github.com/ethereum/devp2p/blob/master/caps/snap.md */
@SuppressWarnings("unused")
Copy link
Contributor

Choose a reason for hiding this comment

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

can this annotation be removed now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

apparently yes 👍

@garyschulte garyschulte mentioned this pull request Mar 30, 2024
Open
@garyschulte
squash of snap server commits
0e1b41f 
Signed-off-by: garyschulte <garyschulte@gmail.com>
garyschulte and others added 15 commits March 29, 2024 17:22
@garyschulte
address potentially null storage format that occurrs in some unit tests
73567a2 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
add snapserver experimental flag, refactor snap server start
d3a91aa 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
add a timeout to prevent snap requests from breach of protocol by tak…
0700841 
…ing more than 5 seconds

Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
changelog
261a39e 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
add handling for missing sync config in controller builder for unit t…
0a3a3b0 
…ests

Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
fix initial sync listener registration and startup ordering
f9976b1 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
reuse stopWatch from individual methods in snap predicates and short …
19f2c71 
…circuit

Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
some cleanup and change to snap-server cli param to disable messages …
bd6270e 
…rather than protocol

Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
add predicate to empty account range fallback. add log warning when s…
25d81e0 
…nap request takes too long

Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
use a new WorldStateStorageCoordinator with explicit cached storage f…
237f118 
…or proof generation

Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
default limit hash to 0xFF..FF if not present in the snap range message.
0f74064 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
review feedback
633ac98 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
implement feedback from Karim
aa9cc79 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte @macfarla
Apply suggestions from code review
f66661c 
Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte
additional feedback from Sally
270e2cc 
Signed-off-by: garyschulte <garyschulte@gmail.com>
@garyschulte garyschulte merged commit 34fc5ee into hyperledger:main Mar 30, 2024
42 checks passed
@garyschulte garyschulte deleted the snap-server-rebase branch April 3, 2024 19:24
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
@garyschulte @macfarla @amsmota
Snap server rebase (hyperledger#6640)
fc63186 
* initial snap server implementation

Signed-off-by: garyschulte <garyschulte@gmail.com>
Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: amsmota <antonio.mota@citi.com>
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
@garyschulte @macfarla @amsmota
Snap server rebase (hyperledger#6640)
6ef9e2a 
* initial snap server implementation

Signed-off-by: garyschulte <garyschulte@gmail.com>
Co-authored-by: Sally MacFarlane <macfarla.github@gmail.com>
Signed-off-by: amsmota <antonio.mota@citi.com>
@garyschulte garyschulte mentioned this pull request Apr 22, 2024
Merged
8 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matkt matkt matkt left review comments

@macfarla macfarla macfarla approved these changes

@jflo jflo Awaiting requested review from jflo

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement snapsync as a server
3 participants
@garyschulte @macfarla @matkt