Repository navigation
Conversation
|
Interesting approach - doesn't this require the destination host to have twice the storage of the final datadir though? I think to actually land this feature, need a way for the receiving node to import the store without needing significantly more storage than the final store size. Maybe even a v2 protocol extension that lets an rbitcoin node serve structured hot data to another rbitcoin node for quick sync or something. Open to other ideas too, or to you contradicting me. |
|
(rebase to get green coverage) |
|
There are trade-offs with any approach. This approach provides atomic changes, needs ~2x disk space. The in-place approach would require decode all things that encrypted with the original I can can do one or another, or both :) |
|
Will continue thinking about what I think the most rbitcoiny thing here is. Thank you! Won't merge this feature until after 0.9 sometime in the next week. |
There was a problem hiding this comment.
REQUEST_CHANGES. 2 bugs, 2 nits.
- Connect-time consensus is not re-run, so a store's own
--hot-digeststill imports an invalid chain. Seeimport_hot/hot_import_check_block. - Opening the source can rewrite that datadir. See
import_hot_store.
Nits: a later start that still passes --assumehot* exits because store/ is not empty (the operator note already says to drop the flags). assumehot_import_then_follow extends with an empty block, so it never spends a coin that exists only in the imported store.
The extra disk copy is already the open question on the thread.
| hash: *hash, | ||
| }); | ||
| } | ||
| self.confirm_blocks_run(&prepared)?; |
There was a problem hiding this comment.
import_hot stops at confirm_blocks_run plus annotate_spends_for_confirmed. Connect's structural phase never runs, so a second spend of one output is stored as a multi-spender, and an immature coinbase, a BIP30 overwrite, a BIP68 failure, or a coinbase above subsidy plus fees is kept. hot_import_check_block calls ValidationContext::archive_structure, so BIP34 and the pre-segwit witness rule are skipped too. --hot-digest hashes the store as it sits, so that store's own digest matches. The new node then serves the tip with no background validation and no RPC warning. Run those structural checks during the import (scripts can stay off) and cover a bad store whose expected digest is its own --hot-digest.
| seal.tip_height | ||
| ))); | ||
| } | ||
| let src = Query::open_or_create_layout(src)?; |
There was a problem hiding this comment.
Query::open_or_create_layout revalidates the tip window and can shrink the source and rewrite tip_seal before the match check on the next lines. HeaderHead::open also rewrites header.head when the file has fewer slots than the open target. assumehot_import builds that target from the destination layout (..dst.clone()), so the destination --head-scale and RBITCOIN_HEAD_SLOTS_HEADER apply to the source. A refused import, or a source whose header head is smaller than that target, changes the datadir being copied. Open the source without repair and without a head rewrite.
There was a problem hiding this comment.
COMMENT. Both bugs from a8b21fb are fixed. Rebase onto current master: the coverage job failed on this head, and a rebase is what cleared it last time.
Fixed: import_hot connects through confirm_wire_run (genesis through accept_and_connect_block) under a height-only milestone, so a bad store's own digest is refused for an immature coinbase, a double spend, a coinbase above subsidy, and a missing BIP34 push. The source opens through open_import_source / HeaderHead::open_source; a tiny header.head stays at its own slots, and a tip past tip_seal is refused without being shrunk.
Nits still open: a later start that still passes --assumehot* exits because store/ is not empty. assumehot_import_then_follow still extends with an empty block, so it never spends an imported coin.
| if dst.dir.exists() { | ||
| let mut entries = std::fs::read_dir(&dst.dir).map_err(|e| StoreError::io(&dst.dir, e))?; | ||
| if entries.next().is_some() { | ||
| return Err(HotImportError::NotEmpty); |
There was a problem hiding this comment.
Nit: a second start that still passes --assumehot* hits this and exits. The imported store is already the one just written. Ignore the flags when store/ is non-empty and its digest matches, and fail only on a mismatch.
There was a problem hiding this comment.
Fixed in b559984, but not by re-checking the digest. Once the node follows a block, the store's digest changes, because it covers spenders as of the tip. Recomputing it is also a full walk on every restart.
Instead, the import writes store/assumehot (height, tip hash, digest) into store.import/ before the rename, so it lands with the store. On start, kept_import reads it before the source lock is taken:
- same digest: log "already imported; flags ignored" and open the store, also past the imported tip and with the source deleted;
- another digest: refused, naming both digests;
- no record but
store/is not empty: refused as before.
Covered by assumehot_restart_finds_its_own_import, and by the journey restarting with the flags after A's datadir is deleted.
| assert!(!b.join("store.import").exists()); | ||
|
|
||
| let last = blocks.last().unwrap(); | ||
| let missed = mine_regtest_block(last.block_hash(), last.header.time + 1, height + 1, vec![]); |
There was a problem hiding this comment.
Nit: this block has no inputs, and extend_one does not spend an imported output either. The follow only shows that an empty block connects. Spend a coin that exists only because the import recomputed it.
There was a problem hiding this comment.
Fixed in b559984. The block B missed now spends script B's coin (the fixture's 0x52 output, which B has only from the import) to 0x53. B fetches it from the miner and connects it, and the test checks that the 0x53 history holds the spend. extend_one then follows one more block as before.
e8dce56 to
b559984
Compare
3d96ff7 to
e2aa545
Compare
coreyphillips
left a comment
There was a problem hiding this comment.
Two independent reviews.
needs changing before merge
- Migrated stores produce digests that their imports cannot match (crates/rbitcoin-query/src/hot_digest.rs:210-217)
worth doing, does not block
- Import and digest walk print no progress, and their mainnet cost is unknown (crates/rbitcoin-query/src/hot_digest.rs:126-200)
- Import always builds the scripthash index, whatever --sh-index says (crates/rbitcoin-query/src/hot_digest.rs:194)
- The documented --hot-digest then --assumehot flow, and the tip-seal no-op, are untested (crates/rbitcoin-store/src/integrity.rs:199-206)
- Record and rename are not followed by a parent directory fsync (crates/rbitcoin-query/src/hot_digest.rs:323-334)
nits
- Docs suggest reading the source in place, but it must be writable (docs/operator/storage.md:150)
the reviewers disagree, your call
- Opening an import source can truncate its archive (crates/rbitcoin-store/src/store.rs:476)
- Staging cleanup deletes sources nested inside the staging directory (crates/rbitcoin-query/src/hot_digest.rs:399-404)
- Imports expose frozen scripthash answers when indexing is disabled (crates/rbitcoin-query/src/hot_digest.rs:412-416)
| pub fn hot_digest(&self) -> Result<Option<HotDigest>, QueryError> { | ||
| let Some(tip) = self.tip_height() else { | ||
| return Ok(None); | ||
| }; | ||
| let mut eng = sha256::Hash::engine(); | ||
| eng.input(DOMAIN); | ||
| let mut tip_hash = [0u8; 32]; | ||
| for h in 0..=tip.0 { | ||
| let block = self.reconstruct_block_at_height(Height(h))?; | ||
| tip_hash = block.block_hash().to_byte_array(); | ||
| let wire = bitcoin::consensus::encode::serialize(&block); | ||
| eng.input(&h.to_le_bytes()); | ||
| eng.input(&tip_hash); | ||
| eng.input(&(wire.len() as u64).to_le_bytes()); | ||
| eng.input(&wire); | ||
| for (fk, tx) in self.block_tx_fks(Height(h))?.into_iter().zip(&block.txdata) { | ||
| self.hot_digest_tx(&mut eng, fk, tx.output.len() as u32, tip.0)?; | ||
| } | ||
| } | ||
| Ok(Some(HotDigest { | ||
| height: tip.0, | ||
| tip_hash, | ||
| digest: sha256::Hash::from_engine(eng).to_byte_array(), | ||
| })) | ||
| } | ||
|
|
||
| /// Rewrite every confirmed height of `src` into this empty store, then | ||
| /// refuse unless this store's [`Self::hot_digest`] equals `expected`. | ||
| pub fn import_hot( | ||
| &self, | ||
| src: &Query, | ||
| params: &ChainParams, | ||
| expected: [u8; 32], | ||
| ) -> Result<(), HotImportError> { | ||
| if self.tip_height().is_some() { | ||
| return Err(HotImportError::NotEmpty); | ||
| } | ||
| let Some(tip) = src.tip_height() else { | ||
| return Err(StoreError::NotFound.into()); | ||
| }; | ||
| // Scripts are skipped through the tip, as with `--milestone` there; | ||
| // every other connect check runs. | ||
| let milestone = rbitcoin_consensus::Milestone::height(tip.0); | ||
| let refused = |heights: std::ops::RangeInclusive<u32>, e: ConfirmError| match e { | ||
| ConfirmError::Store(e) => HotImportError::Store(e), | ||
| ConfirmError::Rules(e) => HotImportError::Invalid { | ||
| heights, | ||
| reason: e.to_string(), | ||
| }, | ||
| }; | ||
| for run_start in (0..=tip.0).step_by(IMPORT_RUN as usize) { | ||
| let run_end = run_start.saturating_add(IMPORT_RUN - 1).min(tip.0); | ||
| let mut run = Vec::with_capacity((run_end - run_start + 1) as usize); | ||
| for h in run_start..=run_end { | ||
| let block = src.reconstruct_block_at_height(Height(h))?; | ||
| hot_import_check_block(src, params, milestone, Height(h), &block)?; | ||
| if h == 0 { | ||
| // A run connects onto a parent; genesis has none. | ||
| crate::accept_and_connect_block(self, params, Height(0), &block, milestone) | ||
| .map_err(|e| refused(0..=0, e))?; | ||
| } else { | ||
| run.push((Height(h), block)); | ||
| } | ||
| } | ||
| if let Some(&(first, _)) = run.first() { | ||
| crate::confirm_wire_run(self, params, milestone, &run) | ||
| .map_err(|e| refused(first.0..=run_end, e))?; | ||
| } | ||
| self.apply_sh_pending()?; | ||
| } | ||
| let got = self.hot_digest()?.map(|d| d.digest).unwrap_or_default(); | ||
| if got != expected { | ||
| return Err(HotImportError::DigestMismatch { expected, got }); | ||
| } | ||
| Ok(()) |
There was a problem hiding this comment.
worth doing, does not block: Import and digest walk print no progress, and their mainnet cost is unknown
An import on a real chain runs for hours and prints nothing until it ends. Query::hot_digest (hot_digest.rs:126) is one thread. For every output it calls spenders_create. For every spender it calls is_confirmed_strong_at and then hot_digest_position, which reads confirmed and header_txs. That is several random reads per output, over roughly 3 billion mainnet outputs. The walk runs once for --hot-digest on the source. It runs again at the end of import_hot (line 196), after a full connect of every block in 64-height runs. Neither import_hot nor hot_digest logs anything. assumehot_import in run.rs logs only after success. On mainnet the operator sees a node that has said nothing for hours and cannot tell a slow import from a hung one. The PR lists production scale as not covered. I read the code and did not measure it.
There was a problem hiding this comment.
Fixed in f48d5c1. The import registers a /progress stage, assumehot import, counted in heights. The digest walk registers its own, hot digest. Both log an INFO line every 10 s with the height reached and the time elapsed. run.rs also logs when the import starts. The health listener starts before run_node, so GET /progress works during the import. Test: assumehot_import_reports_progress. I have not measured mainnet cost.
There was a problem hiding this comment.
The /progress part is answered on the storage.md:152 thread. Mainnet cost is still unmeasured: that needs an operator-host run, and I have not done one.
| crate::confirm_wire_run(self, params, milestone, &run) | ||
| .map_err(|e| refused(first.0..=run_end, e))?; | ||
| } | ||
| self.apply_sh_pending()?; |
There was a problem hiding this comment.
worth doing, does not block: Import always builds the scripthash index, whatever --sh-index says
The staging store gets a full scripthash index even when the destination runs with --sh-index off, which is the default. The staging Query from open_or_create_layout keeps the library defaults: index mode Tip and sh_index_enabled = true. So enqueues_sh_writebehind() is true for every block that confirm_wire_run connects, and import_hot drains that work with apply_sh_pending() after each run (line 194). The node reads config.shindex only later, in apply_startup_index_mode (run.rs:1977), after the import has finished. This costs import time and disk the operator did not ask for. It also breaks the doc's claim that the destination needs about the size of the source store/: a source run without --sh-index produces a larger destination. One fix is to pass shindex into import_hot_store and call set_sh_index_enabled on the staging query.
There was a problem hiding this comment.
Fixed in f48d5c1. import_hot_store takes the destination's --sh-index and calls set_sh_index_enabled on the staging query. With it off, no scripthash index is built. Test: assumehot_import_follows_the_destinations_sh_index runs both settings and checks recover_sh_indexed_through. storage.md now says the destination is smaller when the source has an index and this node does not.
| let prev = TipSeal::load(self.path())?; | ||
| let confirmed_len = u64::from(tip.0) + 1; | ||
| // Open republishes after its tip-window check. An unchanged tip writes | ||
| // nothing, so opening a clean store leaves its files as they were. | ||
| if let Some(p) = &prev { | ||
| if (p.tip_height, p.tip_hash, p.confirmed_len) == (tip.0, rec.hash, confirmed_len) { | ||
| return Ok(()); | ||
| } |
There was a problem hiding this comment.
worth doing, does not block: The documented --hot-digest then --assumehot flow, and the tip-seal no-op, are untested
The usage in storage.md is: stop the source, run --hot-digest on it, then import with that hash. No test runs that sequence. The journey computes the source digest with Query::open_or_create_tiny, and pin_assumehot takes it from the live handle before shutdown. --hot-digest opens the source through run_node, which runs the start-up repairs. The new early return in publish_tip_seal (integrity.rs:203) seems to exist so that this open writes nothing. I found no test in the diff that fails without that early return, because import_hot_store never republishes the seal on the source. This is a change to production store code with no test that pins it. I did not build the tree with it reverted. A scenario that runs --hot-digest on a stopped datadir, compares the source tree bytes before and after, and then imports with the printed hash would cover both.
There was a problem hiding this comment.
Fixed in f48d5c1. node_cli_and_surface_smoke now runs the operator binary (journey-node) with --hot-digest on the stopped source. It checks that store/ is byte-identical afterwards, that the printed line matches, and then imports with the printed hash. Without the early return in publish_tip_seal, the test fails on the byte check.
| fn write_import_record(dir: &Path, d: &HotDigest) -> Result<(), StoreError> { | ||
| use std::io::Write; | ||
| let p = dir.join(IMPORTED_NAME); | ||
| let mut buf = [0u8; IMPORTED_BYTES]; | ||
| buf[..8].copy_from_slice(IMPORTED_MAGIC); | ||
| buf[8..12].copy_from_slice(&d.height.to_le_bytes()); | ||
| buf[12..44].copy_from_slice(&d.tip_hash); | ||
| buf[44..76].copy_from_slice(&d.digest); | ||
| let mut f = std::fs::File::create(&p).map_err(|e| StoreError::io(&p, e))?; | ||
| f.write_all(&buf).map_err(|e| StoreError::io(&p, e))?; | ||
| f.sync_all().map_err(|e| StoreError::io(&p, e)) | ||
| } |
There was a problem hiding this comment.
worth doing, does not block: Record and rename are not followed by a parent directory fsync
write_import_record fsyncs the file but not store.import/. The rename at line 430 is not followed by an fsync of the datadir. If power is lost after the rename, the restart can find store/ holding data but no assumehot record. Restarting with the same flags then fails with "destination store is not empty". TipSeal::store already calls rbitcoin_store::file::fsync_parent_dir for the same reason.
There was a problem hiding this comment.
Fixed in f48d5c1. The record is written with file::write_synced_tmp_rename, which also fsyncs store.import/. After the store.import/ → store/ rename, the datadir is fsynced with fsync_parent_dir, as TipSeal::store does. There is no test for the fsync itself.
| state, addressed by height. It does not depend on `store.secret`, the schema | ||
| the store started on, orphans, or reorg history, so two honest nodes on the | ||
| same tip print the same digest. Copy the source datadir to the new host, or | ||
| read it in place. |
There was a problem hiding this comment.
nit: Docs suggest reading the source in place, but it must be writable
storage.md says to copy the source datadir or read it in place, and that the import never writes to the source. In fact lock_dir(src) creates .lock in the source datadir, and Store::open_source opens the tables read-write (the comment in pin_assumehot says so). A read-only mount or snapshot of a stopped datadir is therefore refused. The doc should say the source must be writable.
There was a problem hiding this comment.
Fixed in f48d5c1. storage.md now says the source datadir must be writable, because the import takes its .lock and opens its tables read-write, so a read-only mount or snapshot is refused.
| write_seqsigwit_reloc(&path)?; | ||
| } | ||
| let cold_path = layout.is_split().then_some(seqsigwit_dir); | ||
| let (txs, cold_path) = open_layout_txs(&layout)?; |
There was a problem hiding this comment.
the reviewers disagree, your call: Opening an import source can truncate its archive
A refused import can truncate an incomplete source copy. open_layout_txs still invokes the repairing TxTable::open_seqsigwit. I confirmed this by removing txid.body from a clean height-5 source: import failed after resetting the source's Class A counts to zero and replacing tx.head.
Objection: Real, but non-blocking rather than blocking: Store::open_source still goes through open_layout_txs to TxTable::open_seqsigwit. That call creates a missing txid.body, truncates every Class A stem to the shortest count in repair_class_a_count_skew, then wipes and recreates tx.head. refuse_unopenable_source only checks scripthash.body and header_txs_first.body, so this contradicts the changelog's "never writes to the source". It is non-blocking because it only affects a source that is already incomplete, and a normal open would apply the same repair.
There was a problem hiding this comment.
Agreed, fixed in f48d5c1. I removed each file of a closed store in turn. Ten of them were recreated or rebuilt by the open, including txid.body, the txstat files and tx.head. Store::open_source now refuses a source missing any of them, before anything is opened. Test: open_source_refuses_another_schema_and_any_missing_file removes every top-level file in turn and checks that the open fails and the tree is unchanged, so a new table can't be missed. One case remains: a copy cut short inside a file, with every file present but at different lengths, is still trimmed by the count-skew repair, as any open trims it. storage.md says so.
There was a problem hiding this comment.
Fixed in aa128b1 with rearden-grok's comment on store.rs:2121: a source whose tables have different lengths is refused, not trimmed, and the refused import leaves it byte-identical.
| let mut name = dst.dir.file_name().unwrap_or_default().to_os_string(); | ||
| name.push(".import"); | ||
| let staging = dst.dir.with_file_name(name); | ||
| if staging.exists() { | ||
| // Left by an import that did not finish; never renamed, so never used. | ||
| std::fs::remove_dir_all(&staging).map_err(|e| StoreError::io(&staging, e))?; |
There was a problem hiding this comment.
the reviewers disagree, your call: Staging cleanup deletes sources nested inside the staging directory
Staging cleanup can delete the source before its digest is checked. With --datadir NEW --assumehot-from NEW/store.import, the source store sits inside the directory removed here. I reproduced this layout with a wrong digest: import returned DigestMismatch, and neither the source nor the destination store remained.
Objection: Real, but non-blocking rather than blocking: staging comes only from dst (<datadir>/store.import), and neither run.rs nor import_hot_store compares it with src. With --assumehot-from NEW/store.import, line 404 deletes the already-open source before the import runs, and line 423 deletes the rebuilt store on a mismatch. The source has to sit at the reserved staging name, so this is non-blocking, but the loss is irreversible and a path check before line 402 would close it.
There was a problem hiding this comment.
Agreed, fixed in f48d5c1. It was worse with the right digest: the import reported success after deleting its own source, because the open file handles stayed readable. The import now canonicalizes both paths and refuses when the source store is inside the destination datadir, or the destination datadir is inside the source. The check runs before anything is removed. New error: "the source and destination datadirs overlap". Test: assumehot_import_refuses_overlapping_datadirs checks both directions and that the source is unchanged.
| match self.store.txstat_row(fk)? { | ||
| None => eng.input(&[0]), | ||
| Some(r) => { | ||
| eng.input(&[1]); | ||
| eng.input(&r.fee_sat.to_le_bytes()); | ||
| eng.input(&r.base.to_le_bytes()); | ||
| eng.input(&r.wit_extra.to_le_bytes()); | ||
| } |
There was a problem hiding this comment.
needs changing before merge: Migrated stores produce digests that their imports cannot match
A valid store migrated from schema 24 fails import using its own digest. Migration leaves historical txstat rows unstamped, which this function hashes as absent, while import recomputes and stamps them. I confirmed DigestMismatch with a migrated height-5 source that passed the source checks.
There was a problem hiding this comment.
Fixed in f48d5c1. When a txstat row is unstamped, the digest now hashes the row connect would stamp. It computes the row from the block it already reads and writes nothing. This also covers getblockstats, which stamps old rows one block at a time, so before this fix the digest depended on which blocks had been queried. Test: hot_digest_ignores_unstamped_txstat migrates a store from schema 24, checks its digest before and after stamping, and imports it with that digest.
| let q = Query::open_or_create_layout(StoreLayout { | ||
| dir: staging.clone(), | ||
| ..dst.clone() | ||
| })?; | ||
| q.import_hot(&src, params, expected)?; |
There was a problem hiding this comment.
the reviewers disagree, your call: Imports expose frozen scripthash answers when indexing is disabled
Importing with scripthash indexing disabled makes Electrum serve frozen wallet data. The staging query defaults indexing on and writes a watermark; startup disables further indexing, but that watermark keeps scripthash methods available. I confirmed through run_node, startup configuration, and Electrum dispatch that after connecting block 103, a spent imported output still reported a 49 BTC balance and the new output's history was empty.
Objection: Real, but non-blocking rather than blocking: The staging Query defaults to sh_index_enabled true and IndexMode::Tip (lib.rs:682, lib.rs:691), so the import enqueues scripthash jobs and apply_sh_pending builds a durable index. On the next start, recover_sh_indexed_through returns Some(tip) via has_durable_index, so sh_history_available() stays true with shindex off and serves answers frozen at the import height instead of failing closed. Restarting without --sh-index after indexing already behaves this way, but here the operator never opted in. It also needs an Electrum or Esplora listener without --sh-index, so non-blocking. Applying the node's shindex setting to the staging query would fix it.
There was a problem hiding this comment.
Agreed, fixed in f48d5c1 with the --sh-index fix above. With --sh-index off, the staging store builds no index and keeps no watermark, so recover_sh_indexed_through returns None and scripthash methods are not served from the import.
|
Thanks for the review. All nine are addressed in f48d5c1. |
coreyphillips
left a comment
There was a problem hiding this comment.
Rechecked after the branch was rewritten, so every earlier point was verified again from scratch. 1 blocking finding the reviewers disagree about.
addressed
- Import always builds the scripthash index, whatever --sh-index says (crates/rbitcoin-query/src/hot_digest.rs:194). Fixed:
import_hot_storecallsq.set_sh_index_enabled(shindex)on the staging query (hot_digest.rs:512), and run.rs:350 passesconfig.shindex.enqueues_sh_writebehindissh_index && is_tip, so with the flag off nothing is queued.assumehot_import_follows_the_destinations_sh_indexpasses for both settings. - The documented --hot-digest then --assumehot flow, and the tip-seal no-op, are untested (crates/rbitcoin-store/src/integrity.rs:199-206). Fixed:
pin_assumehotrunsjourney-node --hot-digeston the stopped source, comparesstore/bytes, then imports with the printed hash. It passes on the branch. I also removed the early return inpublish_tip_sealin a /tmp copy: the test then fails at the byte check, so it does pin the no-op. - Record and rename are not followed by a parent directory fsync (crates/rbitcoin-query/src/hot_digest.rs:323-334). Fixed:
write_import_recordnow usesfile::write_synced_tmp_rename, which fsyncs the file and thenstore.import/(file.rs:91-105). After the rename,fsync_parent_dir(root)opens and syncs the datadir itself (hot_digest.rs:530-531). There is no test, as you say. - Docs suggest reading the source in place, but it must be writable (docs/operator/storage.md:150). Fixed: storage.md:155-157 now says the source datadir must be writable because the import takes its
.lockand opens its tables read-write, so a read-only mount or snapshot is refused. - Staging cleanup deletes sources nested inside the staging directory (crates/rbitcoin-query/src/hot_digest.rs:399-404). Fixed:
refuse_destination(hot_digest.rs:403-412) canonicalizes both paths and returnsOverlapbefore staging is removed.assumehot_import_refuses_overlapping_datadirspasses for theNEW/store.import/storesource and leaves it unchanged. A narrower gap in the other direction is a new finding. - Migrated stores produce digests that their imports cannot match (crates/rbitcoin-query/src/hot_digest.rs:210-217). Fixed:
hot_digest_txstat(hot_digest.rs:257-276) hashes the row connect would stamp when a cell is unstamped. It uses the sametxstat_in_sum_from_blockandtxstat_row_from_txhelpers.hot_digest_ignores_unstamped_txstatpasses: it migrates from schema 24, checks the digest is unchanged before and after stamping, and imports with it. - Imports expose frozen scripthash answers when indexing is disabled (crates/rbitcoin-query/src/hot_digest.rs:412-416). Fixed: This is answered by the same
set_sh_index_enabled(shindex)call. With--sh-indexoff, the staging store queues no scripthash jobs and leaves no durable index.assumehot_import_follows_the_destinations_sh_indexassertsrecover_sh_indexed_through()isNonefor shindex=false, and it passes.
still open
- Import and digest walk print no progress, and their mainnet cost is unknown (crates/rbitcoin-query/src/hot_digest.rs:126-200). An import on a real chain runs for hours and prints nothing until it ends.
Query::hot_digest(hot_digest.rs:126) is one thread. For every output it callsspenders_create. For every spender it callsis_confirmed_strong_atand thenhot_digest_position, which readsconfirmedandheader_txs. That is several random reads per output, over roughly 3 billion mainnet outputs. The walk runs once for--hot-digeston the source. It runs again at the end ofimport_hot(line 196), after a full connect of every block in 64-height runs. Neitherimport_hotnorhot_digestlogs anything.assumehot_importin run.rs logs only after success. On mainnet the operator sees a node that has said nothing for hours and cannot tell a slow import from a hung one. The PR lists production scale as not covered. I read the code and did not measure it. Codex still sees it: Progress stages and timed logs exist atcrates/rbitcoin-query/src/hot_digest.rs:30-50, and the progress test passes, but mainnet cost remains unmeasured.crates/rbitcoin-node/src/cli.rs:312-313bypasses the health listener, so standalone--hot-digeststill cannot expose the promised/progressendpoint. - Opening an import source can truncate its archive (crates/rbitcoin-store/src/store.rs:476). A refused import can truncate an incomplete source copy.
open_layout_txsstill invokes the repairingTxTable::open_seqsigwit. I confirmed this by removingtxid.bodyfrom a clean height-5 source: import failed after resetting the source's Class A counts to zero and replacingtx.head. Codex still sees it: The missing-file guard still reaches repairingTxTable::open_seqsigwitthroughcrates/rbitcoin-store/src/store.rs:2121. I reproduced a refused height-5 import with every file present and a zero-counttxid.body: it truncated other archive tables and rebuilttx.head.
new since the last review
worth doing, does not block
- --hot-digest is documented to show on GET /progress but starts no listener (docs/operator/storage.md:152)
- Overlap check accepts a destination datadir inside the source datadir (crates/rbitcoin-query/src/hot_digest.rs:403-412)
| connect would stamp it), orphans, or reorg history, so two honest nodes on the | ||
| same tip print the same digest. Printing it opens the store as a node does but | ||
| writes nothing to it. On a large chain the walk takes hours; it logs progress | ||
| every 10 seconds and shows on `GET /progress` as `hot digest`. |
There was a problem hiding this comment.
worth doing, does not block: --hot-digest is documented to show on GET /progress but starts no listener
The --hot-digest walk never appears on GET /progress, although storage.md:152 and the changelog say it does. node_cli_main returns into hot_digest_main (cli.rs), which calls only run_node. run_health is started only in run_p2p (run.rs:367), so a --health-listen passed with --hot-digest is ignored. The hot digest stage is registered, but nothing serves it. Only the INFO lines every 10 s on stderr remain, and those are hidden at --log-level error or warn. Relatedly, run.rs:347 logs "progress every 10s and on GET /progress" on every import, including under --smoke or without --health-listen, where no endpoint exists. I confirmed this by reading the call path. Fix either the docs or the log line, or start the health listener in hot_digest_main.
There was a problem hiding this comment.
Fixed in aa128b1 by narrowing the claims. storage.md now says --hot-digest logs progress every 10 s and serves no GET /progress (--health-listen is ignored with it). It says the import shows on GET /progress only with --health-listen. The changelog says the same, and the run.rs line is now assumehot: importing … (progress every 10s). I did not add a listener to --hot-digest, which exits when its walk ends.
| fn refuse_destination(src: &Path, dst: &StoreLayout) -> Result<(), HotImportError> { | ||
| if dst.cold_dir.is_some() { | ||
| return Err(HotImportError::ColdDestination); | ||
| } | ||
| let root = dst.dir.parent().unwrap_or(&dst.dir); | ||
| let canon = |p: &Path| std::fs::canonicalize(p).map_err(|e| StoreError::io(p, e)); | ||
| let (src_dir, dst_root) = (canon(src)?, canon(root)?); | ||
| if src_dir.starts_with(&dst_root) || dst_root.starts_with(&src_dir) { | ||
| return Err(HotImportError::Overlap); | ||
| } |
There was a problem hiding this comment.
worth doing, does not block: Overlap check accepts a destination datadir inside the source datadir
refuse_destination compares the source store (SRC/store) with the destination datadir. So --datadir SRC/new --assumehot-from SRC is accepted, but storage.md:156 and :170 and the changelog say neither datadir may contain the other. I confirmed it with a scratch test in a /tmp copy: import_hot_store(SRC/store, StoreLayout::tiny(SRC/new/store), ..) returned Ok(5) and wrote SRC/new/store/assumehot. The import itself is safe. But storage.md:202 tells the operator the source can be deleted once the import is done, and rm -rf SRC then deletes the new node. Compare the source datadir (src.parent(), or pass the datadir in) with the destination datadir, or narrow the docs to what the check enforces.
There was a problem hiding this comment.
Fixed in aa128b1. import_hot_store now takes the source datadir, as --assumehot-from does, and opens <src>/store itself. refuse_destination compares datadir with datadir, so --datadir SRC/new --assumehot-from SRC is refused with "the source and destination datadirs overlap". assumehot_import_refuses_overlapping_datadirs covers that case, which returned Ok(5) before the fix, and checks that nothing is written under SRC.
There was a problem hiding this comment.
REQUEST_CHANGES. 1 bug, 1 suggestion.
The two bugs from a199523 stay fixed. import_hot still connects through confirm_wire_run and accept_and_connect_block under a height-only milestone, and assumehot_import_refuses_a_store_that_breaks_connect_rules still refuses an immature coinbase, a double spend, a coinbase above subsidy, and a missing BIP34 push. The source still opens through open_import_source / HeaderHead::open_source: a tiny header.head keeps its slots, and a tip past tip_seal is refused without being shrunk.
Both nits are fixed. kept_import treats a later --assumehot as already done when store/assumehot names that digest, including after the tip has moved and the source is gone, and ImportedOther refuses a different digest. assumehot_import_then_follow spends script B's coin from the imported block and checks that Electrum history.
--hot-digest returns from hot_digest_main before run_p2p, and health listen defaults to off. The documented commands never pass --health-listen, so neither walk is on GET /progress. The 10s logs and the in-process stages are real (assumehot_import_reports_progress). Serve /progress on those commands, or stop claiming the endpoint.
The test job is cargo test (no nextest timings). Wall is 180s, against 170s on master 118dafdb and 159s on a199523. Sequential suite time is 112.8s vs 102.1s on that master run. The new tests are the small part: query 4.71s→6.08s (438→449 tests; the hot_digest tests finish in the first ~2s of that suite), store 2.22s→2.66s (one new test), rbitcoin-test 23.78s→24.67s. That last wall is outbound_full_relay_refills_to_max_outbound on both runs; assumehot_import_then_follow is done about 6.5s after the suite starts. The other ~8s is rbitcoin-net 36.65s→42.12s and rbitcoin-ibd 16.91s→19.09s, same test counts, on a branch whose parent is 49210ea. No new test is slow enough to block on its own.
The merge onto current master is clean. Store repairs added since 49210ea stay on the normal open.
| if layout.is_split() { | ||
| refuse_cold_index_dirs(path, &seqsigwit_dir)?; | ||
| } | ||
| let txs = TxTable::open_seqsigwit(path, &seqsigwit_dir, layout.open_opts())?; |
There was a problem hiding this comment.
open_source reaches this TxTable::open_seqsigwit, which still calls repair_class_a_count_skew. A file that is present but short (a zero-count txid.body) sets the target to the minimum count, and class_a_skew_apply_truncate cuts the other Class A stems to match, before the digest check, with nothing put back if the import then refuses. refuse_unopenable_source only checks that the names exist. Refuse that skew on the source path instead of trimming it. import_hot_store and the changelog say a refused import leaves the source as it was; storage.md already says this copy is trimmed.
There was a problem hiding this comment.
Fixed in aa128b1. open_layout_txs now opens a source with TxTable::open_seqsigwit(.., repair: false), which refuses where a node's open repairs: Class A count skew, a txstat of another length, and a tx.head that is unreadable, empty, or ahead of or behind the bodies. A zero-count txid.body is refused and nothing is trimmed.
I probed by cutting files, which found two more writes, the txstat alignment and the tx.head rebuild. It also found one outside the tx table: ScriptHashHead::from_file rewrote ingest.occ on every open of a head of 16 MiB or less, but it reads that sidecar only for larger heads, so that write is removed.
open_source_refuses_another_schema_and_never_writes removes each top-level entry. It cuts every file in the tree to its header and by one byte, and swaps each top-level entry with the same store's copy at 0 or 3 transactions. After each open_source it checks the tree is byte-identical. It failed first on input.loc. I reverted each refusal by hand, and the test caught each one. storage.md and the changelog now say such a copy is refused.
|
Addressed in aa128b1:
Mainnet cost is still not measured. |
…ehot) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Fixes #590
Start a node from another node's store instead of running IBD.
Usage
What it does
store.secret, orphans or reorg history.run_node, before the store opens, while holding the source's datadir lock.--datadir-colddestination;--datadir-cold, on an old schema, not cleanly stopped, or pruned.store.import/and renames it tostore/only if the digest matches.Trust
The digest names the chain, and you trust whoever gave it to you. There is no background validation, unlike Core's assumeutxo.
Tests
rbitcoin-queryhot_digestsuite: the digest, the import, each refusal, and tampered blocks and headers.node_cli_and_surface_smoke: CLI exit codes, wrong hash, locked source, import.end_of_ibd::assumehot_import_then_follow: node B imports node A's store with no peer, serves Electrum history, then syncs and follows over P2P.Not covered
--sh-indexoff.