wave5: LockTime::ZERO/Sequence::MAX canonicalization + double_sha256 dedup - #1362
Conversation
… VecDeque::contains
|
I'll fix CI failures and address comments from users with write access. I'll skip comments containing "(aside)".
Original prompt from a
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Review limit reachedYou'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 46 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (40)
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. Comment |
There was a problem hiding this comment.
This cleanup PR successfully consolidates canonical constants and removes code duplication across the codebase. The changes include:
- Replacing manual constant constructions (
LockTime::from_consensus(0),Sequence::from_consensus(u32::MAX)) with canonical constants (LockTime::ZERO,Sequence::MAX,OutPoint::null()) - Removing duplicate
double_sha256implementations in favor of the canonicalbitcoin_rs_primitives::encode::double_sha256 - Optimizing collection membership checks to use built-in methods (
containsvsiter().any())
All changes maintain functional equivalence while improving code consistency and readability. No defects found.
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.
…+ 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. -->
Summary
Wave-5 of the aggressive-cleanup campaign: canonical-constant consolidation on
primitivesspellings that survived wave-4'sOutPoint::null()pass, plus residual hash/idempotence dedup.CUT
LockTime::from_consensus(0)→LockTime::ZERO(const already existed;from_consensusis identity — byte-identical)Sequence::from_consensus(u32::MAX)/Sequence::from_consensus(0xffff_ffff)→Sequence::MAX(the0xffff_ffffspelling was a wave-4 miss pattern)OutPoint::new(Txid::default(), 0xffff_ffff)→OutPoint::null()(same miss pattern,rpc::handlers::mining+rpc::resttest fixtures)double_sha256test helpers →bitcoin_rs_primitives::encode::double_sha256canonical owner (node/tests/crash_recovery.rs— also dropped its now-unusedsha2import;script/tests/segwit_v0_sighash_edges.rsbecomes a one-line delegate)p2p::block_stager::received_order_contains—.iter().any(|q| q == hash)→VecDeque::containsKEEP (adversarial verdicts on rejected cuts)
rpc/src/context.rs:2557rows.get(0)—rowsissonic_rs::Value, notVec;Valuehas no.first()utxo/src/shard.rs:170.iter().any(|t| *t == output.script_pubkey)—script_pubkeyis&[u8];slice::containscan't compareVec<u8>elements against a slice without allocconsensus::verify_block::sha256d— already a thin adapter overencode::double_sha256returning[u8; 32]for merkle pathsunreachable!inprimitives/layout.rs,index/block.rs,rpc/rest.rs— named invariants on validated spans, not slop.expect()sites insrc/— audit confirmed all are test-mod or test-harness files (zero in prod paths)core_vectors.rs.any(|r| r == reason)— non-allocating&Stringvs&stridiom;Vec<String>::containswould needto_owned()Zero public-API delta → 0.11.0 stands, no corpus re-pin.
Gates:
cargo check --workspace --all-targetsclean;cargo clippy-D warningson all 4 CI profiles (workspace, consensus no-default, chainstate fjall, node fjall+zmq, rpc no-default);cargo fmt --check; tests green for primitives/consensus/utxo/mempool/chain/index/storage (~900 tests).Link to Devin session: https://app.devin.ai/sessions/7338687e984d465b81e17da443ba16e5
Open in Devin Desktop: https://app.devin.ai/desktop/session/7338687e984d465b81e17da443ba16e5?variant=devin
Requested by: @metaphorics
Summary by cubic
Consolidates canonical spellings across the codebase: ~85
LockTime::from_consensus(0)→LockTime::ZERO, ~30Sequence::from_consensus(u32::MAX)/0xffff_ffff→Sequence::MAX, and twoOutPoint::new(Txid::default(), 0xffff_ffff)→OutPoint::null(). Replaces localdouble_sha256test helpers withbitcoin_rs_primitives::encode::double_sha256and swaps.iter().any(...)forVecDeque::containsinp2p::block_stager.No public API delta; the replaced
from_consensuscalls are identity functions, so all values are byte-identical.Written for commit 790cecb. Summary will update on new commits.