client: create snap sync fetcher to sync accounts #2107

scorbajio · 2022-08-06T01:59:02Z

AccountFetcher
Capture data now done/part of

client: Enhance snap protocol spec tests #2157
- getAccountRange
- getStorageRanges
- getByteCodes
- getTrieNodes

My understanding is that there needs to be one fetcher per each get call defined in the snap specs. Open to suggestions on this design plan.

codecov · 2022-08-06T02:15:59Z

Codecov Report

Merging #2107 (420d087) into master (0cf9654) will decrease coverage by 0.72%.
The diff coverage is 100.00%.

Additional details and impacted files

Flag	Coverage Δ
block	`?`
blockchain	`90.19% <ø> (ø)`
client	`?`
common	`95.82% <ø> (ø)`
devp2p	`91.62% <ø> (-0.09%)`	⬇️
ethash	`?`
evm	`83.36% <ø> (ø)`
rlp	`∅ <ø> (?)`
statemanager	`?`
trie	`?`
tx	`?`
util	`84.50% <100.00%> (+0.01%)`	⬆️
vm	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

scorbajio · 2022-08-06T04:38:11Z

When calling getAccountRange, the proof and accounts arrays contain data elements that look like this:

{
  account: {
    hash: <Buffer 59 41 33 56 5c 9e 83 0b 70 3d d8 a3 24 55 45 73 d2 16 e1 60 a0 83 28 db 62 f7 85 c9 ff 90 61 0d>,
    body: [
      <Buffer 05>,
      <Buffer 16 48 4e a6 bc cd 3f>,
      Uint8Array(0) [],
      Uint8Array(0) []
    ]
  },
  proof: <Buffer f8 66 9d 3b a0 c8 17 db 1f e9 ac 90 64 49 6c 76 75 81 3b 54 3d c9 ec 10 5c 1c 24 4e 43 3b c8 9c b8 46 f8 44 01 80 a0 56 e8 1f 17 1b cc 55 a6 ff 83 45 ... 54 more bytes>
}

Sometimes, the proof is undefined:

  account: {
    hash: <Buffer 59 41 34 ee 04 72 a6 7a b4 0e 94 e5 aa bb 00 74 02 a7 74 fb c9 d8 bd 2c 76 bc 36 36 9c b5 fa f1>,
    body: [
      <Buffer 01>,
      Uint8Array(0) [],
      <Buffer 8f b4 c3 c2 7f 69 21 c6 44 dd fc 94 24 97 5b 91 b4 58 82 47 ca 2a 1d ec 36 96 da 65 a3 97 bb f5>,
      <Buffer f1 b5 74 43 1f 38 38 d9 cd ff 6e 70 1a fd 5a 05 86 52 da b5 ae 55 23 28 8a 83 d5 fa d7 69 61 39>
    ]
  },
  proof: undefined

Will need to also finish implementing StorageRanges, ByteCodes, and TrieNodes and collecting data for them too.

scorbajio · 2022-08-06T04:47:52Z

Now that there is reference data for getAccountRange, next steps are to figure out how to divide the fetching process into smaller tasks, if necessary. When fetching accounts, we need to verify the integrity of the account data using the included proof.

g11tech · 2022-08-06T06:47:04Z

