client: Snap Sync Scenario Improvements #3200
Conversation
Codecov ReportAttention:
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
…to snap-sync-debugging
…s-monorepo into snap-sync-debugging
|
Updated this via UI. @scorbajio what is the status of this PR? |
This is ready for review. cc @g11tech |
| @@ -27,7 +27,7 @@ import type { Job, SnapFetcherDoneFlags } from './types' | |||
| import type { Debugger } from 'debug' | |||
| const { debug: createDebugLogger } = debugDefault | |||
|
|
|||
| const TOTAL_RANGE_END = BIGINT_2 ** BIGINT_256 - BIGINT_1 | |||
| const TOTAL_RANGE_END = BigInt(BIGINT_2 ** BIGINT_256 - BIGINT_1) | |||
There was a problem hiding this comment.
wouldn't this be bigint anyway?
There was a problem hiding this comment.
Yes, I think this was something I did while debugging and forgot to clean up. Fixed.
|
|
||
| if (task.multi === true) { | ||
| // peer does not respect origin or limit for multi-account storage fetch | ||
| return setLengthLeft(bigIntToBytes(BIGINT_0), 32) |
There was a problem hiding this comment.
even if peer doesn't respect the origin or limit, but if we provide it, does it give an issue (just curious)
There was a problem hiding this comment.
It seems like origin/limit is still used while servicing the request, but it's a bit open ended (see https://github.com/ethereum/go-ethereum/blob/a193bb0c730e413db56424a084cc172892c68dd5/eth/protocols/snap/handler.go#L346). I just use the max range if it's a multi-account request to be safe.
| origin = bigIntToBytes(first + BIGINT_1) | ||
| let origin: Uint8Array | ||
|
|
||
| try { |
There was a problem hiding this comment.
could you add a comment to code which particular error scenario motivated you to put this in try/catch
There was a problem hiding this comment.
The parsing of partialResult was resulting in indexing and undefined errors since lastSlot was an array in certain instances instead of a slot object. I may need to rethink some of the typing we are doing in the fetchers, that's why it was necessary to use ts-ignore. I will add a comment about it to the code.
|
Updated this via UI |
holgerd77
changed the title
The goal of this PR is to debug the current snap sync implementation and verify that the following scenarios from @g11tech are handled as expected: