Skip to content

Commit

Permalink
Fix storage range issue during snapsync (#7624)
Browse files Browse the repository at this point in the history 
Signed-off-by: Karim Taam <[email protected]>
Co-authored-by: Simon Dudley <[email protected]>
  • Loading branch information
@matkt @siladu
matkt and siladu authored Oct 2, 2024
1 parent 49bf37c commit 9310e10
Show file tree
Hide file tree
Showing 2 changed files with 94 additions and 21 deletions.
10 changes: 5 additions & 5 deletions ...java/org/hyperledger/besu/ethereum/eth/sync/snapsync/request/StorageRangeDataRequest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@

import static org.hyperledger.besu.ethereum.eth.sync.snapsync.RequestType.STORAGE_RANGE;
import static org.hyperledger.besu.ethereum.eth.sync.snapsync.StackTrie.FlatDatabaseUpdater.noop;
import static org.hyperledger.besu.ethereum.trie.RangeManager.MAX_RANGE;
import static org.hyperledger.besu.ethereum.trie.RangeManager.MIN_RANGE;
import static org.hyperledger.besu.ethereum.trie.RangeManager.findNewBeginElementInRange;
import static org.hyperledger.besu.ethereum.trie.RangeManager.getRangeCount;
Expand Down Expand Up @@ -192,12 +191,13 @@ public Stream<SnapDataRequest> getChildRequests(
getRootHash(), accountHash, storageRoot, key, value);
childRequests.add(storageRangeDataRequest);
});
if (startKeyHash.equals(MIN_RANGE) && endKeyHash.equals(MAX_RANGE)) {
// need to heal this account storage
downloadState.addAccountToHealingList(CompactEncoding.bytesToPath(accountHash));
}
});

if (startKeyHash.equals(MIN_RANGE) && !taskElement.proofs().isEmpty()) {
// need to heal this account storage
downloadState.addAccountToHealingList(CompactEncoding.bytesToPath(accountHash));
}

return childRequests.stream();
}

Expand Down
105 changes: 89 additions & 16 deletions ...test/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/AccountHealingTrackingTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -64,9 +64,7 @@ public class AccountHealingTrackingTest {
DataStorageConfiguration.DEFAULT_BONSAI_CONFIG);

private WorldStateStorageCoordinator worldStateStorageCoordinator;

private WorldStateProofProvider worldStateProofProvider;

private MerkleTrie<Bytes, Bytes> accountStateTrie;

@Mock SnapWorldDownloadState snapWorldDownloadState;
Expand All @@ -82,9 +80,7 @@ public void setup() {
}

