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
implemented snapshot synchronization #1473
implemented snapshot synchronization #1473
Conversation
342534c
to
8836811
Compare
8836811
to
424f598
Compare
// Send back anything accumulated (or empty in case of errors) | ||
return p2p.Send(peer.rw, AccountRangeMsg, &AccountRangePacket{ | ||
ID: req.ID, | ||
Accounts: accounts, |
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.
accounts
can be nil. Is it okay?
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.
Is there any test case to cover this case?
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.
I have added testcases, but encoding/decoding nil
accounts were okay. Am I missing something? Please take another look.
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.
If you are using testDownloader, DeliverSnapPacket() does nothing.
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.
@jeongkyun-oh I added my test case with the name of handler2_test.go
@jeongkyun-oh Is it possible to turn off this feature? For example, CNs do not want to provide snapshot request to avoid performance degradation. |
c4d46e4
to
4986395
Compare
4986395
to
1884c3d
Compare
} | ||
|
||
// incHash returns the next hash, in lexicographical order (a.k.a plus one) | ||
func incHash(h common.Hash) common.Hash { |
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.
This function doesn't handle the overflow. It seems that it is intended, right? We might need a comment for this.
snapshots.Disable() | ||
} | ||
logger.Warn("Enabling snapshot sync prototype") | ||
d.snapSync = true |
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.
What is d.snapSync
for? Do we have any reason to execute the above lines only once?
db83862
to
1072225
Compare
5e52674
to
39174c1
Compare
b40814b
to
39174c1
Compare
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.
LGTM, but need to have more test case.
Proposed changes
snapsync is a faster synchronisation feature derived from Ethereum. This PR ported the snapsync related PR from Ethereum.
Synchronisation methods from Ethereum
full
sync: fetch headers and bodies (transactions) only, but reexecute the downloaded transactions to output transaction receipts and world states. Klaytn supports only full sync for now.fast
sync: fetch headers, bodies, and receipts until pivot block (no trie verification), and download all the trie nodes on the pivot.snap
sync: fetch headers, bodies, and receipts until pivot block and download snapshot data (trie leaf nodes) on the pivot after which the entire trie is reconstructed.We had to fetch staking information as well to verify headers.
Snap sync components
klay
/istanbul
protocol.Snap sync process
0x000...000
...0xfff...fff
) to 16 spaces, and it requests the range to retrieve the snapshot account data whose key is within that range. So, max 16 snap peers can be served to send those data.TODO
Types of changes
Please put an x in the boxes related to your change.
Checklist
Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.
$ make test
)