Conversation
|
Converting to draft because this depends on 65bf637 |
5e51121 to
ca0c06b
Compare
ca0c06b to
d85b6f7
Compare
d85b6f7 to
d646771
Compare
d646771 to
243df2d
Compare
rustaceanrob
left a comment
There was a problem hiding this comment.
Left an initial opinion regarding PeerManager
90cbae7 to
b4e3dfe
Compare
rustaceanrob
left a comment
There was a problem hiding this comment.
Looks like a good start. I'm doing an IBD on mainnet right now, and I'm seeing a kind of stuttering behavior where every once and a while (say 10k blocks), the processing will be stuck on a block for a small amount of time. It's hard to tell if that is held up on kernel or the network. Either way, this is a good baseline to experiment with.
On memory,8 peers * 16 blocks * 4mb is about a half GB of memory at maximum. This is a worst case as well, so something I might try is increasing that to something like 96 blocks to see if that meaningfully affects the throughput.
b4e3dfe to
3975ec9
Compare
|
Made the suggested changes, rebased, and I'll run different batch sizes and report the metrics. |
|
Needs rebase |
Blocks are fetched from a single peer, so the sync runs at the speed of whichever peer was chosen and makes no progress while that peer is slow or unresponsive. Raise DEFAULT_MAX_PEERS from 1 to 8 and give the peers a shared DownloadState holding the blocks left to fetch, the ones in flight, and the ones already received. populate_download_queue fills it by walking back from the best header to the active chain. A block is claimed by one peer, and a peer that disconnects requeues whatever it still owes, so no block is left unfetched. Blocks now arrive out of order, so the block_buffer each peer kept in AwaitingBlock moves into DownloadState and the validation thread drains it. is_on_active_chain releases a block only once its parent has connected, so the kernel sees each chain in order up to whatever has connected so far. Once no peer has work left, the oldest buffered block is sent regardless, since it belongs to a competing branch that will never satisfy that check on its own.
3975ec9 to
8a5e536
Compare
|
Rebased. Previously we kept our own copy of the tip in Because we ask the kernel now, the check has to happen on the block processing thread, which owns the chainstate. Peers just buffer what arrives. Got rid of the block channel because the buffer passes the blocks now. Previously we flushed a peer's leftovers once it had received everything it asked for, because a competing branch never gets a parent onto the active chain and those blocks would sit there forever. With a shared buffer no single peer finishing tells us that anymore, so now we wait until no peer has work left and send the oldest block we hold. Also updated the README. |
| connection to this peer happens to fail for some reason, a new peer will be selected. | ||
| A direct connection can also be selected from the command line. See `--help` for | ||
| this. | ||
| IBD is done from several peers at once, selected from the DNS seed nodes. The |
There was a problem hiding this comment.
Thanks for updating, but I think we should perhaps remove this paragraph and add it to a doc/design.md or something. I think most users will just want to tinker with the binaries so we should keep the readme brief
|
I'll re-review tomorrow, thanks! |
|
Ok, working on a proper multi-peer integration test, but which needs |
rustaceanrob
left a comment
There was a problem hiding this comment.
Reviewed with both an LLM and myself a few times. The logic makes sense to me here. I will let you update the readme and any modifications you would like to add before merge
|
Okay, I'm going to do a little bit more performance/stress testing of the post-rebase changes. I'll push up the documentation changes and anything else if need be. |
Initial block download fetches blocks from a single peer, so the sync runs at that peer's pace and stalls when it is slow or unresponsive.
This connect to several peers at once, eight by default similar to Core, and split the block download between them. A shared queue hands out batches of hashes and an in-flight set keeps two peers from requesting the same block. When a peer disconnects, the blocks it still owed return to the queue.
This PR is following Bitcoin Core's behavior in the following ways:
DEFAULT_MAX_PEERS = 8MAX_OUTBOUND_FULL_RELAY_CONNECTIONSDOWNLOAD_BATCH_SIZE = 16MAX_BLOCKS_IN_TRANSIT_PER_PEERDownloadState::in_flightmapBlocksInFlightrequeue_unreceivedFinalizeNodeIt diverges from Core on block ordering. Core writes blocks as they arrive and lets chain activation order them, bounded by
BLOCK_DOWNLOAD_WINDOW. kernel-node already hands blocks to the kernel in chain order, so this keeps that behavior troughbuffer_and_drainwhich holds each arriving block by parent hash and releases an unbroken run once the parent has been handed off. As an aside: I am curious as to whether or why the ordering is needed at all, but that feels like a separate question.Possibly useful additions to the kernel:
btck_block_tree_entry_has_data- Tells us whether a block's data is already on disk, so the download queue can ask the kernel what is missing instead of tracking it separately. I have a draft of this along withhas_undoandis_valid.btck_ValidationInterfaceActiveTipChange- Tells us when the tip moves, allowing the node to follow the kernel instead of keeping a copy.btck_chainstate_manager_is_initial_block_download- Maybe this would be better than usingSynchronizationState, which saves the value from a notification, to tell us whether the node is still catching up?btck_ValidationInterfaceUpdatedBlockTip- It carries the fork point, but not sure if it's useful beyond that.