-
Notifications
You must be signed in to change notification settings - Fork 716
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
client: Snap Sync Scenario Improvements #3200
Changes from 11 commits
5748f81
c543e69
9b27598
965e484
6c96098
7d5f18d
c90a46a
ac50860
60dbcfb
e76dc8d
e72ea25
cbd0820
d988c88
75cf3aa
1dcddb4
fe14b3c
9af7efd
8c9d1c8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,7 +27,7 @@ | |
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) | ||
|
||
type StorageDataResponse = StorageData[][] & { completed?: boolean } | ||
|
||
|
@@ -65,6 +65,7 @@ | |
|
||
export type JobTask = { | ||
storageRequests: StorageRequest[] | ||
multi: boolean | ||
} | ||
|
||
export class StorageFetcher extends Fetcher<JobTask, StorageData[][], StorageData[]> { | ||
|
@@ -151,26 +152,34 @@ | |
* @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 { | ||
const { task, partialResult } = job | ||
if (task.storageRequests.length > 1 || task.storageRequests[0].first === BIGINT_0) { | ||
// peer does not respect origin or limit for multi-account storage fetch | ||
return setLengthLeft(bigIntToBytes(BIGINT_0), 32) | ||
} | ||
const { first } = task.storageRequests[0]! | ||
let origin = undefined | ||
if (partialResult) { | ||
const lastSlotArray = partialResult[partialResult.length - 1] | ||
const lastSlot = lastSlotArray[lastSlotArray.length - 1] | ||
origin = bigIntToBytes(bytesToBigInt(lastSlot.hash) + BIGINT_1) | ||
} else { | ||
origin = bigIntToBytes(first + BIGINT_1) | ||
let origin: Uint8Array | ||
|
||
try { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 |
||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 setLengthLeft(origin, 32) | ||
return new Uint8Array(0) | ||
} | ||
|
||
private getLimit(job: Job<JobTask, StorageData[][], StorageData[]>): Uint8Array { | ||
const { task } = job | ||
if (task.storageRequests.length > 1) { | ||
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 +227,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.storageRequests.length === 1) { | ||
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 +254,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.length} hashset ${task.storageRequests.length} ` | ||
`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 +335,7 @@ | |
|
||
// single account requests should check if task range is satisfied since origin and limit | ||
// are being respected | ||
if (task.storageRequests.length === 1 && !(task.storageRequests[0].first === BIGINT_0)) { | ||
if (task.multi === false) { | ||
if (!hasRightElement) { | ||
// all data has been fetched for account storage trie | ||
completed = true | ||
|
@@ -383,7 +398,7 @@ | |
const highestReceivedhash = accountSlots[accountSlots.length - 1].hash | ||
let updateHighestReceivedHash = false | ||
const request = job.task.storageRequests[0] | ||
if (request.first > BIGINT_0) { | ||
if (job.task.multi === false) { | ||
updateHighestReceivedHash = true | ||
} | ||
|
||
|
@@ -400,7 +415,12 @@ | |
this.accountToHighestKnownHash.delete(bytesToHex(request.accountHash)) | ||
} | ||
|
||
return Object.assign([], fullResult, { requests: job.task.storageRequests }) | ||
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 +435,9 @@ | |
* Store fetch result. Resolves once store operation is complete. | ||
* @param result fetch result | ||
*/ | ||
async store(result: StorageData[][] & { requests: StorageRequest[] }): Promise<void> { | ||
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 +452,12 @@ | |
let slotCount = 0 | ||
const storagePromises: Promise<unknown>[] = [] | ||
result[0].map((slotArray, i) => { | ||
const accountHash = result.requests[i].accountHash | ||
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 +521,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 +550,7 @@ | |
], | ||
first: myFirst, | ||
count: max, | ||
multi: false, | ||
} | ||
tasks.push(task) | ||
myFirst += BigInt(max) | ||
|
@@ -540,6 +569,7 @@ | |
], | ||
first: myFirst, | ||
count: myCount, | ||
multi: false, | ||
} | ||
tasks.push(task) | ||
myFirst += BigInt(myCount) | ||
|
@@ -587,12 +617,13 @@ | |
let fullJob = undefined | ||
if (this.storageRequests.length > 0) { | ||
fullJob = { | ||
task: { storageRequests: this.storageRequests }, | ||
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 { | ||
|
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.
wouldn't this be bigint anyway?
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.
Yes, I think this was something I did while debugging and forgot to clean up. Fixed.