chain: Overhaul persistence layer#450
Conversation
There was a problem hiding this comment.
Pull request overview
This PR overhauls the chain persistence and concurrency model by introducing MVCC-style snapshots for read-only access and a scratchpad for serialized writes, aiming to eliminate reader/writer contention in chain.Manager while preserving consistent views of committed chain state.
Changes:
- Refactors
chain.DBandchain.StoreintoSnapshot()+Scratchpad()APIs, with committed state exposed via snapshots and buffered writes via scratchpads. - Updates
chain.Managerto use snapshots for read methods and a scratchpad (behind a mutex) for state-mutating methods, plus adds concurrency-focused tests. - Updates callers/tests and BoltDB/MemDB/CacheDB implementations to support the new snapshot/scratchpad semantics; includes a changeset documenting the breaking API changes.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| wallet/wallet_test.go | Adapts wallet tests to NewDBStore / NewManager signature changes and new tip-state access. |
| syncer/syncer_test.go | Updates syncer tests for new store/manager initialization APIs. |
| rhp/v4/rpc_test.go | Updates test node setup for new store/manager initialization APIs. |
| miner_test.go | Updates miner tests for new store/manager initialization APIs. |
| internal/cmd/sync/main.go | Updates command to new NewDBStore / NewManager call patterns. |
| internal/cmd/calcswaps/main.go | Switches to scratchpad reads/writes and fixes height loop underflow behavior. |
| db.go | Reworks Bolt-backed DB into snapshot (read tx) + scratchpad (lazy write tx) model. |
| chain/pool_test.go | Updates pool tests for new store/manager initialization APIs. |
| chain/migrate.go | Migrates DB migration logic to operate via the store scratchpad/snapshot helpers. |
| chain/manager.go | Implements MVCC-style snapshot reads and scratchpad writes; updates Store interfaces and manager locking strategy. |
| chain/manager_test.go | Updates existing tests and adds new concurrency/snapshot behavior tests. |
| chain/db.go | Introduces DB snapshot/scratchpad interfaces and implements snapshotting in MemDB/CacheDB/DBStore. |
| chain/db_test.go | Updates DB tests for new APIs and adds snapshot behavior tests for MemDB/CacheDB/BoltDB. |
| chain/chain_test.go | Updates chain tests for new store/manager initialization APIs. |
| .changeset/support_concurrent_chain_manager_reads_via_mvcc_snapshots.md | Documents the breaking interface/API changes and new MVCC behavior. |
Files not reviewed (1)
- internal/cmd/calcswaps/main.go: Generated file
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
peterjan
requested review from
Alrighttt,
ChrisSchinnerl,
chris124567,
n8mgr and
peterjan
peterjan
assigned
lukechampine and unassigned
peterjan,
ChrisSchinnerl,
n8mgr,
chris124567 and
Alrighttt
| s.applyElements(cau) | ||
| } | ||
| s.tip = cs | ||
| if s.shouldFlush() { |
There was a problem hiding this comment.
Why not use flushIfFull in this case? A snapshot reader could briefly see the tip height go down whereas previously tip height was monotonic
There was a problem hiding this comment.
Deletions also contribute to unflushed so I think using flushIfFull isn't enough here. Either we explicitly skip flushing when the resulting height is lower or we prevent delete from incrementing unflushed.
But also this just makes it feel like dbScratchpad is too low level to be able to decide whether flushing is safe.
| "lukechampine.com/frand" | ||
| ) | ||
|
|
||
| // NOTE: due to a bug in the transaction validation code, calculating payouts |
There was a problem hiding this comment.
Removing this comment seems unrelated to this PR?
| @@ -0,0 +1,14 @@ | |||
| --- | |||
| default: minor | |||
There was a problem hiding this comment.
Probably deserves a major since it breaks every single piece of node software we got. e.g. chain.NewManager also has a different signature now without the tipState.
| // is called. Its methods observe the accumulated writes. It is not safe for | ||
| // concurrent use. | ||
| // | ||
| // Unlike a StoreScratchpad, a DBScratchpad must not flush autonomously: |
There was a problem hiding this comment.
(nit) slightly confusing that a DBScratchpad must not flush autonomously but a dbScratchpad flushes all the time because it's not actually a DBScratchpad but a StoreScratchpad
| s.applyElements(cau) | ||
| } | ||
| s.tip = cs | ||
| if s.shouldFlush() { |
There was a problem hiding this comment.
Deletions also contribute to unflushed so I think using flushIfFull isn't enough here. Either we explicitly skip flushing when the resulting height is lower or we prevent delete from incrementing unflushed.
But also this just makes it feel like dbScratchpad is too low level to be able to decide whether flushing is safe.
This supersedes #437, implementing true MVCC snapshots for the Manager. Read-only methods (like
Tip(),BlocksForHistory(), etc.) see a consistent "snapshot" of the Manager state, while state-mutating methods (likeAddBlocks) operate on a "scratchpad," that, once committed, becomes the next snapshot. These are equivalent to BoltDB'sViewandUpdate, respectively.The effect of this is that readers can't block writers, and writers can't block readers. Writers do block other writers -- for example,
AddBlockscalls still need to be serialized -- but that's it.This code is a pretty even split between myself and Claude. I designed the interfaces and refined the APIs to my liking, while Claude handled the mechanical refactoring, the MemDB updates, and writing tests. I suspect there is some slop lurking in here that will turn up in review, but overall I'm happy with the result.