client: Snap Sync Scenario Improvements #3200
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -65,6 +65,7 @@ | |
|
|
||
| export type JobTask = { | ||
| storageRequests: StorageRequest[] | ||
| multi: boolean | ||
| } | ||
|
|
||
| export class StorageFetcher extends Fetcher<JobTask, StorageData[][], StorageData[]> { | ||
|
|
@@ -151,26 +152,35 @@ | |
| * @returns origin of job is set using either @first property of fetcher or latest hash of partial job | ||
| */ | ||
| private getOrigin(job: Job<JobTask, StorageData[][], StorageData[]>): Uint8Array { | ||
| let origin: Uint8Array | ||
|
|
||
| // this try block contains code that is currently susceptible to typing issues and resulting errors | ||
| try { | ||
| const { task, partialResult } = job | ||
|
|
||
| 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. |
||
| } | ||
| const { first } = task.storageRequests[0]! | ||
| if (partialResult) { | ||
| const lastSlotArray = partialResult[partialResult.length - 1] | ||
| const lastSlot = lastSlotArray[lastSlotArray.length - 1] | ||
| // @ts-ignore | ||
| origin = bigIntToBytes(bytesToBigInt(lastSlot[lastSlot.length - 1].hash) + BIGINT_1) | ||
| } else { | ||
| origin = bigIntToBytes(first + BIGINT_1) | ||
| } | ||
| return setLengthLeft(origin, 32) | ||
| } catch (e: any) { | ||
| this.debug(e) | ||
| } | ||
| return new Uint8Array(0) | ||
| } | ||
|
|
||
| private getLimit(job: Job<JobTask, StorageData[][], StorageData[]>): Uint8Array { | ||
| const { task } = job | ||
| if (task.multi === true) { | ||
| // peer does not respect origin or limit for multi-account storage fetch | ||
| return setLengthLeft(bigIntToBytes(TOTAL_RANGE_END), 32) | ||
| } | ||
|
|
@@ -218,16 +228,18 @@ | |
| this.debug( | ||
| `requested account hashes: ${task.storageRequests.map((req) => bytesToHex(req.accountHash))}` | ||
| ) | ||
| this.debug(`request is multi: ${job.task.multi}`) | ||
|
|
||
| // only single account requests need their highest known hash tracked since multiaccount requests | ||
| // are guaranteed to not have any known hashes until they have been filled and switched over to a | ||
| // fragmented request | ||
| if (task.multi === false) { | ||
| const highestKnownHash = this.accountToHighestKnownHash.get( | ||
| bytesToHex(task.storageRequests[0].accountHash) | ||
| ) | ||
| if (highestKnownHash && compareBytes(limit, highestKnownHash) < 0) { | ||
| // skip this job and don't rerequest it if it's limit is lower than the highest known key hash | ||
| this.debug(`skipping request with limit lower than highest known hash`) | ||
| return Object.assign([], [{ skipped: true }], { completed: true }) | ||
| } | ||
| } | ||
|
|
@@ -243,7 +255,11 @@ | |
| // Reject the response if the hash sets and slot sets don't match | ||
| if (rangeResult === undefined || task.storageRequests.length < rangeResult.slots.length) { | ||
| this.debug( | ||
| `Slot set is larger than hash set: slotset ${ | ||
| rangeResult?.slots !== undefined ? rangeResult.slots.length : 0 | ||
| } hashset ${task.storageRequests.length} proofset ${ | ||
| rangeResult?.proof !== undefined ? rangeResult.proof.length : 0 | ||
| } ` | ||
| ) | ||
| return undefined | ||
| } | ||
|
|
@@ -320,7 +336,7 @@ | |
|
|
||
| // single account requests should check if task range is satisfied since origin and limit | ||
| // are being respected | ||
| if (task.multi === false) { | ||
| if (!hasRightElement) { | ||
| // all data has been fetched for account storage trie | ||
| completed = true | ||
|
|
@@ -383,7 +399,7 @@ | |
| const highestReceivedhash = accountSlots[accountSlots.length - 1].hash | ||
| let updateHighestReceivedHash = false | ||
| const request = job.task.storageRequests[0] | ||
| if (job.task.multi === false) { | ||
| updateHighestReceivedHash = true | ||
| } | ||
|
|
||
|
|
@@ -400,7 +416,12 @@ | |
| this.accountToHighestKnownHash.delete(bytesToHex(request.accountHash)) | ||
| } | ||
|
|
||
| return Object.assign( | ||
| [], | ||
| fullResult, | ||
| { requests: job.task.storageRequests }, | ||
| { multi: job.task.multi } | ||
| ) | ||
| } else { | ||
| if (updateHighestReceivedHash && highestReceivedhash !== undefined) { | ||
| this.accountToHighestKnownHash.set(bytesToHex(request.accountHash), highestReceivedhash) | ||
|
|
@@ -415,7 +436,9 @@ | |
| * Store fetch result. Resolves once store operation is complete. | ||
| * @param result fetch result | ||
| */ | ||
| async store( | ||
| result: StorageData[][] & { requests: StorageRequest[] } & { multi: boolean } | ||
| ): Promise<void> { | ||
| try { | ||
| if (JSON.stringify(result[0]) === JSON.stringify({ skipped: true })) { | ||
| // return without storing to skip this task | ||
|
|
@@ -430,7 +453,12 @@ | |
| let slotCount = 0 | ||
| const storagePromises: Promise<unknown>[] = [] | ||
| result[0].map((slotArray, i) => { | ||
| let accountHash | ||
| if (result.multi) { | ||
| accountHash = result.requests[i].accountHash | ||
| } else { | ||
| accountHash = result.requests[0].accountHash | ||
| } | ||
| const storageTrie = this.stateManager['_getStorageTrie'](accountHash) | ||
| for (const slot of slotArray as any) { | ||
| slotCount++ | ||
|
|
@@ -494,6 +522,7 @@ | |
| ) | ||
| tasks.unshift({ | ||
| storageRequests: this.storageRequests, // TODO limit max number of accounts per single fetch request | ||
| multi: true, | ||
| }) | ||
| this.storageRequests = [] // greedilly request as many account slots by requesting all known ones | ||
| return tasks | ||
|
|
@@ -522,6 +551,7 @@ | |
| ], | ||
| first: myFirst, | ||
| count: max, | ||
| multi: false, | ||
| } | ||
| tasks.push(task) | ||
| myFirst += BigInt(max) | ||
|
|
@@ -540,6 +570,7 @@ | |
| ], | ||
| first: myFirst, | ||
| count: myCount, | ||
| multi: false, | ||
| } | ||
| tasks.push(task) | ||
| myFirst += BigInt(myCount) | ||
|
|
@@ -587,12 +618,13 @@ | |
| let fullJob = undefined | ||
| if (this.storageRequests.length > 0) { | ||
| fullJob = { | ||
| task: { storageRequests: this.storageRequests, multi: true }, | ||
| } as Job<JobTask, StorageData[][], StorageData[]> | ||
| } else if (this.fragmentedRequests.length > 0) { | ||
| fullJob = { | ||
| task: { | ||
| storageRequests: [this.fragmentedRequests[0]], | ||
| multi: false, | ||
| }, | ||
| } as Job<JobTask, StorageData[][], StorageData[]> | ||
| } else { | ||
|
|
||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.