Use REST spenttxouts for block indexing, plus consistency fixes and RocksDB multi-CF - #217
Use REST spenttxouts for block indexing, plus consistency fixes and RocksDB multi-CF#217shesek wants to merge 20 commits into
Conversation
shesek
changed the title
02b2052 to
39d3ca3
Compare
|
Shoutout and a big thanks to @romanz that made this possible! 🙌 |
| "REST spenttxouts failed for {} (is bitcoind running with -rest=1?): {}", | ||
| blockhash, e | ||
| )) | ||
| })?; |
There was a problem hiding this comment.
Should we have retries here like above in pub getblock() (above starting line 581)?
| return Ok(()); | ||
| } | ||
|
|
||
| #[cfg(not(feature = "liquid"))] |
There was a problem hiding this comment.
--no-spenttxouts is still a supported Bitcoin mode, but the non-Liquid test harness now always sets use_spenttxouts: true (tests/common.rs line 125). That leaves this refactored legacy Bitcoin path untested; Liquid exercises a similar path, but under different chain types/cfgs.
Could we add at least one Bitcoin integration test or TestRunner variant with use_spenttxouts: false, covering sync plus a spend/history/utxo query?
There was a problem hiding this comment.
My thinking was that Liquid exercises the relevant code paths for the legacy indexer, and that it could be sufficient since the Bitcoin/Liquid-specific differences all live in the lower-level helpers shared by both indexing modes.
But I agree it's probably not good enough. It seems best to also test this against an older Bitcoin Core release that didn't support spenttxouts at all, which is what I implemented in 8958426.
| let mut log = stderrlog::new(); | ||
| // Base verbosity is 2 (Info), each -v flag adds one level: | ||
| // no flags = Info, -v = Debug, -vv = Trace | ||
| log.verbosity(2 + m.occurrences_of("verbosity") as usize); |
There was a problem hiding this comment.
I'm wondering if we should add a log.modules(["electrs", "electrs_macros", etc]); here...
Reason is that when in trace mode... the new spenttxouts calls route through ureq-proto which hex-dumps all of the /rest/block/*.bin and /rest/spenttxouts/*.bin responses... and I was piping the logs to a file and ran out of disk space when they got to 1.3 TB 😅 Might be nice to scope the verbosity to just modules we "care about".
There was a problem hiding this comment.
Huh yeah that's no good >.< Should definitely be addressed.
Rather than hiding dependency logging entirely, it could be useful to keep warn-level logs for dependencies (plus potentially info/debug too in higher -v levels, perhaps with a new -vvv mode) while hiding trace-level logs which cause the log size blow up.
However it seems that stderrlog doesn't support setting a per-module logging level, so we have to settle for either hiding dependency logging entirely or showing it at the global logging level.
There's a compromise we can make: have no -v (warn+info) and one -v (debug) enable logs for all modules, make -vv (trace) set log.modules(["electrs"]), and treat -vvv as enabling trace for all modules.
This would result in quite a surprising behavior, though: no -v and one -v would show dependency logging, while increasing to -vv suddenly hides it. But still, it seems better than always hiding it?
Another alternative we can consider is switching from stderrlog to env_logger, which does support per-module verbosity levels.
| .iter() | ||
| .map(|hash| json!([hash, /*verbose=*/ false])) | ||
| .collect(); | ||
| let url = format!("http://{}/rest/block/{}.bin", self.addr, blockhash); |
There was a problem hiding this comment.
Codex-discovered review note (blocking): new-index now supports --daemon-rpc-fallback-addr, but REST requests here always use the fixed primary addr. If JSON-RPC fails over, block and spenttxouts fetching can continue targeting the unavailable primary.
During the rebase, could we make REST backend selection consistent with daemon failover—ideally trying the active RPC backend first and the alternate backend for retryable failures? It would be good to cover this with a test where the primary becomes unavailable during indexing.
There was a problem hiding this comment.
Implemented in 90a62fa.
I considered a few approaches for this, and ended up letting Connection be the source of truth for backend selection and failover. The REST client reuses the active Connection address and triggers the Connection reconnection mechanism on transport errors.
Recycling is implemented through a new RestAgent wrapper. This also required putting the agent behind an Arc<Mutex<T>> for interior mutability and synchronization, but the cost should be negligible since the lock is only held for the max-age check and cheap Arc cloning in the typical case (or the ureq::Agent re-initialization when recycling).
| let previous_txos: Vec<TxOut> = daemon | ||
| .get_spent_txouts(block_entry.entry.hash())? | ||
| .into_iter() | ||
| .flatten() |
There was a problem hiding this comment.
Codex-discovered review note (blocking): Flattening the response loses the per-transaction boundaries before they are validated. index_block() verifies the total number of consumed prevouts, but a malformed response with the correct total and incorrect per-transaction counts could associate prevouts with the wrong inputs.
Could we first verify that (1) the response contains block.txdata.len() - 1 entries and (2) each entry has exactly as many prevouts as the corresponding non-coinbase transaction has inputs? Then flattening it for the existing indexing interface is safe.
There was a problem hiding this comment.
This codebase generally assumes bitcoind returns valid consistent replies, there are quite a few other places where we could assert more defensively for malformed replies but currently don't.
I'm not saying we necessarily shouldn't validate in this particular case, but not sure where the line is crossed. wdyt?
| initial_sync_batch_size: value_t_or_exit!(m, "initial_sync_batch_size", usize), | ||
| db_cache_index_filter_blocks: m.is_present("cache_index_filter_blocks"), | ||
| #[cfg(not(feature = "liquid"))] | ||
| use_spenttxouts: !m.is_present("no_spenttxouts"), |
There was a problem hiding this comment.
Codex-discovered review note (recommended): spenttxouts is now the default, but startup still accepts older Bitcoin Core versions and only discovers missing REST support once indexing begins. Could we perform an early capability check and return an actionable error such as: “spenttxouts indexing requires Bitcoin Core 30+ with -rest=1; use --no-spenttxouts for older Core”?
This would distinguish an unsupported Core version, disabled REST, and a transient connection failure.
There was a problem hiding this comment.
I implemented a variant of your suggestion in e93d17d: it errors if REST is disabled, but checks for spenttxouts support to automatically select the indexing mode, instead of the --no-spenttxouts CLI option. When it's unsupported, it logs a warning suggesting to upgrade Bitcoin Core.
Do you think that's better, or would you prefer the explicit --no-spenttxouts and an error when the setting is incompatible with the daemon?
|
Codex-discovered review note (blocking): The consistency document is valuable, but this PR does not appear to add automated tests for the new completion-marker and crash-recovery behavior. Given that out-of-order indexing is the reason these mechanisms changed, I think we should turn the principal recovery cases into tests before merging. At minimum:
Each test should reopen the database and verify both the visible tip and history/UTXO results, rather than only checking marker rows. |
Remove the V1-to-V2 migration script. This branch makes several DB schema changes and no longer supports a migration path for old DBs, requiring a reindex instead.
Remove blk*.dat support and the `FetchFrom` dispatch around it, leaving support for RPC-based syncing only. Drop the now-obsolete `--blocks-dir` and `--jsonrpc-import` config, daemon BLK-file helpers and initial-sync switching logic.
Drop light-mode indexing and its RPC-based querying. The indexer now always stores the data needed for local block and transaction queries. This changes the compatibility marker format which breaks DB compatibility, but doesn't bump `DB_VERSION`. It is bumped once in a later commit, when switching to column families.
Consume prevouts as ordered iterables during block indexing instead of building a `HashMap` and looking up each prevout by outpoint. This avoids extra map build/lookup work and matches the shape needed later to consume the `spenttxouts` format. The shared shape is one ordered prevout vector per block: txstore-based lookup partitions the flat batch `multi_get` result into that shape, and the `spenttxouts` mode will flatten Bitcoin Core's per-block/per-transaction results into the same shape.
Split the batch-level add/index helpers into `add_block()` and `index_block()` to make the row collectors reusable by the later `spenttxouts` indexing mode.
Have `headers_to_process()` return `HeaderWork` entries recording whether each header still needs txstore rows, history rows, or both. This avoids taking another lock to re-check block status after fetching blocks, and will be shared by both indexing modes.
Remove pre-existing unnecessary cloning. This cleanup is unrelated to the `spenttxouts` changes and only affects the existing indexing mode.
- Switch block fetching to the binary REST `block` endpoint, making REST the only supported block fetch source for both Bitcoin Core and Elements. - Add REST client support for `spenttxouts`, currently unused until the `spenttxouts` indexing mode is added in the next commit. - Reuse the bounded daemon Rayon pool for parallel REST requests. - Start test regtest nodes with REST enabled.
The new mode fetches each block's spent prevouts using binary REST, avoids local TXO lookups during indexing, and allows blocks to be processed in parallel without chain-order dependencies. Bitcoin Core indexing now defaults to the spenttxouts-based mode, with `--no-spenttxouts` as a fallback for older releases that don't support it (<v30). Elements stays on the existing txstore-based indexing mode. The integration tests node was updated to Bitcoin Core v30.2.
…o prevent OOM With pin_l0_filter_and_index_blocks_in_cache enabled, each L0 SST file pins its bloom filter and index blocks in the block cache. With the old settings (trigger=64, 256 MB write buffers), L0 could accumulate to the stop threshold — at ~9.75 MB pinned per file, this consumed ~3.7 GB across 3 DBs, overflowing the 2 GB block cache and spilling to heap, triggering OOM during mainnet initial sync around block 750k. Lower the trigger to 32 and set slowdown=×3 (96 files) and stop=×4 (128 files). With 128 MB write buffers (~4.9 MB filter blocks per file), pinned metadata at the stop threshold is ~1.88 GB across 3 DBs — within the 2 GB cache at all times. Set the write buffer CLI flag accordingly: --db-write-buffer-size-mb=128
Since the spenttxouts-based indexer can process blocks in parallel and complete them out of order, consistency now requires crash recovery based on completion markers and revised tip-publication rules: - Derive startup chain visibility from the persisted `t` chain plus txstore/history completion markers. The first daemon-aware `update()` then sweeps history-complete stale blocks before normal indexing work. - Change reorg handling to persist the common ancestor in the DB prior to undoing stale block data, so a crash during stale cleanup restarts from the rollback point and completes recovery. - Store the block height alongside the block header in `B` rows, so that stale recovery can rebuild `HeaderEntry`s from RocksDB alone. This changes the DB schema but does not bump `DB_VERSION`. It is bumped once in a later commit, when switching to column families. - Add an indexer consistency and crash recovery document describing the current safety model, recovery assumptions and caveats. At this point stale cleanup still depends on the daemon for stale block contents, and using separate RocksDB databases can still lead to a corrupted state because cross-DB WAL-disabled writes may be flushed implicitly and independently.
Reconstruct stale `BlockEntry`s and spent prevouts from local txstore `B`, `X`, `T`, and `O` rows instead of fetching them from the daemon. This is used for both live reorg cleanup and startup stale cleanup, so undo no longer depends on the currently connected daemon having the stale block.
- Store `default`, `txstore`, `history`, and `cache` as column families in one physical RocksDB. - Move compatibility metadata and persisted tip `t` into the default column family. - Flush txstore/history bulk writes with RocksDB atomic flush before publishing `t`, replacing the separate-DB flush sequence and removing the remaining cross-DB durability caveat. - Bump `DB_VERSION` for the schema changes in this branch.
(to be squashed into `feat(daemon): Add Daemon REST client for block/spenttxouts endpoints`)
(to be squashed into `cleanup(index): Remove BLK-file support and related config`)
(to be squashed into `feat(daemon): Add Daemon REST client for block/spenttxouts endpoints`)
(to be squashed into `feat(index): Add spenttxouts-based indexing mode`)
(to be squashed into `fix(index): Redesign crash recovery and stale block cleanup`)
- Check for daemon REST support on startup. - Probe for `spenttxouts` support and use the default indexing mode when available. Otherwise, warn and fall back to legacy indexing. - Remove `--no-spenttxouts`.
- Retain the bulk L0 compaction trigger at 32, but slow and stop writes at 48/64 files and at 8/32 GiB of pending compaction debt. This lets RocksDB regulate ingestion when compaction cannot keep up. - Expose the target SST size and bulk-load pending-compaction limits as tuning knobs, with defaults of 1024 MB and 8/32 GiB. - Cap max_subcompactions at four to limit compaction concurrency and memory pressure under high db_parallelism settings. Spenttxouts processing remains fully parallel, with no application-level polling or chunking. Co-authored-by: Philippe McLean <philippe.mclean@gmail.com>
39d3ca3 to
2938ada
Compare
The reason I added the consistency document is that testing the full indexer behavior through the high-level What can be tested more easily is the underlying
Testing case 4 gets into trickier territory. I'm not sure whether adding it or the other higher-level tests is worth the complexity at this stage, but I'm open to discussing this. One thing that makes this more appealing is that the same testing infrastructure we would need here (bitcoind mocking and cross-restart tests) would also be valuable for testing edge-case mempool behaviors. I would still leave it for a followup PR though.
We already have other tests tying the visible tip to history/UTXO results. I opted to keep the |
|
Rebased and addressed review comments. I kept the fixes as separate commits to aid review, to be squashed later. |
Thank you, will check them out. One thing I notice is that Looks like the cause is 8958426 ("test(index): Test legacy indexer with Bitcoin Core 29"), which moved the Core version selector out of the dev-dependency and into a default-on feature: and cargo install strips dev-dependencies, so once test-core-30 activates it references a dependency that isn't in the install-time manifest. cargo build and cargo test keep dev-deps, which is why CI here is green... only cargo install breaks. Reproduced on 2938ada |
3 participants
This PR adds a new indexing mode using the Bitcoin Core
spenttxoutsREST endpoint to look up spent prevouts and switches to REST for raw block contents (in both indexing modes).The existing indexing mode operates in two phases: first populate
txstore, then populatehistorywhile looking up spent prevouts intxstore. This makes indexing read-heavy and introduces an ordering requirement: before a block can be indexed intohistory, it and all its ancestors must first be populated intotxstore.The new indexer fetches each block alongside its spent prevouts directly from the Bitcoin Core binary REST API using the
blockandspenttxoutsendpoints, then writes the completetxstore+historyDB rows for each block. This allows blocks to be indexed in any order with full parallelism, as a write-only workload with no local RocksDB reads.The new indexer is now the default operating mode for Bitcoin, but the old indexer is selected automatically when
spenttxoutsis not supported: for Elements/Liquid, where it is the only mode, or for Bitcoin Core prior to v30.Commits can be grouped into:
Cleanup: removes obsolete operating modes that are no longer supported, to reduce maintenance burden when making indexer/schema changes. This includes DB migration, BLK import support and light-mode. (923cd01, 56babed, 2967e60)
Groundwork: prepares shared infrastructure for both indexing modes: switch to ordered prevout consumption, extract reusable code and introduce
HeaderWork, plus a perf fix. (cf403ea, d60d985, 495bf2b, a2d6df9)Core work: adds binary REST client support and the
spenttxouts-based indexer, and tunes RocksDB for increased write throughput. (9cc2622, 4a03aba, e08b42f, e93d17d, 2938ada)Consistency: adapts crash recovery, tip publication, stale undo and RocksDB durability for out-of-order block indexing. (ed7af75, 9222fcb, c1a23c9)
Consistency and crash recovery
Out-of-order block indexing required adapting crash recovery to use a new startup stale cleanup mechanism, where Electrs compares the daemon's best chain with the
Dcompletion markers and cleans up any stale blocks before normal indexing work. During reorgs, the common ancestortis now persisted before stalehistorydeletion so a mid-way crash will recover from a safe tip and complete cleanup.Store::open()also includes a startup fail-safe mechanism from an earlier recovery design (that was discarded), that trims chain visibility down to the fully indexed contiguous prefix. The current design does not depend on this, but I kept it anyway to guard against unexpected/invalid state. This could alternatively panic instead of trim.Stale undo now uses local RocksDB data instead of the current bitcoind. This makes recovery safe in multi-backend deployments, where Electrs might reconnect to a different daemon that does not have the stale block.
The PR also adds an AI-assisted document describing the safety model, enumerating potential crash-recovery failure cases, and explaining how they are handled. This helped validate the implementation and identified the two (pre-existing) failure cases fixed by 9222fcb and c1a23c9. I wasn't sure whether to include this in the final PR, but ended up keeping it as reference for humans/AIs making consistency/safety-sensitive changes.
RocksDB Column Families
The index now uses a single RocksDB database with four column families:
default,txstore,historyandcache.This was necessary to resolve a consistency/durability issue with the prior multi-DB design, and also desirable regardless.
Potential Followups
spenttxoutssupport (a simple+109patch to rest.cpp plus docs/tests), we could consider supporting recent Core/Elements versions only and remove the old indexer mode entirely.bitcoin_slicescrate for allocation-free block parsing. I initially thought to include this alongside the new indexer, but it's big enough of a change to warrant its own PR. We could take inspiration frombindexwhich usesbitcoin_slices.Backward Compatibility
This PR breaks DB compatibility and requires a complete reindex.