Read concurrency

[Alrighttt]

Changes Manager.mu to sync.RWMutex to enable reads to run in parallel.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

_{Tip: disable this comment in your organization's Code Review settings.}

[Alrighttt]

As discussed with Nate and Luke, I will be redesigning or removing the peer cancellation logic. Don't bother with review for now.

[Alrighttt]

I removed all code related to the peer cancellation mechanism.

[lukechampine]

this makes me sad

maybe we could add a writeable flag on Bucket() instead?

[ChrisSchinnerl]

Not sure you'd like that more but if you want to be able to do multiple operations within a single View we could also do something like

snap := m.store.Snapshot()
defer snap.Close()
for range 10 {
  _, _ = snap.Block(...)
}

Where Snapshot starts a readonly transaction which returns a ReadonlyDB interface only containing a subset of the DB interface. So it's impossible to call CreateBucket in the first place.

[Alrighttt]

Implemented Chris's suggestion

[lukechampine]

I do like the idea of making it impossible to call mutating methods, but to make it airtight, we need a read-only DBBucket as well that has no Put or Delete methods.

[Alrighttt]

Added this in 38d9717

[peterjan]

This should probably be minor considering we're extending the exported Store and DB interfaces? Anyone implementing those will have to add Snapshot for it to compile.

[chris124567]

LGTM besides PJ's comments

[lukechampine]

this is not as clean as I'd hoped. I experimented with some alternative designs, but they didn't come out much better.

I think the issue is that RWMutex and BoltDB have different semantics. RWMutex doesn't give you a snapshot; it just blocks until the writer is finished. So even if we have this nice SnapshotDB that can serve requests even while the DB is being written to, we don't get to actually use it, because it's gated behind an RLock(), which can't complete until Unlock() is called.

And yet, IIRC @Alrighttt said that, empirically, this change eliminated the issue. So I'm confused. I can understand how, if you have a bunch of concurrent readers, this design would no longer force them to be serialized. But that property would seem to break as soon as you have 1 writer in the mix (e.g. a malicious peer submitting txns to your pool, or drip-feeding you sidechain blocks).

Clearly, snapshot semantics are better than RWMutex, because you can continue serving requests even while writing. So maybe we should aim for snapshots of the whole Manager state? IIUC this would require making a deep copy of the transaction pool, but only when we start a write transaction, which doesn't seem too bad.