@Test
void avoidMarkingAccountWhenStorageProofValid() {

// generate valid proof
void shouldMarkAccountForHealingWhenStorageProofIsReceived() {
final Hash accountHash = Hash.hash(accounts.get(0));
final StateTrieAccountValue stateTrieAccountValue =
StateTrieAccountValue.readFrom(RLP.input(accountStateTrie.get(accountHash).orElseThrow()));
Expand All @@ -108,7 +104,7 @@ void avoidMarkingAccountWhenStorageProofValid() {
root ->
RangeStorageEntriesCollector.collectEntries(
collector, visitor, root, Hash.ZERO));
// generate the proof

final List<Bytes> proofs =
worldStateProofProvider.getStorageProofRelatedNodes(
Hash.wrap(storageTrie.getRootHash()), accountHash, Hash.ZERO);
Expand All @@ -127,11 +123,53 @@ void avoidMarkingAccountWhenStorageProofValid() {
snapWorldDownloadState, worldStateProofProvider, slots, new ArrayDeque<>(proofs));
storageRangeDataRequest.getChildRequests(
snapWorldDownloadState, worldStateStorageCoordinator, null);

verify(snapWorldDownloadState).addAccountToHealingList(any(Bytes.class));
}

@Test
void shouldNotMarkAccountForHealingWhenAllStorageIsReceivedWithoutProof() {
final Hash accountHash = Hash.hash(accounts.get(0));
final StateTrieAccountValue stateTrieAccountValue =
StateTrieAccountValue.readFrom(RLP.input(accountStateTrie.get(accountHash).orElseThrow()));

final StoredMerklePatriciaTrie<Bytes, Bytes> storageTrie =
new StoredMerklePatriciaTrie<>(
new StoredNodeFactory<>(
(location, hash) ->
worldStateKeyValueStorage.getAccountStorageTrieNode(
accountHash, location, hash),
Function.identity(),
Function.identity()),
stateTrieAccountValue.getStorageRoot());

final RangeStorageEntriesCollector collector =
RangeStorageEntriesCollector.createCollector(Hash.ZERO, MAX_RANGE, 10, Integer.MAX_VALUE);
final TrieIterator<Bytes> visitor = RangeStorageEntriesCollector.createVisitor(collector);
final TreeMap<Bytes32, Bytes> slots =
(TreeMap<Bytes32, Bytes>)
storageTrie.entriesFrom(
root ->
RangeStorageEntriesCollector.collectEntries(
collector, visitor, root, Hash.ZERO));

final StorageRangeDataRequest storageRangeDataRequest =
SnapDataRequest.createStorageRangeDataRequest(
Hash.wrap(accountStateTrie.getRootHash()),
accountHash,
storageTrie.getRootHash(),
Hash.ZERO,
MAX_RANGE);
storageRangeDataRequest.addResponse(
snapWorldDownloadState, worldStateProofProvider, slots, new ArrayDeque<>());
storageRangeDataRequest.getChildRequests(
snapWorldDownloadState, worldStateStorageCoordinator, null);

verify(snapWorldDownloadState, never()).addAccountToHealingList(any(Bytes.class));
}

@Test
void markAccountOnInvalidStorageProof() {
void shouldMarkAccountForHealingOnInvalidStorageProof() {
final Hash accountHash = Hash.hash(accounts.get(0));
final StateTrieAccountValue stateTrieAccountValue =
StateTrieAccountValue.readFrom(RLP.input(accountStateTrie.get(accountHash).orElseThrow()));
Expand All @@ -157,8 +195,7 @@ void markAccountOnInvalidStorageProof() {
}

@Test
void markAccountOnPartialStorageRange() {
// generate valid proof
void shouldMarkAccountForHealingOnInvalidStorageWithoutProof() {
final Hash accountHash = Hash.hash(accounts.get(0));
final StateTrieAccountValue stateTrieAccountValue =
StateTrieAccountValue.readFrom(RLP.input(accountStateTrie.get(accountHash).orElseThrow()));
Expand All @@ -174,19 +211,54 @@ void markAccountOnPartialStorageRange() {
stateTrieAccountValue.getStorageRoot());

final RangeStorageEntriesCollector collector =
RangeStorageEntriesCollector.createCollector(
RangeStorageEntriesCollector.createCollector(Hash.ZERO, MAX_RANGE, 1, Integer.MAX_VALUE);
final TrieIterator<Bytes> visitor = RangeStorageEntriesCollector.createVisitor(collector);
final TreeMap<Bytes32, Bytes> slots =
(TreeMap<Bytes32, Bytes>)
storageTrie.entriesFrom(
root ->
RangeStorageEntriesCollector.collectEntries(
collector, visitor, root, Hash.ZERO));

final StorageRangeDataRequest storageRangeDataRequest =
SnapDataRequest.createStorageRangeDataRequest(
Hash.wrap(accountStateTrie.getRootHash()),
accountHash,
storageTrie.getRootHash(),
Hash.ZERO,
MAX_RANGE,
1,
Integer.MAX_VALUE); // limit to 1 in order to have a partial range
MAX_RANGE);
storageRangeDataRequest.addResponse(
snapWorldDownloadState, worldStateProofProvider, slots, new ArrayDeque<>());

verify(snapWorldDownloadState).addAccountToHealingList(any(Bytes.class));
}

@Test
void shouldMarkAccountForHealingOnPartialStorageRange() {
final Hash accountHash = Hash.hash(accounts.get(0));
final StateTrieAccountValue stateTrieAccountValue =
StateTrieAccountValue.readFrom(RLP.input(accountStateTrie.get(accountHash).orElseThrow()));

final StoredMerklePatriciaTrie<Bytes, Bytes> storageTrie =
new StoredMerklePatriciaTrie<>(
new StoredNodeFactory<>(
(location, hash) ->
worldStateKeyValueStorage.getAccountStorageTrieNode(
accountHash, location, hash),
Function.identity(),
Function.identity()),
stateTrieAccountValue.getStorageRoot());

final RangeStorageEntriesCollector collector =
RangeStorageEntriesCollector.createCollector(Hash.ZERO, MAX_RANGE, 1, Integer.MAX_VALUE);
final TrieIterator<Bytes> visitor = RangeStorageEntriesCollector.createVisitor(collector);
final TreeMap<Bytes32, Bytes> slots =
(TreeMap<Bytes32, Bytes>)
storageTrie.entriesFrom(
root ->
RangeStorageEntriesCollector.collectEntries(
collector, visitor, root, Hash.ZERO));
// generate the proof

final List<Bytes> proofs =
worldStateProofProvider.getStorageProofRelatedNodes(
Hash.wrap(storageTrie.getRootHash()), accountHash, Hash.ZERO);
Expand All @@ -205,14 +277,14 @@ void markAccountOnPartialStorageRange() {
snapWorldDownloadState, worldStateProofProvider, slots, new ArrayDeque<>(proofs));
verify(snapWorldDownloadState, never()).addAccountToHealingList(any(Bytes.class));

// should mark during the getchild request
storageRangeDataRequest.getChildRequests(
snapWorldDownloadState, worldStateStorageCoordinator, null);

verify(snapWorldDownloadState).addAccountToHealingList(any(Bytes.class));
}

@Test
void avoidMarkingAccountOnValidStorageTrieNodeDetection() {
void shouldNotMarkAccountForHealingOnValidStorageTrieNodeDetection() {
final Hash accountHash = Hash.hash(accounts.get(0));
final StateTrieAccountValue stateTrieAccountValue =
StateTrieAccountValue.readFrom(RLP.input(accountStateTrie.get(accountHash).orElseThrow()));
Expand All @@ -223,6 +295,7 @@ void avoidMarkingAccountOnValidStorageTrieNodeDetection() {
Hash.wrap(accountStateTrie.getRootHash()),
Bytes.EMPTY);
storageTrieNodeHealingRequest.getExistingData(worldStateStorageCoordinator);

verify(snapWorldDownloadState, never()).addAccountToHealingList(any(Bytes.class));
}
}

0 comments on commit 9310e10

Please sign in to comment.