When calling getAccountRange, the proof and accounts arrays contain data elements that look like this:
{
  account: {
    hash: <Buffer 59 41 33 56 5c 9e 83 0b 70 3d d8 a3 24 55 45 73 d2 16 e1 60 a0 83 28 db 62 f7 85 c9 ff 90 61 0d>,
.....
Sometimes, the proof is `undefined`:
account: {
hash: <Buffer 59 41 34 ee 04 72 a6 7a b4 0e 94 e5 aa bb 00 74 02 a7 74 fb c9 d8 bd 2c 76 bc 36 36 9c b5 fa f1>,
....
proof: undefined
Will need to also finish implementing `StorageRanges`, `ByteCodes`, and `TrieNodes` and collecting data for them too.

awesome glad to see you got it working!

g11tech · 2022-08-06T06:52:17Z

yes we need separate fetchers for fetching each of the ranges 👍

scorbajio · 2022-08-07T01:07:38Z

@g11tech I wasn't able to get forceSnapSync option to work. I would set it like this:

DEBUG=devp2p:* npm run client:start:dev2 -- --forceSnapSync=true ...

I ended up hardcoding it to true. Am I setting it incorrectly?

g11tech · 2022-08-07T15:15:04Z

@g11tech I wasn't able to get forceSnapSync option to work. I would set it like this:
DEBUG=devp2p:* npm run client:start:dev2 -- --forceSnapSync=true ...
I ended up hardcoding it to true. Am I setting it incorrectly?

looks alright to me the way you set, i will check and add a fix in this PR

g11tech · 2022-09-12T05:08:53Z

🎉 the fetcher syncs range on mainnet! (and then create new tasks and sync them further)

g11tech · 2022-09-12T05:11:59Z

packages/client/lib/sync/fetcher/accountfetcher.ts

+    if (accounts.length > 0 && accounts[accounts.length - 1]?.hash.compare(limit) >= 0) {
+      return false
+    } else {
+      // TODO: Check if there is a proof of missing limit in state


@jochem-brouwer @scorbajio Is there an early check that can be done which can see there are no more accounts between the last and the limit

verifyRangeProof returns a boolean if there are more items or not. We could maybe early-check hasRightElement but I don't know if this really speeds it up?

I was originally thinking that if there is an empty response to a request, it means there are no accounts in a given range. There might also be a cryptographic way to check this.

g11tech · 2022-09-12T05:12:36Z

packages/client/lib/sync/fetcher/accountfetcher.ts

+    await trie.verifyRangeProof(stateRoot, origin, keys[keys.length - 1], keys, values, <any>proof)
+  }
+
+  private isMissingRightRange(


@jochem-brouwer @scorbajio pls review the correctness of this fn

Isn't this intended to be the same as trie's hasRightElement?

It seems like the way it is being done is in line with the approach used in geth. If the returned account hash is past the limit requested, it is discarded since it is a part of the next range. This also guarantees there are no more accounts to the right of the last account received within the requested range.

g11tech · 2022-09-12T05:15:22Z

packages/client/lib/sync/fetcher/accountfetcher.ts

+    } else {
+      // validate the proof
+      try {
+        // verifyRangeProof will also verify validate there are no missed states between origin and


@jochem-brouwer @scorbajio pls review if my understanding of proof verification is correct here w.r.t origin that using origin in verifyRangeProof (see the definition of this private function) will validate that there are no missed accounts between origin and the first account recieved

It /should/ throw if there are missing nodes, but we should probably test this (or it is already tested in trie)

The way you are verifying makes sense to me. Could be useful to add some try-except blocks and log any errors in each function body.

g11tech · 2022-09-12T05:51:37Z

packages/client/lib/sync/fetcher/accountfetcher.ts

+   * @param result fetch result
+   */
+  async store(result: AccountData[]): Promise<void> {
+    this.debug(`Stored ${result.length} accounts in account trie`)


right now store doesn't do anything, as a separate helper class with be added in a followup PR to track and aid the fetcher

…s-monorepo into snap-client-fetchers

g11tech · 2023-02-14T18:30:18Z

packages/client/lib/sync/fetcher/blockfetcherbase.ts

+   * @param job
+   * @param withIndex pass true to additionally output job.index
+   */
+  jobStr(job: Job<JobTask, JobResult, StorageItem>, withIndex = false) {


is this something that relates to PR or you added for convenience factor?

ahh i see because you added jobstr as abstract method

g11tech · 2023-02-14T18:30:36Z

packages/client/lib/sync/fetcher/blockfetcher.ts

@@ -107,7 +107,6 @@ export class BlockFetcher extends BlockFetcherBase<Block[], Block> {
    } else if (result.length > 0 && result.length < job.task.count) {
      // Save partial result to re-request missing items.
      job.partialResult = result
-      this.debug(`Partial result received=${result.length} expected=${job.task.count}`)


why remove debug log?

g11tech

looks good though still needs to be extensively tested locally and on testnets for the fetcher start/end conditions and consistency, but any fixes regarding those can be done in followup PRs as this is still wip

gitpoap-bot · 2023-02-14T18:58:17Z

Congrats, your important contribution to this open-source project has earned you a GitPOAP!

GitPOAP: 2023 EthereumJS Contributor:

Head to gitpoap.io & connect your GitHub account to mint!

Learn more about GitPOAPs here.

scorbajio force-pushed the snap-client-fetchers branch from 295c386 to 4b7480d Compare August 6, 2022 02:13

scorbajio mentioned this pull request Aug 6, 2022

Master tracker for snap sync #2100

Closed

19 tasks

scorbajio mentioned this pull request Aug 6, 2022

client: snap protocol fetchers #2070

Closed

3 tasks

g11tech mentioned this pull request Aug 6, 2022

client: Add client protocol and synchronizer for snap sync #1897

Merged

2 tasks

scorbajio changed the title ~~client: create account fetcher base~~ client: create snap sync fetchers Aug 6, 2022

scorbajio added PR state: WIP package: client labels Aug 6, 2022

g11tech mentioned this pull request Aug 9, 2022

client: Make fetcher typesafe in syncronizers #2120

Merged

g11tech force-pushed the snap-client-fetchers branch from a8d306b to 9a56af0 Compare August 9, 2022 12:05

scorbajio force-pushed the snap-client-fetchers branch from 430b925 to 877a5e1 Compare August 17, 2022 03:36

g11tech force-pushed the snap-client-fetchers branch 2 times, most recently from 12272fb to d472cb2 Compare August 22, 2022 18:52

g11tech force-pushed the snap-client-fetchers branch 2 times, most recently from f3a7314 to f49938a Compare September 7, 2022 13:53

scorbajio mentioned this pull request Sep 11, 2022

client: snap client account fetcher #2277

Closed

g11tech force-pushed the snap-client-fetchers branch from 8451520 to c0995bb Compare September 11, 2022 19:50

g11tech reviewed Sep 12, 2022

View reviewed changes

g11tech force-pushed the snap-client-fetchers branch from 7ff31da to 9fef3e0 Compare September 12, 2022 10:14

g11tech changed the title ~~client: create snap sync fetchers~~ client: create snap sync fetcher to sync accounts Sep 12, 2022

scorbajio and others added 15 commits December 27, 2022 02:59

Add logic for dividing hash ranges and adding them as tasks

ec61f20

Increment count by 1 before next iteration

08d2607

client/snap: remove unnecessary account fetcher logic

1281ef6

client/snap: correctly feed the right values to verifyRangeProof

690f193

lint fixes

5c63390

small cleanup

42605ec

fix account fetcher with previous fixes

67d784e

overhaul and simplify the fetcher and add partial results handling

3cc3130

cleanup comments

465c95a

fix fetch spec tests

79d34e3

Check if right range is missing using return value from verifyRangeProof

61f8400

Add accountfetcher tests

baa491e

Remove request test

1c3980c

Correct return type

da73d2e

Add request test

5a73cd3

g11tech force-pushed the snap-client-fetchers branch from 0e386c8 to 5a73cd3 Compare December 26, 2022 21:32

scorbajio added 5 commits January 27, 2023 16:42

Merge branch 'snap-client-fetchers' of github.com:scorbajio/ethereumj…

8181265

…s-monorepo into snap-client-fetchers

Add test for proof verification

f380d5a

Fix linting issues

5143038

Fix test

2d33b05

Update comment

5b46855

scorbajio force-pushed the snap-client-fetchers branch from 92b1c19 to 5b46855 Compare February 10, 2023 14:50

scorbajio and others added 2 commits February 11, 2023 02:40

Sync with master

ba1ed2a

Merge remote-tracking branch 'origin/master' into snap-client-fetchers

c6c44f6

g11tech reviewed Feb 14, 2023

View reviewed changes

reduce diff

420d087

g11tech approved these changes Feb 14, 2023

View reviewed changes

g11tech merged commit a1ca973 into ethereumjs:master Feb 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: create snap sync fetcher to sync accounts #2107

client: create snap sync fetcher to sync accounts #2107

scorbajio commented Aug 6, 2022 •

edited by g11tech

Loading

codecov bot commented Aug 6, 2022 •

edited

Loading

scorbajio commented Aug 6, 2022 •

edited

Loading

scorbajio commented Aug 6, 2022 •

edited

Loading

g11tech commented Aug 6, 2022 •

edited

Loading

g11tech commented Aug 6, 2022

scorbajio commented Aug 7, 2022

g11tech commented Aug 7, 2022

g11tech commented Sep 12, 2022

g11tech Sep 12, 2022

jochem-brouwer Sep 12, 2022

scorbajio Sep 19, 2022 •

edited

Loading

g11tech Sep 12, 2022

jochem-brouwer Sep 12, 2022

scorbajio Oct 9, 2022 •

edited

Loading

g11tech Sep 12, 2022 •

edited

Loading

jochem-brouwer Sep 12, 2022

scorbajio Sep 19, 2022

g11tech Sep 12, 2022

g11tech Feb 14, 2023

g11tech Feb 14, 2023

g11tech Feb 14, 2023

g11tech left a comment

gitpoap-bot bot commented Feb 14, 2023

client: create snap sync fetcher to sync accounts #2107

client: create snap sync fetcher to sync accounts #2107

Conversation

scorbajio commented Aug 6, 2022 • edited by g11tech Loading

codecov bot commented Aug 6, 2022 • edited Loading

Codecov Report

scorbajio commented Aug 6, 2022 • edited Loading

scorbajio commented Aug 6, 2022 • edited Loading

g11tech commented Aug 6, 2022 • edited Loading

g11tech commented Aug 6, 2022

scorbajio commented Aug 7, 2022

g11tech commented Aug 7, 2022

g11tech commented Sep 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scorbajio Sep 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scorbajio Oct 9, 2022 • edited Loading

Choose a reason for hiding this comment

g11tech Sep 12, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

g11tech left a comment

Choose a reason for hiding this comment

gitpoap-bot bot commented Feb 14, 2023

scorbajio commented Aug 6, 2022 •

edited by g11tech

Loading

codecov bot commented Aug 6, 2022 •

edited

Loading

scorbajio commented Aug 6, 2022 •

edited

Loading

scorbajio commented Aug 6, 2022 •

edited

Loading

g11tech commented Aug 6, 2022 •

edited

Loading

scorbajio Sep 19, 2022 •

edited

Loading

scorbajio Oct 9, 2022 •

edited

Loading

g11tech Sep 12, 2022 •

edited

Loading