Skip to content

fix: keep Node API responsive during txhashset zip#3890

Open
iho wants to merge 1 commit into
mimblewimble:stagingfrom
iho:fix/txhashset-zip-no-write-lock
Open

fix: keep Node API responsive during txhashset zip#3890
iho wants to merge 1 commit into
mimblewimble:stagingfrom
iho:fix/txhashset-zip-no-write-lock

Conversation

@iho

@iho iho commented Jul 9, 2026

Copy link
Copy Markdown

Summary

Fixes #3550: Node API becomes unresponsive while packaging a txhashset zip for a syncing peer.

Cause

Chain::txhashset_read held write locks on header_pmmr and txhashset for the entire rewind + snapshot + multi-GB zip path. API handlers that need any chain access blocked for the whole packaging duration.

Fix

  1. Write locks only for rewind + snapshot (short, consistency-critical).
  2. Read locks for zip_read (copy + package). Concurrent readers (Node API, headers, etc.) can proceed. Compaction/writers still wait until the zip finishes so backend files are not rewritten mid-package.

Test plan

  • cargo test -p grin_chain --test test_txhashset -- --test-threads=1
  • Manual: request txhashset from a peer while polling foreign API (chain height) every second — requests should continue to succeed during zip packaging

Snapshot under write locks, then package the multi-GB archive under
read locks so Node API and other readers stay responsive while a peer
is served a txhashset zip. Compaction still waits for the read locks.

Addresses mimblewimble#3550.

@wiesche89 wiesche89 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This also needs coordination with #3888, which changes cleanup in the same zip_read path. Concurrent creation and cleanup should share clear serialization and ownership.

Comment thread chain/src/chain.rs
let _header_pmmr = self.header_pmmr.read();
let _txhashset = self.txhashset.read();

let file = txhashset::zip_read(self.db_root.clone(), &header)?;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

zip_read was previously serialized by the write locks. It can now run concurrently for the same temp directory and zip path, so requests may clobber each other or reuse a partial archive. Could we serialize archive creation separately or use unique temp files with an atomic rename?

Comment thread chain/src/chain.rs
/// at the provided block hash.
///
/// Write locks are only held while rewinding/snapshotting. Packaging the
/// multi-GB zip is done under *read* locks so the Node API and other

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The height request mentioned in #3550 goes through Chain::head() and does not take either of these locks. Since the multi-GB copy, zip write, and sync_all() remain unchanged, could we reproduce the original timeout before claiming this fixes it?

Comment thread chain/src/chain.rs
txhashset::zip_read(self.db_root.clone(), &header)
.map(|file| (header.output_mmr_size, header.kernel_mmr_size, file))
})
// Shared locks: allow concurrent chain reads (API, header queries) while

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This only allows concurrent readers until a writer is waiting. With parking_lot::RwLock, later readers then queue behind that writer for the rest of the zip. Could we test the API with a writer waiting during packaging?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants