From 86ba9254d24d1c38539d48bbb51bfed25fe309bb Mon Sep 17 00:00:00 2001 From: Jason Frame Date: Thu, 6 Jun 2024 16:02:13 +1000 Subject: [PATCH 1/5] Must return at least one bytecode in GetByteCodes response Signed-off-by: Jason Frame --- .../besu/ethereum/eth/manager/snap/SnapServer.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java index c2fa186489c..447207a49f7 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java @@ -456,8 +456,9 @@ MessageData constructGetBytecodesResponse(final MessageData message) { } else { Optional optCode = worldStateStorageCoordinator.getCode(Hash.wrap(codeHash), null); if (optCode.isPresent()) { - if (sumListBytes(codeBytes) + optCode.get().size() > maxResponseBytes - || stopWatch.getTime() > StatefulPredicate.MAX_MILLIS_PER_REQUEST) { + var sizeTooLarge = sumListBytes(codeBytes) + optCode.get().size() > maxResponseBytes; + var timeTooLong = stopWatch.getTime() > StatefulPredicate.MAX_MILLIS_PER_REQUEST; + if (!codeBytes.isEmpty() && (sizeTooLarge || timeTooLong)) { break; } codeBytes.add(optCode.get()); From 339f6225ec8a50fded50676d9539d25dca7690b8 Mon Sep 17 00:00:00 2001 From: Jason Frame Date: Fri, 7 Jun 2024 11:24:05 +1000 Subject: [PATCH 2/5] test for getbytecode returning at least one bytecode Signed-off-by: Jason Frame --- .../eth/manager/snap/SnapServerTest.java | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServerTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServerTest.java index c15056d66cc..9b674e32fba 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServerTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServerTest.java @@ -494,6 +494,29 @@ public void assertCodeLimitRequest() { assertThat(codes.codes().size()).isEqualTo(codeLimit * 90 / 100); } + @Test + public void assertCodeMustReturnAtLeastOneByteCode() { + insertTestAccounts(acct1, acct2, acct3, acct4); + + final BytesValueRLPOutput tmp = new BytesValueRLPOutput(); + tmp.startList(); + tmp.writeBigIntegerScalar(BigInteger.ONE); + tmp.writeList( + List.of(acct3.accountValue.getCodeHash(), acct4.accountValue.getCodeHash()), + (hash, rlpOutput) -> rlpOutput.writeBytes(hash)); + tmp.writeBigIntegerScalar(BigInteger.ZERO); + tmp.endList(); + + var codeRequest = + (ByteCodesMessage) + snapServer.constructGetBytecodesResponse(new GetByteCodesMessage(tmp.encoded())); + + assertThat(codeRequest).isNotNull(); + ByteCodesMessage.ByteCodes codes = codeRequest.bytecodes(false); + assertThat(codes).isNotNull(); + assertThat(codes.codes().size()).isEqualTo(1); + } + static SnapTestAccount createTestAccount(final String hexAddr) { return new SnapTestAccount( Hash.wrap(Bytes32.rightPad(Bytes.fromHexString(hexAddr))), From 69907fa83a766fc2332f1bc4410931e649eb4724 Mon Sep 17 00:00:00 2001 From: Jason Frame Date: Fri, 7 Jun 2024 15:49:45 +1000 Subject: [PATCH 3/5] send at least response item for accounts, trienodes and storageranges Signed-off-by: Jason Frame --- .../ethereum/eth/manager/snap/SnapServer.java | 18 ++- .../eth/manager/snap/SnapServerTest.java | 133 +++++++++++++++++- 2 files changed, 143 insertions(+), 8 deletions(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java index 447207a49f7..acf229c1f51 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java @@ -456,9 +456,9 @@ MessageData constructGetBytecodesResponse(final MessageData message) { } else { Optional optCode = worldStateStorageCoordinator.getCode(Hash.wrap(codeHash), null); if (optCode.isPresent()) { - var sizeTooLarge = sumListBytes(codeBytes) + optCode.get().size() > maxResponseBytes; - var timeTooLong = stopWatch.getTime() > StatefulPredicate.MAX_MILLIS_PER_REQUEST; - if (!codeBytes.isEmpty() && (sizeTooLarge || timeTooLong)) { + if (!codeBytes.isEmpty() + && (sumListBytes(codeBytes) + optCode.get().size() > maxResponseBytes + || stopWatch.getTime() > StatefulPredicate.MAX_MILLIS_PER_REQUEST)) { break; } codeBytes.add(optCode.get()); @@ -512,8 +512,9 @@ MessageData constructGetTrieNodesResponse(final MessageData message) { var optStorage = storage.getTrieNodeUnsafe(CompactEncoding.decode(triePath.get(0))); if (optStorage.isPresent()) { - if (sumListBytes(trieNodes) + optStorage.get().size() > maxResponseBytes - || stopWatch.getTime() > StatefulPredicate.MAX_MILLIS_PER_REQUEST) { + if (!trieNodes.isEmpty() + && (sumListBytes(trieNodes) + optStorage.get().size() > maxResponseBytes + || stopWatch.getTime() > StatefulPredicate.MAX_MILLIS_PER_REQUEST)) { break; } trieNodes.add(optStorage.get()); @@ -531,7 +532,9 @@ MessageData constructGetTrieNodesResponse(final MessageData message) { storage.getTrieNodeUnsafe( Bytes.concatenate(accountPrefix, CompactEncoding.decode(path))); if (optStorage.isPresent()) { - if (sumListBytes(trieNodes) + optStorage.get().size() > maxResponseBytes) { + if (!trieNodes.isEmpty() + && sumListBytes(trieNodes) + optStorage.get().size() + > maxResponseBytes) { break; } trieNodes.add(optStorage.get()); @@ -609,11 +612,12 @@ public boolean test(final Pair pair) { return false; } + var hasNoRecords = recordLimit.get() == 0; var underRecordLimit = recordLimit.addAndGet(1) <= MAX_ENTRIES_PER_REQUEST; var underByteLimit = byteLimit.accumulateAndGet(0, (cur, __) -> cur + encodingSizeAccumulator.apply(pair)) < maxResponseBytesFudgeFactor; - if (underRecordLimit && underByteLimit) { + if (hasNoRecords || (underRecordLimit && underByteLimit)) { return true; } else { shouldContinue.set(false); diff --git a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServerTest.java b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServerTest.java index 9b674e32fba..7950b2b8aff 100644 --- a/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServerTest.java +++ b/ethereum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServerTest.java @@ -221,6 +221,37 @@ public void assertAccountLimitRangeResponse() { assertThat(assertIsValidAccountRangeProof(Hash.ZERO, rangeData)).isTrue(); } + @Test + public void assertAccountLimitRangeResponse_atLeastOneAccount() { + List randomLoad = IntStream.range(1, 4096).boxed().collect(Collectors.toList()); + Collections.shuffle(randomLoad); + randomLoad.stream() + .forEach( + i -> + insertTestAccounts( + createTestAccount( + Bytes.concatenate( + Bytes.fromHexString("0x40"), + Bytes.fromHexStringLenient(Integer.toHexString(i * 256))) + .toHexString()))); + + final BytesValueRLPOutput tmp = new BytesValueRLPOutput(); + tmp.startList(); + tmp.writeBytes(storageTrie.getRootHash()); + tmp.writeBytes(Hash.ZERO); + tmp.writeBytes(HASH_LAST); + tmp.writeBigIntegerScalar(BigInteger.ZERO); + tmp.endList(); + var tinyRangeLimit = new GetAccountRangeMessage(tmp.encoded()).wrapMessageData(BigInteger.ZERO); + + var rangeData = + getAndVerifyAccountRangeData( + (AccountRangeMessage) snapServer.constructGetAccountRangeResponse(tinyRangeLimit), 1); + + // assert proofs are valid for the requested range + assertThat(assertIsValidAccountRangeProof(Hash.ZERO, rangeData)).isTrue(); + } + @Test public void assertLastEmptyRange() { // When our final range request is empty, no next account is possible, @@ -353,6 +384,42 @@ public void assertStorageLimitRangeResponse() { .isTrue(); } + @Test + public void assertStorageLimitRangeResponse_atLeastOneSlot() { + insertTestAccounts(acct1, acct2, acct3, acct4); + + final BytesValueRLPOutput tmp = new BytesValueRLPOutput(); + tmp.startList(); + tmp.writeBigIntegerScalar(BigInteger.ONE); + tmp.writeBytes(storageTrie.getRootHash()); + tmp.writeList( + List.of(acct3.addressHash, acct4.addressHash), + (hash, rlpOutput) -> rlpOutput.writeBytes(hash)); + tmp.writeBytes(Hash.ZERO); + tmp.writeBytes(HASH_LAST); + tmp.writeBigIntegerScalar(BigInteger.ZERO); + tmp.endList(); + var tinyRangeLimit = new GetStorageRangeMessage(tmp.encoded()); + + var rangeData = + (StorageRangeMessage) snapServer.constructGetStorageRangeResponse(tinyRangeLimit); + + // assert proofs are valid for the requested range + assertThat(rangeData).isNotNull(); + var slotsData = rangeData.slotsData(false); + assertThat(slotsData).isNotNull(); + assertThat(slotsData.slots()).isNotNull(); + assertThat(slotsData.slots().size()).isEqualTo(1); + var firstAccountStorages = slotsData.slots().first(); + // expecting to see complete 10 slot storage for acct3 + assertThat(firstAccountStorages.size()).isEqualTo(1); + assertThat(slotsData.proofs().size()).isNotEqualTo(0); + + assertThat( + assertIsValidStorageProof(acct4, Hash.ZERO, firstAccountStorages, slotsData.proofs())) + .isTrue(); + } + @Test public void assertAccountTriePathRequest() { insertTestAccounts(acct1, acct2, acct3, acct4); @@ -404,6 +471,39 @@ public void assertAccountTrieLimitRequest() { assertThat(trieNodes.size()).isEqualTo(accountNodeLimit * 90 / 100); } + @Test + public void assertAccountTrieLimitRequest_atLeastOneTrieNode() { + insertTestAccounts(acct1, acct2, acct3, acct4); + + var partialPathToAcct1 = Bytes.fromHexString("0x01"); // first nibble is 1 + var partialPathToAcct2 = CompactEncoding.bytesToPath(acct2.addressHash).slice(0, 1); + var partialPathToAcct3 = Bytes.fromHexString("0x03"); // first nibble is 1 + var partialPathToAcct4 = Bytes.fromHexString("0x04"); // first nibble is 1 + final BytesValueRLPOutput tmp = new BytesValueRLPOutput(); + tmp.startList(); + tmp.writeBigIntegerScalar(BigInteger.ONE); + tmp.writeBytes(storageTrie.getRootHash()); + tmp.writeList( + List.of( + List.of(partialPathToAcct4), + List.of(partialPathToAcct3), + List.of(partialPathToAcct2), + List.of(partialPathToAcct1)), + (path, rlpOutput) -> + rlpOutput.writeList(path, (b, subRlpOutput) -> subRlpOutput.writeBytes(b))); + tmp.writeBigIntegerScalar(BigInteger.ZERO); + tmp.endList(); + + var trieNodeRequest = + (TrieNodesMessage) + snapServer.constructGetTrieNodesResponse(new GetTrieNodesMessage(tmp.encoded())); + + assertThat(trieNodeRequest).isNotNull(); + List trieNodes = trieNodeRequest.nodes(false); + assertThat(trieNodes).isNotNull(); + assertThat(trieNodes.size()).isEqualTo(1); + } + @Test public void assertStorageTriePathRequest() { insertTestAccounts(acct1, acct2, acct3, acct4); @@ -456,6 +556,37 @@ public void assertStorageTrieLimitRequest() { assertThat(trieNodes.size()).isEqualTo(trieNodeLimit * 90 / 100); } + @Test + public void assertStorageTrieLimitRequest_atLeastOneTrieNode() { + insertTestAccounts(acct1, acct2, acct3, acct4); + + var pathToSlot11 = CompactEncoding.encode(Bytes.fromHexStringLenient("0x0101")); + var pathToSlot12 = CompactEncoding.encode(Bytes.fromHexStringLenient("0x0102")); + var pathToSlot1a = CompactEncoding.encode(Bytes.fromHexStringLenient("0x010A")); // not present + + final BytesValueRLPOutput tmp = new BytesValueRLPOutput(); + tmp.startList(); + tmp.writeBigIntegerScalar(BigInteger.ONE); + tmp.writeBytes(storageTrie.getRootHash()); + tmp.writeList( + List.of( + List.of(acct3.addressHash, pathToSlot11, pathToSlot12, pathToSlot1a), + List.of(acct4.addressHash, pathToSlot11, pathToSlot12, pathToSlot1a)), + (path, rlpOutput) -> + rlpOutput.writeList(path, (b, subRlpOutput) -> subRlpOutput.writeBytes(b))); + tmp.writeBigIntegerScalar(BigInteger.ZERO); + tmp.endList(); + + var trieNodeRequest = + (TrieNodesMessage) + snapServer.constructGetTrieNodesResponse(new GetTrieNodesMessage(tmp.encoded())); + + assertThat(trieNodeRequest).isNotNull(); + List trieNodes = trieNodeRequest.nodes(false); + assertThat(trieNodes).isNotNull(); + assertThat(trieNodes.size()).isEqualTo(1); + } + @Test public void assertCodePresent() { insertTestAccounts(acct1, acct2, acct3, acct4); @@ -495,7 +626,7 @@ public void assertCodeLimitRequest() { } @Test - public void assertCodeMustReturnAtLeastOneByteCode() { + public void assertCodeLimitRequest_atLeastOneByteCode() { insertTestAccounts(acct1, acct2, acct3, acct4); final BytesValueRLPOutput tmp = new BytesValueRLPOutput(); From 6fe0c5dd16b8b625c6c087a3729057562cbf49fc Mon Sep 17 00:00:00 2001 From: Jason Frame Date: Tue, 11 Jun 2024 14:10:08 +1000 Subject: [PATCH 4/5] add explanatory comment Signed-off-by: Jason Frame --- .../hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java | 1 + 1 file changed, 1 insertion(+) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java index acf229c1f51..b5f1982c5b7 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java @@ -617,6 +617,7 @@ public boolean test(final Pair pair) { var underByteLimit = byteLimit.accumulateAndGet(0, (cur, __) -> cur + encodingSizeAccumulator.apply(pair)) < maxResponseBytesFudgeFactor; + // Only enforce limits when we have at least 1 record as the spec requires we return at least 1 record if (hasNoRecords || (underRecordLimit && underByteLimit)) { return true; } else { From 524f9cfd2d5d2f191b9cc9a08f627e499ffea2d7 Mon Sep 17 00:00:00 2001 From: Jason Frame Date: Tue, 11 Jun 2024 14:31:00 +1000 Subject: [PATCH 5/5] spotless Signed-off-by: Jason Frame --- .../hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java index b5f1982c5b7..adac7aa944f 100644 --- a/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java +++ b/ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java @@ -617,7 +617,8 @@ public boolean test(final Pair pair) { var underByteLimit = byteLimit.accumulateAndGet(0, (cur, __) -> cur + encodingSizeAccumulator.apply(pair)) < maxResponseBytesFudgeFactor; - // Only enforce limits when we have at least 1 record as the spec requires we return at least 1 record + // Only enforce limits when we have at least 1 record as the snapsync spec + // requires at least 1 record must be returned if (hasNoRecords || (underRecordLimit && underByteLimit)) { return true; } else {