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

Snap sync moving root/healing implementation #3332

Open
wants to merge 63 commits into
base: master
Choose a base branch
from

Conversation

scorbajio
Copy link
Contributor

This change will add the ability to the snap sync fetchers to have their target roots updated and will explore adding continual healing to the snap sync process.

Copy link

codecov bot commented Mar 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 42.58%. Comparing base (d14a5ed) to head (c3946da).

Additional details and impacted files

Impacted file tree graph

Flag Coverage Δ
block 67.57% <ø> (ø)
blockchain 83.59% <100.00%> (+0.10%) ⬆️
client ?
common 89.85% <ø> (ø)
devp2p 0.00% <ø> (ø)
evm 65.18% <ø> (ø)
genesis 0.00% <ø> (ø)
mpt 52.12% <ø> (+0.07%) ⬆️
statemanager 67.41% <ø> (ø)
tx 76.70% <ø> (?)
util 71.23% <ø> (ø)
vm 58.28% <ø> (ø)
wallet 0.00% <ø> (ø)

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

Indigo Alpha and others added 23 commits March 27, 2024 11:28
…ly on protocol binding, small local refactor
if (fetcher === null) {
return
}
fetcher.updateStateRoot(latest!.stateRoot as Uint8Array)
Copy link
Contributor

Choose a reason for hiding this comment

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

no we don't need stateroot from peer, we already have latest stateroot coming in from CL

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought #3354 was for tracking and accessing the latest root/height from the peer latest function. Are you suggesting passing down the latest root from FCU calls?

@@ -276,6 +277,8 @@ export class AccountFetcher extends Fetcher<JobTask, AccountData[], AccountData>
}
break
case TrieNodeFetcher:
// TODO update to check if heal phase completed successfully and then continue with next
Copy link
Contributor

Choose a reason for hiding this comment

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

this function with this case would/should anyway be called when heal phase completed successfully

@@ -435,6 +447,10 @@ export class TrieNodeFetcher extends Fetcher<JobTask, Uint8Array[], Uint8Array>
return tasks
}

updateStateRoot(stateRoot: Uint8Array) {
Copy link
Contributor

Choose a reason for hiding this comment

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

need to see that the root is not continually updated.

so the thing is: you start trie node fetcher with latest root, keep requesting data till peers give you, if they start replying with empty, then we restart the entire fetcher with the new root.

so this is to be implemented as a while loop in the account fetcher's fetch where we start and run the trie node fetcher (either fetcher resolves or errors with error "out of range root", which will cause account fetcher to spin a new trie node fetcher with new root)

@scorbajio scorbajio mentioned this pull request Oct 1, 2024
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants