Skip to content

refactor: decomplexification wave 3 — third-order cuts, net −638 LOC - #1360

Merged
metaphorics merged 91 commits into
mainfrom
devin/purge-wave3
Oct 2, 2026
Merged

metaphorics merged 91 commits into
mainfrom
devin/purge-wave3

Conversation

@metaphorics

@metaphorics metaphorics commented Oct 2, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Wave 3 of the decomplexification campaign (follows #1356, #1358): a third pass across all 14 crates against post-merge main 835ccde1, hunting third-order structure — wave-2 KEEPs re-audited to their consumer roots, types-as-containers collapsed, zero-callsite trait impls, Option/Result fields provably never None/Err, dead cfg(test) seams, and allocation overkill. 89 commits, +795/−1433 (net −638 LOC).

Every cut was verified bidirectionally (constructors, call sites, cross-crate consumers); items surviving that check are reported below as KEEPs.

Cross-crate boundary cuts (integrator phase)

  • utxo::UtxoReader::get deleted — sole workspace caller was node/mining.rs; migrated to get_entry(&outpoint).map(|coin| coin.txout).
  • chain/Cargo.toml: dropped bytemuck — fully unused after the wave-3 NodeId Pod/Zeroable derive removal.
  • primitives: PartialEq<u32> for CompactTarget, Display for OutPoint, From<Script> for Vec<u8> deleted — call sites in chain/mempool tests repathed (.to_consensus(), as_bytes().to_vec()).
  • reference_set.rs thiserror::Error impls — agent cut REVERTED: the rpc test fixture #[path]-includes this file and propagates ReferenceError; bin-only grep missed it. Classified KEEP.
  • script kernel-gated test .into() sites repathed to Amount::from_sat (From<u64> was deleted in wave 2).

Audited and KEPT

  • index: DerivedIndexQueryEngine dual body-source (body_source + block_source.block_body_source) is not redundant — the engine resolves hash tip-relative (node_at_height_from(tip_id, …)) while IndexBlockSource resolves active-chain (active_node_at_height); they diverge under reorg. Same for TxIndexSpawn's twin fields and resolve_block_body_bytes's fallback branch.
  • mempool::RemovalReason::Clear — boundary enum carried by MutationOutcome; variant kept though its only constructor is now test-seam-gated.
  • chain::tree::median_time_past_at(node, window) — parity test legitimately exercises windows 0/5/15 (same verdict as wave 2).
  • chain::{median_time_past_before_height, ancestor_chain} — single production caller each in chainstate, but chain owns tree traversal; folding would push chain logic across its boundary.
  • utxo::WindowOverlayError — single-variant error enum with .is_err()-only caller; kept as honest boundary error surface rather than collapsing to bool.
  • utxo max_script_size plumbing — callers pass consensus-owned MAX_SCRIPT_SIZE; removing the param would need a utxo→consensus dep the wrong direction.
  • p2p: handshake::{start, post_verack_messages}/Peer::new, compact_blocks::{Outcome, Reconstruction} — external test/bench harness consumers (benches are separate crates needing pub).
  • node: DisconnectMutationError/pub mod chain_effects, spawn_tx_ingress_consumer, sync::block_sync, ~25 NodeState accessors — pinned by external tests/benches/rpc harness.
  • rpc::Context::new → Default — clippy::new_without_default requires the derive pairing.
  • script: SigVersion::{Base, WitnessV0, Tapscript} + TxSignatureChecker constructors — tests/ecdsa_encoding_order.rs seam; Stack machinery — tests/stack_depth.rs.
  • storage: RetentionRegistry, HistoryAccess, MandatoryRetention, checkpoint::*, recovery_evidence::*, undo::* — all prod-consumed by chainstate/node/index.
  • consensus: verify_prepared_units/BlockScriptChecks/BatchScriptFailure — chainstate window batch-verify API; kernel feature surface (unbuildable on this VM — CI lane covers it).
  • mempool: for_each_identity, stable_generation, max_total_bytes, force_chain_generation, attach_observer_leg, submit_transaction, preview_transactions, esplora/fee-history surface — all prod callers verified in p2p/node/rpc.
  • primitives: Display for LockTime (gate-red revert — verify_tx.rs:298 formats it positionally), PartialEq<u32> for Sequence (supertrait of live PartialOrd<u32>), From<u32> for CompactTarget (0.into() in index bench), IntoIterator for &Witness, ByteSpan::is_empty (clippy len_without_is_empty pairing).
  • bin: CoreValue trait + conf-table machinery, apply_core_exception, parse_core_bool vs node::parse_bool (conf swallows / env rejects unparseable bools — not dupes), ReferenceError.detail (parser diagnostic on ManifestUnreadable).

Per-crate third-order highlights

  • primitives (3 cuts): zero-callsite trait impls only — PartialEq<u32> for CompactTarget, Display for OutPoint, From<Script> for Vec<u8>.
  • consensus (5): NativePreparedTx marker + native::verify_input forwarder; verify_prepared_units_with_hooks + its hook generics; BlockRuleContext::non_contextual shim; dead PartialEq/Eq on BlockRuleContext/SoftforkState; unused trace::{BlockConnectedArgs, AddedArgs, RemovedArgs} aliases (probe arg tuples preserved byte-for-byte for SDT ABI).
  • script (13): SigVersion::Taproot variant + check_schnorr_signature sigversion param (key-path verification never built it); eval loop rebuilt per-instruction iterator + hand-rolled remove_all/instruction_len splitter → iterates Instructions directly; advance()/push_opcode_for()/executed_push dead blocks.
  • storage (4): BlockPruner<S> + prune_step/prune_prefixed_rows + re-export; logical_column_family single-caller wrapper; dead derives on ExecutedFrontier/StagedPrune/PruneOutcome/StorageBackend.
  • utxo (8): UtxoChangeListener trait collapsed to concrete CoinStatsListener (sole impl); with_capacity_hint → new() (callers passed bool exprs, not capacities); for_each_coin 6-scalar tuple → SnapshotCoin<'_>; stable_view_len/stable_view_record_count shims; dead Clone/Default derives on DisconnectReceipt/UndoBatch.
  • chain (7): SignallingDeployment::locked_in field + collapsed Started/LockedIn arms; CachedState container → u8 tag through Bip9Cache; StableRead::try_lock test-seam gated; NodeId Default/PartialOrd/Ord/Pod/Zeroable derives + bytemuck import (dep dropped integrator-side); validate_empty_tree_root/compare_expected_bits single-site inlined; vestigial extern crate alloc.
  • chainstate (7): always-identical restored_initial params deleted (values owned internally); WindowGroup.first_prev redundant cache; three identical flush-failure builders → flush_group helper; WitnessPresence two-variant enum → has_witness: bool; into_scratch_parts/ApplyScratchCapacities collapsed; ApplyError #[from]→#[source] where no ? sites exist.
  • index (4): TxIndexWriter/TxIndexSnapshot dead Err(Unsupported*) default bodies → required methods; IndexError::{UnsupportedRollback, UnsupportedAnchor} variants deleted (only constructors were the removed defaults); free fns converted to Worker methods.
  • mempool (11): test-seam gating, not deletion — ReplacementCandidate/ReplacementPlan re-export + struct + with_sigop_cost, MempoolGateway::{orphan_count, recent_rejects_count, is_rejected, has_observer, admit_transaction, clear, evict_below_fee_rate}, Mempool::{clear, evict_below_fee_rate, insert_entry}, fee-decay accessors, OrphanPool::len/AdmissionLifecycle::is_rejected/rejects_len all gated #[cfg(any(test, feature = "test-seam"))] — production admission runs submit_transaction's claimed path; README admission-door docs corrected.
  • p2p (5): DroppedBlock{hash} → Hash256 (consumer maps through tree.lookup); InventoryVector alias deleted → &Inventory/&[Inventory]; BlockStager/StagedBlock/post_verack_messages demoted pub→pub(crate).
  • mining (9): shared publish path between generation wakes; single-flight slot cleared on flight id alone; signet challenge hex decoded at compile time via const fn; selection plumbing unified on FixedReservation/SelectedBody structs (5-/3-tuple returns replaced); merkle_root_from_txids/modified_fee single-use inlined; witness_commitment_script extends WITNESS_COMMITMENT_PREFIX; FakeMiningControl 17-field literal → from_parts().
  • rpc (3): Context::with_zmq_publisher zero-callsite; dead index == 0 && coinbase guards (is_coinbase requires inputs.len() == 1); Auth enum {Basic, Cookie} → struct {user, password_hash} (identical fields both variants).
  • node (6): EvidenceIdentity::labels(); ChainFollowers::{with_zmq_publisher, derived_index} gated to test seams; NetworkSelection::parse folded into FromStr; settle_window_* demoted private; DerivedIndexHost/TxIndexSpawn/NodePruneService::new → pub(crate).
  • bin (5): Option clones → take() in main; cfg(test) load() error.exit() → ? (would kill the test binary); strip_inline_comment find(['#',';']); Vec<UserConfig> → [UserConfig; 2] (always exactly [global, selected]); thiserror impls reverted (see boundary).

Gates (local equivalents of CI)

  • cargo fmt --all -- --check ✓
  • ./scripts/ci-pr.sh clippy — all 4 profiles ✓ (-D warnings)
  • ./scripts/ci-pr.sh test-crates — all 4 profiles ✓ (consensus 126, chainstate 69, node 253, rpc 631 tests)
  • cargo build --locked -p bitcoin-rs ✓
  • cargo check fuzz crate ✓ (lockfile regenerated)
  • cargo deny is CI-only; only dep graph change is chain dropping bytemuck.
  • kernel and rocksdb feature lanes can't build on this VM (no libbitcoinkernel/libclang) — cfg-symmetric shapes verified by inspection; CI covers them.

Versioning

Workspace already pinned at 0.11.0 (wave-2 bump); wave-3 pub-API deltas ride the same unreleased minor. No version changes this wave.

Link to Devin session: https://app.devin.ai/sessions/77becdea40c146279ea9dbe191ac35e1
Open in Devin Desktop: https://app.devin.ai/desktop/session/77becdea40c146279ea9dbe191ac35e1?variant=devin
Requested by: @metaphorics


Summary by cubic

Wave 3 of the decomplexification campaign: 89 commits pruning dead code, collapsing single-implementation abstractions, and confining test-only APIs to test seams across all 14 crates (net −638 LOC). Behavior is unchanged; the public surface shrinks a bit and some pub items become pub(crate) or test-seam-gated.

  • Cross-crate cuts: UtxoReader::get is gone (sole caller migrates to get_entry), Display for OutPoint, PartialEq<u32> for CompactTarget, and From<Script> for Vec<u8> are removed with call sites rerouted, and chain drops its unused bytemuck dependency.
  • Re-audited KEEPs: the reference_set thiserror removal was reverted (an rpc fixture path-includes the file), and several single-caller items stay because they are the honest boundary surface (chain tree traversal, WindowOverlayError, p2p compact-block state, etc.).
  • Test seams: mempool mutation doors and RBF replacement types are gated behind #[cfg(any(test, feature = "test-seam"))]; production admission and removal paths are unchanged. MempoolGateway::admit_transaction, clear, and evict_below_fee_rate move under the gate.
  • Script: the eval loop now drives one Instructions cursor instead of rebuilding per-instruction state; unconstructible SigVersion::Taproot and its sighash parameter are deleted.
  • RPC: Auth enum collapses to a struct (the variant tag was never read), Context::with_zmq_publisher is removed, and dead coinbase guards drop.
  • Wave-3 findings folded in: the apply_window length-mismatch pre-check is restored, listener docs now state the shard-failure delivery contract, and the README admission-door docs describe submit_transaction as the production door.

Verification

  • fmt, clippy, and test-crates pass across all four profiles; cargo build --locked -p bitcoin-rs and the fuzz crate check clean.
  • kernel/rocksdb lanes are verified by inspection only (this VM cannot build those features); CI covers them.
  • Workspace stays at 0.11.0; the pub-API changes ride the same unreleased minor.

Written for commit 1cefa90. Summary will update on new commits.

Review in cubic

Zero call sites across the workspace: benches/evidence and
benches/sync_pipeline consume EvidenceIdentity via serde and direct
field access, and the Prometheus exporter never installs the identity
as labels (OBS-01). The accessor was dead public API.
with_zmq_publisher has one caller — the in-crate ZMQ ordering test —
and derived_index() is read only by in-crate unit tests. Production
builds no longer export them, matching the wave-2 gating convention
(RuntimeInputs::with_mempool_observer, MetricsServer::local_addr).
Its only caller is the FromStr impl two lines below; every external
parse path goes through options::parse_network -> from_str. Nothing
outside config.rs referenced it.
settle_window_success/settle_window_failure were pub(crate) but are
called only inside p2p_chain_adapter::commit_window; no test or
sibling module references them.
config_from and measure_storage copy three Option fields out of cli only
because cli was not declared mut; the values are moved into the request
and layer immediately after. take() expresses the consume directly and
drops the dead clones.
error.exit() on a parse failure terminates the whole test binary instead
of failing the calling test; anyhow already converts clap::Error, so the
match collapses to ?.
parse_for_network always emits exactly two layers, so the Vec is an
unbounded container for a bounded shape; an array states the pair
directly and callers still extend()/iter() unchanged.
The two find() calls plus a min() over Option pairs reimplemented "first
occurrence of either byte"; find with a char set is that operation
directly.
ReferenceError values are only ever compared with assert_eq! or
.expect()ed on Ok — nothing Displays them or propagates them as
std::error::Error, so the derive and its #[error] strings are dead
impls. The variant/field Debug repr is the diagnostic the gate prints.
DerivedIndexHost and its methods, NodePruneService::new, and
TxIndexSpawn were pub(crate) but every construction and call site
sits inside the crate::state module tree (state.rs, state::open,
state::index, state::tests). Nothing in lifecycle, sync, or tests
outside that subtree names them.
Both call sites of restored_initial pass open_existing: false and
ResumeSource::Checkpoint; the only open_existing: true bootstrap is the
inline construction on the journal-replay path. Own both constants inside
the helper.
The field mirrored pending.first().prev_hash: it was set only when staging
into an empty pending, and read only inside flush where pending is
non-empty by construction. Derive it from the first staged commit.
…phan error variants

Every TxIndexWriter implementor (the sole one being RwLock<IndexWriter>)
overrides seed_script_live_stream, reset_capabilities, and anchor_watermark;
both TxIndexSnapshot implementors (StoreTxIndexSnapshot, QuerySnapshot)
override capability_watermark and live_rows. The trait defaults were
unreachable refusal shims returning IndexError::UnsupportedRollback /
UnsupportedAnchor, and those two variants had no other constructors anywhere
in the workspace. Make the methods required and delete the variants.
Zero callers workspace-wide: production supplies the publisher through
ContextHandles::zmq_publisher in lifecycle wiring, and no test attaches
one post-hoc.
tx_render::transaction_json and both compat::convert raw-transaction
renderers computed 'index == 0 && coinbase'. is_coinbase requires
inputs.len() == 1, so index is always 0 when the flag is true and the
enumerate existed only for the redundant check.
…minated

Auth::{Basic,Cookie} carried identical {user, password_hash} fields and
validate_header merged both arms into the same comparison — nothing in
the workspace matched on or constructed the variants directly. The enum
was a container for a tag nobody read; constructors keep their
source-specific docs.
…bing

wait_for_revision_quiet, wait_for_batch_deadline, and load_body_prefix were
free functions whose parameters were drawn entirely from the one Worker that
calls them (runtime, wake_rx, quiet_period, and a should_stop closure that
was always self.runtime.should_stop). Method form removes the plumbing.
A staged body dropped for retry or eviction was wrapped in a
single-field struct; every consumer only read .hash. Return
Hash256 directly from insert/prune/evict and drop the type.
The alias named the registry Inventory type without narrowing it;
the three signatures it decorated now name Inventory directly.
Reconstruction, its Outcome enum, and the receive/prune entry
points are used only by the listener's message loop. The mempool
hint trait stays public for node's MempoolGateway impl.
BlockStager and StagedBlock had no consumer outside the crate:
the scheduler and download window use them, and node only names
the type in a comment. Demote the re-export and the pub items
inside.
…ders

Three map_err closures in apply_window_admitted built the same fatal
WindowApplyError (two also abandoned the group; the third's group was
dropped unconditionally). flush_group owns the flush/extend-or-abandon
pattern.
The mismatched-length error here duplicated the identical check inside
apply_window_admitted (reached through connect_window): begin_transition
is side-effect-free beyond holding the lock, so the pre-check only
duplicated the error constructor. The Ok/Err arms both dropped the
transition and returned the result.
The two-variant enum wrapped a single bool produced once in
plan_block_transactions and read once at the wtxid dispatch. Carry
has_witness on BlockTxPlan directly.
The 4-tuple and the capacities struct existed only to be destructured at
the single call into ApplyScratch::from_prepared_parts. Pass the
crate-private BlockTxPlan itself; scratch keeps the fields it owns.
UtxoCommit and BlockBodyPersistence constructed every site through
explicit map_err; their #[from]-generated From impls had zero call sites
in the whole workspace (probe-built clean). #[source] preserves the
error-chain source() behavior.
The SendCmpct plan is only consumed by handshake's own exchange and
tests; nothing outside the module calls it.
benches/write_message.rs drives Reconstruction::new /
receive_cmpctblock / receive_blocktxn and matches on
compact_blocks::Outcome: the reconstruction state is a bench seam,
not dead surface.
publish_generation and publish_generation_from duplicated the same
invalidate/install/notify body, and the live tip-hash lookup was spelled
out twice. Extract live_tip_hash and publish_key so each entry point only
builds its key.
Flight ids are unique per installed flight, so the key equality and
result-presence predicates in the clear check were implied by the id
match (a same-id flight is necessarily this key's, and its result was
just stored above).
- header_sync_roundtrip: CompactTarget vs u32 comparisons via to_consensus()/
  revert on u32 field (primitives dropped PartialEq<u32>)
- mempool tests: Vec::from(Script) -> as_bytes().to_vec() (From deleted)
- segwit_v0_sighash_edges: u64.into() -> Amount::from_sat (kernel-gated)
- reference_set: restore thiserror::Error impls (rpc fixture path-includes it)
- UtxoReader::get deleted; sole caller migrates to get_entry().map(txout)
- chain Cargo.toml: drop unused bytemuck dep (NodeId Pod/Zeroable removed)
- block_stager tests: assert_is_empty lint
@devin-ai-integration

Copy link
Copy Markdown
Contributor

I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".

  • Disable automatic comment, CI, and merge conflict monitoring

Original prompt from a

@gosuda/bitcoin-rs @skills:code-simplification @skills:taste @skills:tasty-abstraction Aggressively cleanup and decomplexify codebase.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@coderabbitai

coderabbitai Bot commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Next included review available in 14 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used the included review currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: fa88a2ae-44d9-444a-9382-e94f288f5947

📥 Commits

Reviewing files that changed from the base of the PR and between 9609083 and 1cefa90.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • fuzz/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (91)
  • bin/bitcoin-rs/src/bitcoin_conf.rs
  • bin/bitcoin-rs/src/main.rs
  • crates/chain/Cargo.toml
  • crates/chain/src/bip9_cache.rs
  • crates/chain/src/count.rs
  • crates/chain/src/deployment.rs
  • crates/chain/src/header_sync.rs
  • crates/chain/src/lib.rs
  • crates/chain/src/node.rs
  • crates/chain/src/reorg.rs
  • crates/chain/src/transition.rs
  • crates/chain/src/tree.rs
  • crates/chain/tests/header_sync_roundtrip.rs
  • crates/chainstate/src/connect.rs
  • crates/chainstate/src/error.rs
  • crates/chainstate/src/lib.rs
  • crates/chainstate/src/prepare.rs
  • crates/chainstate/src/recovery.rs
  • crates/chainstate/src/scratch.rs
  • crates/chainstate/src/window.rs
  • crates/consensus/src/bip9.rs
  • crates/consensus/src/kernel.rs
  • crates/consensus/src/trace.rs
  • crates/consensus/src/verify_block.rs
  • crates/consensus/src/verify_tx.rs
  • crates/consensus/tests/overhaul_parse_parity.rs
  • crates/index/src/index/error.rs
  • crates/index/src/index/snapshot.rs
  • crates/index/src/runtime/catch_up.rs
  • crates/index/src/runtime/reconciliation.rs
  • crates/index/src/writer.rs
  • crates/mempool/README.md
  • crates/mempool/src/admission.rs
  • crates/mempool/src/fee_estimator.rs
  • crates/mempool/src/gateway.rs
  • crates/mempool/src/lib.rs
  • crates/mempool/src/orphan.rs
  • crates/mempool/src/pool.rs
  • crates/mempool/src/rbf.rs
  • crates/mining/src/coinbase.rs
  • crates/mining/src/control.rs
  • crates/mining/src/coordinator.rs
  • crates/mining/src/network_hashps.rs
  • crates/mining/src/policy.rs
  • crates/mining/src/template.rs
  • crates/node/src/chain_effects.rs
  • crates/node/src/config.rs
  • crates/node/src/metrics.rs
  • crates/node/src/mining.rs
  • crates/node/src/p2p_chain_adapter.rs
  • crates/node/src/state.rs
  • crates/node/src/state_index.rs
  • crates/node/src/state_storage.rs
  • crates/p2p/src/block_stager.rs
  • crates/p2p/src/handshake.rs
  • crates/p2p/src/inv.rs
  • crates/p2p/src/lib.rs
  • crates/p2p/src/sync/receive.rs
  • crates/primitives/src/outpoint.rs
  • crates/primitives/src/script.rs
  • crates/primitives/src/units.rs
  • crates/rpc/src/auth.rs
  • crates/rpc/src/compat/convert.rs
  • crates/rpc/src/context.rs
  • crates/rpc/src/tx_render.rs
  • crates/script/src/checker.rs
  • crates/script/src/eval.rs
  • crates/script/src/interpreter.rs
  • crates/script/src/script.rs
  • crates/script/src/taproot.rs
  • crates/script/tests/segwit_v0_sighash_edges.rs
  • crates/storage/README.md
  • crates/storage/examples/storage_footprint.rs
  • crates/storage/src/footprint.rs
  • crates/storage/src/lib.rs
  • crates/storage/src/pruning.rs
  • crates/storage/src/pruning/block_pruner.rs
  • crates/storage/tests/backend_equivalence.rs
  • crates/storage/tests/backend_metrics.rs
  • crates/storage/tests/bounded_prefix_scan.rs
  • crates/storage/tests/durable_head_store.rs
  • crates/storage/tests/overhaul_atomic_durability.rs
  • crates/storage/tests/prune_then_reorg.rs
  • crates/storage/tests/storage_footprint.rs
  • crates/utxo/benches/utxo_commit.rs
  • crates/utxo/src/contract.rs
  • crates/utxo/src/listener.rs
  • crates/utxo/src/set.rs
  • crates/utxo/src/shard.rs
  • crates/utxo/src/snapshot.rs
  • crates/utxo/src/stats/coin_stats.rs
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@amazon-q-developer amazon-q-developer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This large refactoring PR (93 files, 89 commits, net -638 LOC) successfully improves code organization and maintainability across the codebase. The changes focus on cleaner abstractions, removal of redundant state tracking, and better separation of concerns.

Key changes reviewed:

  • Script evaluation refactoring with cleaner iterator-based instruction processing
  • Improved window processing with extracted helper functions
  • Removal of redundant state tracking in multiple components
  • Better error handling patterns

No critical defects found that would block merge. The refactorings maintain functional correctness while improving code quality and readability.


You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.


⚠️ This PR contains more than 30 files. Amazon Q is better at reviewing smaller PRs, and may miss issues in larger changesets.

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Runtime smoke test — PASSED (Devin testing agent)

cargo build --locked -p bitcoin-rs --features fjall; offline regtest node.

  • getnetworkinfo → version: 1100, subversion: "/bitcoin-rs:0.11.0/"
  • Golden path genesis RPCs exact; auth 401/-32601/404; REST + Esplora surfaces 200
  • generatetoaddress → tip 0→3; invalidateblock(h2) → disconnect to h1, getchaintips tracks invalid fork, gettxoutsetinfo undo → txouts=1/50 BTC; GBT rebuilds on new tip
  • pruneblockchain → -32603 "pruning is disabled"; reconsiderblock → -32601 (registered unimplemented)
  • SIGTERM → exit 0, orderly teardown; fjall restart preserves tip

Notes: concurrent generatetoaddress raced once → honest Duplicate rejection (expected contention, not a hang). One transient batch of REST/Esplora 404s ~5s after first boot could not be reproduced across 3 more boots — likely environmental, flagged for awareness.

@metaphorics

Copy link
Copy Markdown
Collaborator Author

Fix conflicts.

# Conflicts:
#	crates/mempool/src/gateway.rs
#	crates/node/src/p2p_chain_adapter.rs
@devin-ai-integration

Copy link
Copy Markdown
Contributor

Done — merged main (96090838, #1359's lint-suppression audit) in 58cb2955. Both conflicts were the same overlap: main's allow→expect audit vs my pub(crate)→private demotion / test-seam gate. Resolution keeps both intents (#[expect] + demoted/gated). Workspace check + fmt + clippy clean.

@cubic-dev-ai cubic-dev-ai Bot 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.

All reported issues were addressed

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread crates/mempool/README.md Outdated
Comment thread crates/utxo/src/listener.rs Outdated
Comment thread crates/chainstate/src/lib.rs
Comment thread crates/utxo/src/listener.rs Outdated
Comment thread crates/chain/src/transition.rs
Comment thread crates/mempool/src/rbf.rs Outdated
- chainstate apply_window: restore len-mismatch check before begin_transition
- mempool README: submit_transaction is the pub door, returns SubmitError
- utxo listener docs: shard-failure delivery contract + method-level links
- chain StableRead type doc: try_lock is test-seam-gated
- rbf with_sigop_cost: drop redundant cfg (impl block already gated), fix doc
@devin-ai-integration

Copy link
Copy Markdown
Contributor

Fixed in 1cefa909 — restored the blocks.len() != serialized.len() check before begin_transition so malformed windows fail fast with the explicit WindowApplyError instead of waiting on the transition lock.

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Fixed in 1cefa909 — corrected to MempoolGateway::submit_transaction returning SubmitError; dropped capture_admission (pub(crate)) and the stale PolicyError/MempoolError names.

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Fixed in 1cefa909 — documented that the batch callback delivers every event for mutations that landed even when a shard fails, before the shard error is returned.

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Fixed in 1cefa909 — link definitions now target CoinStatsListener::on_insert_coins / on_remove_coins / on_committed_event_batches methods.

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Fixed in 1cefa909 — type doc now reads "offers lock and nothing else in production builds".

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Fixed in 1cefa909 — dropped the redundant #[cfg] (the impl ReplacementCandidate block is already test-seam-gated) and reworded the doc: with_sigop_cost sets the fact admission resolves itself; production code never names the type.

@metaphorics
metaphorics merged commit 312c354 into main Oct 2, 2026
14 checks passed
@metaphorics
metaphorics deleted the devin/purge-wave3 branch October 2, 2026 12:05
metaphorics added a commit that referenced this pull request Oct 3, 2026
…+ importer repair, net −27 LOC (#1364)

## Summary

Wave 7 of the decomplexification campaign (waves 1–6 landed: #1356,
#1358, #1359, #1360, #1361/#1362, #1363; ~−3.6k LOC). This wave covered
the non-crate surfaces — dependencies/features, docs, tooling/CI config,
and the test harness — plus one live-bug repair found while auditing.
The per-crate fan-out is deferred (see below); integrator-level
verification found no additional provable cuts.

**net −27 LOC** (+77/−104) — small by design: the suite is saturated, so
most verdicts are keeps.

### Cuts

- `crates/node/Cargo.toml` — removed dead `proptest` dev-dependency
(zero refs; only a doc comment names a *different* crate's proptest
file) and collapsed redundant feature unification:
`node/{rocksdb,fjall,redb}` re-listed `storage+X`/`utxo+X` edges already
forwarded under the same name by `chainstate/X` and `index/X`;
`node/kernel` re-listed `consensus/kernel` already forwarded by
`chainstate/kernel`. Resolved feature sets verified byte-identical
(`cargo tree`, 235 entries, zero diff).
- `clippy.toml` — dropped `disallowed-types` config for a lint that is
never enabled.
- `deny.toml` — dropped the empty `advisories.ignore` list.
- `scripts/install-bitcoind.sh` — no net change (the `--export` removal
was restored: it's a public script interface, absence of in-repo callers
doesn't prove unobservability).
- `bin/bitcoin-rs/tests/support/clean_stderr.rs` — whole-file delete;
its only helper collapsed into `e2e::helpers` (the `#[path]`-include
chain shortened one hop).
- `bin/bitcoin-rs/tests/**` — deduped `required_str`/`required_u64` test
helpers onto `e2e::ValueExt`; demoted `WorkspaceGraph` graph fields to
private.
- `docs/policies/p2p-compatibility.md`, `crates/rpc/README.md`,
`crates/node/README.md` — stale-name fixes: `after_connect`→`on_connect`
(C-event publisher), `download_window.rs`→`download_window/policy.rs`
refs, dead `prometheus-http` feature bullet (the feature itself was
deleted in `20a88f12`; the README line survived).

### Repair (not a cut)

- `scripts/import_qa_assets.py` — the QAC-01 importer still parsed the
pre-wave-1 `COMMANDS: &[Command]` shape, so it **fail-closed on every
real run** since `COMMANDS` became `&[&str]`. Repaired the table regex +
count check, and fixed a latent `_mask_rust_raw_strings` bug (an `r"`
tail inside an ordinary string was treated as a raw-string opener,
silently eating table rows). Fixture tests updated. Post-review
hardening on top: char/byte literals (`'"'`, `b'x'`, `'\u{41}'`) are
consumed before `"` is treated as a string opener, and command
extraction now slices back into the unmasked source (the mask preserves
offsets) so raw-string COMMANDS entries keep their selector indices
instead of being masked out and shifting `map_p2p` selectors.

### Keeps (audit ledger — verified, not cut)

- `utxo/{rocksdb,redb}` and `chainstate/kernel` feature names gate zero
in-crate items but are the deliberate same-name forwarding convention
the `g17_dependency_direction` gate encodes (`BACKEND_FORWARDING_CRATES`
+ same-name rule); removing them saves ~8 lines at the cost of an
asymmetric backend surface and only relocates the kernel edge. Kept.
- `chainstate/kernel` — the documented kernel-capability propagation
path (node routes `kernel` solely through it).
- `deny.toml` `bans`/`licenses`/`advisories` rules,
`.github/workflows/**` `ci-gate` jobs — security policy / required merge
checks, out of scope by contract.
- `scripts/run-p2p-core-interop.sh`, fuzz corpus/provenance pipeline
(`fuzz-policy.sh`, `validate-corpus-seeds.sh`, `run-fuzz-campaign.sh`),
`docs/contracts/qa-corpus.md` importer contract — boundary surfaces.
- `crates/rpc/tests/support/fixture.rs` `#[path]`-consume of
`reference_set.rs` — cross-crate test seam; repath recorded, not cut.
- e2e crate public API, `spawn_tx_ingress_consumer`, `sync::block_sync`,
`is_standard_tx` — pinned by their own e2e/contract tests *and* live
prod callers; the pins are the coverage.
- Every remaining `[features]` entry workspace-wide — each gates cfg
sites or forwards a backend/capability (16 test-seam producer→consumer
edges re-verified earned).
- `docs/benchmarks/**`, node test-fixture dedup — owned by open PRs
#1344/#1346.

### Deferred

- 14 per-crate agents + aux were queued but the org's SWE-2 promo cap (7
concurrent sessions) is saturated by an unrelated workflow — the run is
resumable (`wfr-1f85f1f4343b47eeb5c97896e5dcffaa`); any yield lands as a
follow-up.
- `CONCEPTS.md` retained-bench count vs. actual `benches/` dirs — MEDIUM
certainty, overlaps #1344.

### Gates

`cargo fmt --all -- --check` · `cargo clippy --workspace --all-targets
-- -D warnings` · `cargo check --workspace --all-targets` · `cargo test`
on all test-crates incl. `g17_dependency_direction` 12/12 and
`bitcoin-rs-node --features fjall` — all green on the merged state.

Link to Devin session:
https://app.devin.ai/sessions/77becdea40c146279ea9dbe191ac35e1
Open in Devin Desktop:
https://app.devin.ai/desktop/session/77becdea40c146279ea9dbe191ac35e1?variant=devin
Requested by: @metaphorics

<!-- This is an auto-generated description by cubic. -->
---
## Summary by cubic
Wave 7 of the decomplexification campaign cuts dead dependencies and
config, collapses duplicated test harness code, and repairs QAC-01
importer parsing that failed closed on every real run, netting −27 LOC.

**Refactors**

- Dropped the unused `proptest` dev-dependency and redundant
feature-unification edges in `crates/node`; resolved feature sets
verified byte-identical via `cargo tree`.
- Folded `assert_clean_stderr` into `e2e::helpers` (deleting the
`#[path]` include), deduped `required_str`/`required_u64` onto
`ValueExt`, and demoted `WorkspaceGraph` graph fields to private.
- Removed dead config (`clippy.toml` disallowed-types, empty `deny.toml`
ignore list) and fixed stale names in p2p policy, RPC, and node docs.
Restored the `install-bitcoind.sh --export` mode and documented both
script modes.
- Mirrors captured child stderr into the evidence file as it streams,
publishing the `MAX_OUTPUT` tail atomically via a side-file rename.

**Bug Fixes**

- Repaired the QAC-01 importer to parse the `COMMANDS` inventory's
post-wave-1 `&[&str]` shape; it previously failed closed on every real
run.
- Fixed `_mask_rust_raw_strings` and the inventory parser to handle
raw-string and char-literal entries so an `r"` tail no longer counts as
a raw-string opener, no table rows get silently eaten, and command
selectors stay stable across `br#`-style `COMMANDS` entries.

<sup>Written for commit 6db74e6.
Summary will update on new commits.</sup>

<a
href="https://cubic.dev/pr/gosuda/bitcoin-rs/pull/1364?utm_source=github"
target="_blank" rel="noopener noreferrer"
data-no-image-dialog="true"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"><source
media="(prefers-color-scheme: light)"
srcset="https://www.cubic.dev/buttons/review-in-cubic-light.svg"><img
alt="Review in cubic"
src="https://www.cubic.dev/buttons/review-in-cubic-dark.svg"></picture></a>

<!-- End of auto-generated description by cubic. -->
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.

1 participant