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

Snapserver responses to return at least one response #7190

Merged
merged 10 commits into from
Jun 26, 2024
Merged

Snapserver responses to return at least one response #7190

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
86ba925
Must return at least one bytecode in GetByteCodes response
jframe Jun 6, 2024
339f622
test for getbytecode returning at least one bytecode
jframe Jun 7, 2024
69907fa
send at least response item for accounts, trienodes and storageranges
jframe Jun 7, 2024
c74a520
Merge branch 'main' into snapserver_at_least_1_resp
jframe Jun 10, 2024
8776fa3
Merge branch 'main' into snapserver_at_least_1_resp
jframe Jun 11, 2024
6fe0c5d
add explanatory comment
jframe Jun 11, 2024
524f9cf
spotless
jframe Jun 11, 2024
8827c0d
Merge remote-tracking branch 'upstream/main' into snapserver_at_least…
jframe Jun 12, 2024
0bf52da
Merge branch 'main' into snapserver_at_least_1_resp
jframe Jun 24, 2024
148cadd
Merge branch 'main' into snapserver_at_least_1_resp
jframe Jun 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 13 additions & 6 deletions ethereum/eth/src/main/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,8 +456,9 @@ MessageData constructGetBytecodesResponse(final MessageData message) {
} else {
Optional<Bytes> optCode = worldStateStorageCoordinator.getCode(Hash.wrap(codeHash), null);
if (optCode.isPresent()) {
if (sumListBytes(codeBytes) + optCode.get().size() > maxResponseBytes
|| stopWatch.getTime() > StatefulPredicate.MAX_MILLIS_PER_REQUEST) {
if (!codeBytes.isEmpty()
Copy link
Contributor

Choose a reason for hiding this comment

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

sumListBytes operates on the stream, with an orElse(0). it shouldn't matter whether the list is empty or not right? what am I missing here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The sumListBytes will work fine on an empty list. This change isn't to guard against calling sumListBytes with an empty list but to ensure we only do the sumListBytes(codeBytes) > maxReponseBytes check once we have at least one record in the response. So that we always send at least once record in the response.

&& (sumListBytes(codeBytes) + optCode.get().size() > maxResponseBytes
|| stopWatch.getTime() > StatefulPredicate.MAX_MILLIS_PER_REQUEST)) {
break;
}
codeBytes.add(optCode.get());
Expand Down Expand Up @@ -511,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());
Expand All @@ -536,7 +538,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());
Expand Down Expand Up @@ -614,11 +618,14 @@ public boolean test(final Pair<Bytes32, Bytes> 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) {
// 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)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this a "pathological" hive test case where the byteLimit is set too low to allow any records through?

👍 This seems a reasonable fix if the spec says there must always be at least one record in the response, but we should add a comment about why we are doing this, because it isn't obvious why we would do this otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There wasn't any "pathological" test for the responses that use this predicate, well none obvious anyway. The "pathological" test was for getByteCodes with a limit too small to allow any response. But since the spec does state we need to return at least one response I made the changes anyway to be compliant.

return true;
} else {
shouldContinue.set(false);
Expand Down
154 changes: 154 additions & 0 deletions ...reum/eth/src/test/java/org/hyperledger/besu/ethereum/eth/manager/snap/SnapServerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,37 @@ public void assertAccountLimitRangeResponse() {
assertThat(assertIsValidAccountRangeProof(Hash.ZERO, rangeData)).isTrue();
}

@Test
public void assertAccountLimitRangeResponse_atLeastOneAccount() {
List<Integer> 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,
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -416,6 +483,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<Bytes> trieNodes = trieNodeRequest.nodes(false);
assertThat(trieNodes).isNotNull();
assertThat(trieNodes.size()).isEqualTo(1);
}

@Test
public void assertStorageTriePathRequest() {
insertTestAccounts(acct1, acct2, acct3, acct4);
Expand Down Expand Up @@ -468,6 +568,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<Bytes> trieNodes = trieNodeRequest.nodes(false);
assertThat(trieNodes).isNotNull();
assertThat(trieNodes.size()).isEqualTo(1);
}

@Test
public void assertCodePresent() {
insertTestAccounts(acct1, acct2, acct3, acct4);
Expand Down Expand Up @@ -506,6 +637,29 @@ public void assertCodeLimitRequest() {
assertThat(codes.codes().size()).isEqualTo(codeLimit * 90 / 100);
}

@Test
public void assertCodeLimitRequest_atLeastOneByteCode() {
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))),
Expand Down
Loading