Skip to content

fix: keep Node API responsive during txhashset zip #3890

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
iho wants to merge 1 commit into
base: staging
Choose a base branch
from
+25 −14
Open

fix: keep Node API responsive during txhashset zip #3890

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 25 additions & 14 deletions chain/src/chain.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -858,25 +858,36 @@ impl Chain {
/// Provides a reading view into the current txhashset state as well as
/// the required indexes for a consumer to rewind to a consistent state
/// 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

@wiesche89 wiesche89 Jul 11, 2026

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?

/// readers remain responsive while a peer is served a txhashset archive
/// (see #3550). Compaction and other writers wait until the zip finishes.
pub fn txhashset_read(&self, h: Hash) -> Result<(u64, u64, File), Error> {
// now we want to rewind the txhashset extension and
// sync a "rewound" copy of the leaf_set files to disk
// so we can send these across as part of the zip file.
// The fast sync client does *not* have the necessary data
// to rewind after receiving the txhashset zip.
// Rewind the txhashset extension and sync a "rewound" copy of the
// leaf_set files to disk so we can send these across as part of the zip.
// The fast sync client does *not* have the necessary data to rewind
// after receiving the txhashset zip.
let header = self.get_block_header(&h)?;

let mut header_pmmr = self.header_pmmr.write();
let mut txhashset = self.txhashset.write();
{
let mut header_pmmr = self.header_pmmr.write();
let mut txhashset = self.txhashset.write();

txhashset::extending_readonly(&mut header_pmmr, &mut txhashset, |ext, batch| {
self.rewind_and_apply_fork(&header, ext, batch)?;
ext.extension.snapshot(batch)?;
txhashset::extending_readonly(&mut header_pmmr, &mut txhashset, |ext, batch| {
self.rewind_and_apply_fork(&header, ext, batch)?;
ext.extension.snapshot(batch)?;
Ok(())
})?;
}

// prepare the zip
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

@wiesche89 wiesche89 Jul 11, 2026

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?

// still preventing compaction from rewriting backend files mid-zip.
let _header_pmmr = self.header_pmmr.read();
let _txhashset = self.txhashset.read();

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

@wiesche89 wiesche89 Jul 11, 2026

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?

Ok((header.output_mmr_size, header.kernel_mmr_size, file))
}

/// The segmenter is responsible for generation PIBD segments.
Expand Down
Loading