Backfiller #3263
Merged
Backfiller #3263
Conversation
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
arnetheduck
added a commit
that referenced
this pull request
Jan 9, 2022
Trusted node sync, aka checkpoint sync, allows syncing tyhe chain from a trusted node instead of relying on a full sync from genesis. Features include: * sync from any slot, including the latest finalized slot * backfill blocks either from the REST api (default) or p2p (#3263) Future improvements: * top up blocks between head in database and some other node - this makes for an efficient backup tool * recreate historical state to enable historical queries
Merged
arnetheduck
added a commit
that referenced
this pull request
Jan 9, 2022
Trusted node sync, aka checkpoint sync, allows syncing tyhe chain from a trusted node instead of relying on a full sync from genesis. Features include: * sync from any slot, including the latest finalized slot * backfill blocks either from the REST api (default) or p2p (#3263) Future improvements: * top up blocks between head in database and some other node - this makes for an efficient backup tool * recreate historical state to enable historical queries
arnetheduck
added a commit
that referenced
this pull request
Jan 11, 2022
Trusted node sync, aka checkpoint sync, allows syncing tyhe chain from a trusted node instead of relying on a full sync from genesis. Features include: * sync from any slot, including the latest finalized slot * backfill blocks either from the REST api (default) or p2p (#3263) Future improvements: * top up blocks between head in database and some other node - this makes for an efficient backup tool * recreate historical state to enable historical queries
arnetheduck
added a commit
that referenced
this pull request
Jan 12, 2022
Trusted node sync, aka checkpoint sync, allows syncing tyhe chain from a trusted node instead of relying on a full sync from genesis. Features include: * sync from any slot, including the latest finalized slot * backfill blocks either from the REST api (default) or p2p (#3263) Future improvements: * top up blocks between head in database and some other node - this makes for an efficient backup tool * recreate historical state to enable historical queries
arnetheduck
added a commit
that referenced
this pull request
Jan 17, 2022
* Trusted node sync Trusted node sync, aka checkpoint sync, allows syncing tyhe chain from a trusted node instead of relying on a full sync from genesis. Features include: * sync from any slot, including the latest finalized slot * backfill blocks either from the REST api (default) or p2p (#3263) Future improvements: * top up blocks between head in database and some other node - this makes for an efficient backup tool * recreate historical state to enable historical queries * fixes * load genesis from network metadata * check checkpoint block root against state * fix invalid block root in rest json decoding * odds and ends * retry looking for epoch-boundary checkpoint blocks
arnetheduck
commented
Jan 18, 2022
| except CancelledError: | ||
| if not(isNil(peer)): | ||
| man.pool.release(peer) | ||
| true |
There was a problem hiding this comment.
why not simple.. break here?
There was a problem hiding this comment.
Just because I do not want to investigate one more for/while+exception+break issue.
There was a problem hiding this comment.
it wouldn't be bad to add tests for this in chronos - basically, a few simple for/while/finally etc sanity tests
mratsim
reviewed
Jan 18, 2022
| let forked = | ||
| res.map() do (blcks: seq[phase0.SignedBeaconBlock]) -> auto: | ||
| blcks.mapIt(ForkedSignedBeaconBlock.init(it)) | ||
| return forked |
There was a problem hiding this comment.
I think a template is worth it, so many wasted lines/mindspace
template debugLog(msg: string): untyped {.dirty.} =
debug msg,
peer = peer, slot = req.slot, count = req.count,
step = req.step, peer_speed = peer.netKbps(),
direction = man.direction, error = $res.error(),
topics = "syncman"There was a problem hiding this comment.
typically, this is solved with logScope, but logscope is unfortunately unreliable (cc @zah can we do anything about that?)
beacon_chain/sync/sync_manager.nim
Outdated
| @@ -303,10 +341,6 @@ proc syncStep[A, B](man: SyncManager[A, B], index: int, peer: A) {.async.} = | |||
| peer_speed = peer.netKbps(), index = index, | |||
| direction = man.direction, topics = "syncman" | |||
| peer.updateScore(PeerScoreUseless) | |||
|
|
|||
| # Give the peer time to do some syncing | |||
| await sleepAsync(StatusExpirationTime div 2) | |||
There was a problem hiding this comment.
How do we avoid dos-ing peers?
There was a problem hiding this comment.
Why do you think we DOS-ing peers? We descored peer which is not useful for us and we return it back to the PeerPool. So it is not supposed to be returned for next step.
| peer = peer, index = index, | ||
| peer_score = peer.getScore(), peer_speed = peer.netKbps(), | ||
| errName = exc.name, errMsg = exc.msg, direction = man.direction, | ||
| topics = "syncman" |
There was a problem hiding this comment.
I think the sync_manager needs a debugLog template to avoid littering the codebase with those. (though that might cause issue with filename:linenumber logic from Chronicles?
Backfilling is the process of downloading historical blocks via P2P that are required to fulfill `GetBlocksByRange` duties - this happens during both trusted node and finalized checkpoint syncs. In particular, backfilling happens after syncing to head, such that attestation work can start as soon as possible.
Remove usage of `awaitne`. Add cancellation support. Remove unneeded `sleepAsync()` if peer's head is older than needed. Add `direction` field to all logs. Fix syncmanager wedge issue. Add proper resource cleaning procedure on backward sync finish.
cheatfate
force-pushed
the
chaindag-backfill
branch
from
24bde37
to
df72fec
Compare
etan-status
added a commit
to etan-status/nimbus-eth2
that referenced
this pull request
Feb 13, 2022
When initializing backfill sync, the implementation intends to start at the first unknown slot (`1` before tail). However, an incorrect variable is passed, and backfill sync actually starts at the tail slot instead. This patch corrects this by passing the intended variable. The problem was introduced with the original backfill implementation at status-im#3263.
zah
pushed a commit
that referenced
this pull request
Feb 14, 2022
When initializing backfill sync, the implementation intends to start at the first unknown slot (`1` before tail). However, an incorrect variable is passed, and backfill sync actually starts at the tail slot instead. This patch corrects this by passing the intended variable. The problem was introduced with the original backfill implementation at #3263.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Backfilling is the process of downloading historical blocks via P2P that
are required to fulfill
GetBlocksByRangeduties - this happens duringboth trusted node and finalized checkpoint syncs.
In particular, backfilling happens after syncing to head, such that
attestation work can start as soon as possible.
This PR is captures the part of #3209 that still needs more work.