Separate Nakamoto DB for staging blocks #4373
Conversation
…nsus hash was processed
… to one without blocking writes to the other. This required some changes in how we handle processed blocks, but as long as there's a single thread responsible for finding and processing blocks, we should be fine
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## feat/nakamoto-inv-state-machine #4373 +/- ##
===================================================================
+ Coverage 76.81% 83.29% +6.47%
===================================================================
Files 447 448 +1
Lines 321012 321363 +351
===================================================================
+ Hits 246576 267668 +21092
+ Misses 74436 53695 -20741 ☔ View full report in Codecov by Sentry. |
kantai
left a comment
kantai
left a comment
There was a problem hiding this comment.
This LGTM -- just have some pretty superficial comments.
| // within the same, single thread. Also, it's *very important* that this update | ||
| // succeeds, since *we have already processed* the block. | ||
|
|
||
| loop { |
There was a problem hiding this comment.
The two loops in this function should be (at least) factored out of this function. I'm also not sure why this looping retry logic needs to be here anyways -- other tx_begin(), execute, and commit() blocks don't have this logic, so why is this specifically necessary here?
There was a problem hiding this comment.
Once we commit the block, we must update the processed/orphaned flags in the staging DB. Failure to do that is either a panic (which leaves the node in a corrupt state) or a loop (which at least gives the operator a chance to fix the underlying issue).
| let pox_constants = sort_tx.context.pox_constants.clone(); | ||
| let (receipt, clarity_commit) = match NakamotoChainState::append_block( | ||
|
|
||
| // NOTE: because block status is updated in a separate transaction, we need `chainstate_tx` |
There was a problem hiding this comment.
haha sneaky sneaky. Very helpful comment.
kantai
left a comment
kantai
left a comment
There was a problem hiding this comment.
LGTM -- my only comment is that lazy_static should be avoided if possible.
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This actually fixes #3860, which was closed prematurely since it was not addressed by PR #3992. Doing this now because this is a blocker for the block sync system.
This PR creates a separate DB for holding Nakamoto blocks, separate from the DB that holds processed Nakamoto block headers. The reason this is necessary is because we can expect writes to occur concurrently on both DBs -- the network stack will create transactions against the staging blocks DB to store blocks it receives, and both the miner and chains-coordinator will create transactions on the headers DB as they create / process blocks. In Stacks today, these two data live in the same DB, and it regularly leads to DB contention since the chains-coordinator's and miner's block-processing regularly prevents the block downloader from storing downloaded blocks (which slows down IBD and block delivery).
Because latency is much more of a concern in Nakamoto, it's going to be important that the system can open both kinds of transactions concurrently. This PR makes it possible by storing staging blocks separately from processed block headers.
The most substantial change in this PR is that the act of updating the
processedflag on a block no longer executes within the same transaction that writes the block header. However, this shouldn't lead to any concurrency issues because (1) only the chains coordinator thread performs these tasks, and (2) the code to query processed blocks has been updated to read from both the staging blocks DB and headers DB (this won't block ongoing writes, or be blocked by ongoing writes, since we use WAL mode in both DBs)