Snap server GetTrieNodes to return empty bytes when trienode doesn't exist

[jframe]

PR description

Snap server GetTrieNodes request to return empty bytes instead of ending the request when the trienode doesn't exist. This matches the behaviour of Geth and fixes the remaining failing tests with Hive GetTrieNodes.

Also changed to end the request when the account doesn't exist for retrieving storage to match Geth's behaviour and fix a test.

Testing

Snap synced Geth node against Besu as a snap server with this PR

Fixed Issue(s)

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

[matkt]

we already did a modification for that on main https://github.com/hyperledger/besu/blob/main/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java#L519
and https://github.com/hyperledger/besu/blob/main/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java#L546 and we should not return empty but EMPTY_TRIE_NODE

[matkt]

I think only this one is still needed https://github.com/hyperledger/besu/pull/7305/files#diff-e919e276e0dad67624dfc3a5e37c4e7f5369c9b7301f49e32bc6db3b4a062f44R532
maybe @matthew1001 it can impact you

[matthew1001]

maybe @matthew1001 it can impact you

Do you mean it will change the way empty accounts ranges are received by BFT nodes?

[matkt]

maybe @matthew1001 it can impact you

Do you mean it will change the way empty accounts ranges are received by BFT nodes?

I was thinking how we manage the case where we ask for an account that does not exist and server is returning nothing but maybe it's just a normal case and we should never ask for an account that does not exist. maybe it will not impact you.

[jframe]

we already did a modification for that on main https://github.com/hyperledger/besu/blob/main/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java#L519 and https://github.com/hyperledger/besu/blob/main/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java#L546 and we should not return empty but EMPTY_TRIE_NODE

That was a slightly different case specific to when there was no account and an empty location. This is a more general case and applies whenever we have no account or storage trienode.

[jframe]

maybe @matthew1001 it can impact you

Do you mean it will change the way empty accounts ranges are received by BFT nodes?

It could do as I am changing logic around the handling of nodes that aren't found. Could possibly break snapsync for BFT in the empty state case. Currently testing the BFT snapsync with the PR at the moment.

[jframe]

we already did a modification for that on main https://github.com/hyperledger/besu/blob/main/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java#L519 and https://github.com/hyperledger/besu/blob/main/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java#L546 and we should not return empty but EMPTY_TRIE_NODE

Agree that EMPTY_TRIE_NODE makes more sense but the snapsync hive tests are expecting empty https://github.com/ethereum/go-ethereum/blob/master/cmd/devp2p/internal/ethtest/snap.go#L665 so I think this is correct. If I change it to use EMPTY_TRIE_NODE instead of empty it fails the hive tests. I traced through Geth and when a node isn't found nil is returned and included in the response.

[matthew1001]

we already did a modification for that on main https://github.com/hyperledger/besu/blob/main/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java#L519 and https://github.com/hyperledger/besu/blob/main/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java#L546 and we should not return empty but EMPTY_TRIE_NODE

Agree that EMPTY_TRIE_NODE makes more sense but the snapsync hive tests are expecting empty https://github.com/ethereum/go-ethereum/blob/master/cmd/devp2p/internal/ethtest/snap.go#L665 so I think this is correct. If I change it to use EMPTY_TRIE_NODE instead of empty it fails the hive tests. I traced through Geth and when a node isn't found nil is returned and included in the response.

OK, thanks for the clarification. I'm not able to do much this week while at EthCC but I can try some of my tests on Monday. The basic scenario I used as a guide was running 3 full-sync nodes with snap-server enabled, then sync a 4th node with snap-sync (with no accounts on the chain so far) and that showed up the original issues with empty account ranges.

With the change you're suggesting, will AccountRangeDataRequest.addResponse(...) need changing to accommodate an empty response vs an empty trie node?

[jframe]

OK, thanks for the clarification. I'm not able to do much this week while at EthCC but I can try some of my tests on Monday. The basic scenario I used as a guide was running 3 full-sync nodes with snap-server enabled, then sync a 4th node with snap-sync (with no accounts on the chain so far) and that showed up the original issues with empty account ranges.

Thanks I've tested that scenario and broke with my changes and without your changes. So will keep working on this PR till I get that scenario working.

With the change you're suggesting, will AccountRangeDataRequest.addResponse(...) need changing to accommodate an empty response vs an empty trie node?

I think the empty response might need to be handled differently in the TrieNodeHealingRequest since it's the trienode response I changed, but just a guess at the moment tbh.

[jframe]

@matthew1001 I've made changes to handle empty bytes and it's now working with the 4-node BFT test scenario. Could you have a look when you have a chance?

[jframe]

Updated PR to BFT snap changes @garyschulte @matkt

[garyschulte]

🚢

[matthew1001]

@matthew1001 I've made changes to handle empty bytes and it's now working with the 4-node BFT test scenario. Could you have a look when you have a chance?

Sorry I didn't get time to try these changes out yet @jframe but I'm going to be doing a bunch with BFT/snap while working on Bonsai archive so I'll shout if I see any issues.