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.

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

eth/protocols/snap: fix snap sync failure on empty storage range #28306

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

rjl493456442
Copy link
Member

@rjl493456442 rjl493456442 commented Oct 10, 2023

This pull request addresses an issue in snap sync, specifically when the entire sync process can be halted due to an encountered empty storage range.

Currently, on the snap sync client side, the response to an empty storage range is discarded. However, this response is valid when there are no available storage slots within that range.

For instance, consider a large contract where the entire key space is divided into 16 chunks, and there are no available slots in the last chunk [0xf] -> [end]. When the node receives a request for this particular range, the response includes:

  • The proof with origin [0xf]
  • A nil storage slot set

If we simply discard this response, the finalization of the last range will be skipped, halting the entire sync process indefinitely. The test case TestSyncWithUnevenStorage can reproduce the scenario described above.

Besides, this pull request also defines the common variables MaxAddress and MaxHash.

@rjl493456442 rjl493456442 changed the title eth/protocols/snap: enhance snap sync and fix some corner cases eth/protocols/snap: fix snap sync halt when meet nil storage range Oct 12, 2023
@holiman
Copy link
Contributor

holiman commented Oct 12, 2023

FYI, there was no existing test which triggered the chunked-storage-fetch, so I added one here: 2d426e9#diff-25511a64fb71eea2da50f30896725bd1e6f88edd887ee74f7b70bf2685b24059R791 . Might be useful for you too?

@rjl493456442
Copy link
Member Author

Comment on lines +1806 to +1824
for i := 0; i < 3; i++ {
var n int
for {
n = mrand.Intn(15) // the last range is set empty deliberately
if _, ok := chosen[byte(n)]; ok {
continue
}
chosen[byte(n)] = struct{}{}
break
}
for j := 0; j < slots/3; j++ {
key := append([]byte{byte(n)}, testutil.RandBytes(31)...)
val, _ := rlp.EncodeToBytes(testutil.RandBytes(32))

elem := &kv{key, val}
tr.MustUpdate(elem.k, elem.v)
entries = append(entries, elem)
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

How about something like this?

Suggested change
for i := 0; i < 3; i++ {
var n int
for {
n = mrand.Intn(15) // the last range is set empty deliberately
if _, ok := chosen[byte(n)]; ok {
continue
}
chosen[byte(n)] = struct{}{}
break
}
for j := 0; j < slots/3; j++ {
key := append([]byte{byte(n)}, testutil.RandBytes(31)...)
val, _ := rlp.EncodeToBytes(testutil.RandBytes(32))
elem := &kv{key, val}
tr.MustUpdate(elem.k, elem.v)
entries = append(entries, elem)
}
}
for i := 0; i < slots; i++ {
key := testutil.RandBytes(32)
if key[0] >= 0x80 {
key[0] -= 0x80
}
val, _ := rlp.EncodeToBytes(testutil.RandBytes(32))
elem := &kv{key, val}
tr.MustUpdate(elem.k, elem.v)
entries = append(entries, elem)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, if I do

               if key[0] >= 0x80 {
                       key[0] = 0x00
               }

Then I start to hit the edgecase we want to repro pretty often

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

The changes looks good to me, but the testcase should be improved: it almost never triggers the condition as far as I can tell.

@holiman
Copy link
Contributor

holiman commented Oct 12, 2023

but the testcase should be improved: it almost never triggers the condition as far as I can tell.

I take that back, it seems ok to me!

@rjl493456442
Copy link
Member Author

Deployed it on 05, and it can finish snap sync.

@holiman holiman added this to the 1.13.4 milestone Oct 13, 2023
@holiman holiman changed the title eth/protocols/snap: fix snap sync halt when meet nil storage range eth/protocols/snap: fix snap sync failure on empty storage range Oct 13, 2023
@holiman holiman merged commit 1cb3b6a into ethereum:master Oct 13, 2023
2 checks passed
devopsbo3 pushed a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
…ereum#28306)

This change addresses an issue in snap sync, specifically when the entire sync process can be halted due to an encountered empty storage range.

Currently, on the snap sync client side, the response to an empty (partial) storage range is discarded as a non-delivery. However, this response can be a valid response, when the particular range requested does not contain any slots.

For instance, consider a large contract where the entire key space is divided into 16 chunks, and there are no available slots in the last chunk [0xf] -> [end]. When the node receives a request for this particular range, the response includes:

    The proof with origin [0xf]
    A nil storage slot set

If we simply discard this response, the finalization of the last range will be skipped, halting the entire sync process indefinitely. The test case TestSyncWithUnevenStorage can reproduce the scenario described above.

In addition, this change also defines the common variables MaxAddress and MaxHash.
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
devopsbo3 added a commit to HorizenOfficial/go-ethereum that referenced this pull request Nov 10, 2023
sduchesneau pushed a commit to streamingfast/go-ethereum that referenced this pull request Dec 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants