diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/request/StorageRangeDataRequest.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/request/StorageRangeDataRequest.java index 14f92e2c3f9..4c50926303b 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/request/StorageRangeDataRequest.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/request/StorageRangeDataRequest.java @@ -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; @@ -192,12 +191,13 @@ public Stream 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(); } diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/AccountHealingTrackingTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/AccountHealingTrackingTest.java index 70bdbf8b801..8e4e40311f3 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/AccountHealingTrackingTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/sync/snapsync/AccountHealingTrackingTest.java @@ -64,9 +64,7 @@ public class AccountHealingTrackingTest { DataStorageConfiguration.DEFAULT_BONSAI_CONFIG); private WorldStateStorageCoordinator worldStateStorageCoordinator; - private WorldStateProofProvider worldStateProofProvider; - private MerkleTrie accountStateTrie; @Mock SnapWorldDownloadState snapWorldDownloadState; @@ -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())); @@ -108,7 +104,7 @@ void avoidMarkingAccountWhenStorageProofValid() { root -> RangeStorageEntriesCollector.collectEntries( collector, visitor, root, Hash.ZERO)); - // generate the proof + final List proofs = worldStateProofProvider.getStorageProofRelatedNodes( Hash.wrap(storageTrie.getRootHash()), accountHash, Hash.ZERO); @@ -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 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 visitor = RangeStorageEntriesCollector.createVisitor(collector); + final TreeMap slots = + (TreeMap) + 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())); @@ -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())); @@ -174,11 +211,46 @@ void markAccountOnPartialStorageRange() { stateTrieAccountValue.getStorageRoot()); final RangeStorageEntriesCollector collector = - RangeStorageEntriesCollector.createCollector( + RangeStorageEntriesCollector.createCollector(Hash.ZERO, MAX_RANGE, 1, Integer.MAX_VALUE); + final TrieIterator visitor = RangeStorageEntriesCollector.createVisitor(collector); + final TreeMap slots = + (TreeMap) + 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 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 visitor = RangeStorageEntriesCollector.createVisitor(collector); final TreeMap slots = (TreeMap) @@ -186,7 +258,7 @@ void markAccountOnPartialStorageRange() { root -> RangeStorageEntriesCollector.collectEntries( collector, visitor, root, Hash.ZERO)); - // generate the proof + final List proofs = worldStateProofProvider.getStorageProofRelatedNodes( Hash.wrap(storageTrie.getRootHash()), accountHash, Hash.ZERO); @@ -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())); @@ -223,6 +295,7 @@ void avoidMarkingAccountOnValidStorageTrieNodeDetection() { Hash.wrap(accountStateTrie.getRootHash()), Bytes.EMPTY); storageTrieNodeHealingRequest.getExistingData(worldStateStorageCoordinator); + verify(snapWorldDownloadState, never()).addAccountToHealingList(any(Bytes.class)); } }