Repository navigation
layout: compile library tests against the normal rlib - #1097
Conversation
44878ba to
04f9de8
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
![rearden-grok[bot]](https://avatars.githubusercontent.com/u/730881?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
The compile goal holds. Each library has one tests/suite target and [lib] test = false, so cargo test links the normal rlib. IBD is rbitcoin-ibd; RPC, Electrum, Esplora, and SV2 depend on rbitcoin-net only, and the node still calls the same sync_cancellable arguments. Smoke filters, the journey manifest, and per-hub NODE_COMPACT_FILTERS match that layout. The 89% line-coverage floor matches this run (89.71%, 91583/102091). A 90% floor would still fail.
It is not behavior-equivalent. Twelve tests that exist on e16642338 are absent from the suites (they lived in mod tests blocks the move deleted). Two cfg(test) arms were rewritten as debug_assertions, so a debug node no longer aborts on an undrained io_uring and uses a 2s outbound connect timeout instead of 8s. Eight rbitcoin-test unit tests stay behind #[cfg(test)] and never compile.
Disposition: do not merge until those tests run again and the two debug-node arms match release. The IBD cut, the suite link model, and the 89% floor can stay.
Issue counts by severity
- bugs: 8
- suggestions: 2
- nits: 1
Issues outside the diff
- [nit] .github/workflows/ci.yml:188 -- The coverage job comment still says "Line-coverage 93% floor". The job runs
./scripts/coverage.sh, whose gate isLH*100 >= LF*89. That comment line is not in this diff.CHANGELOG.mdhistory of the old floor is fine.- Suggestion: Say 89% in the workflow comment.
| /// `--proxy` turns the DNS seeds into addr-fetch dials by name. The | ||
| /// proxy resolves the name, so the node holds no address for it. | ||
| #[tokio::test] | ||
| async fn proxied_dns_seed_dials_by_name_through_socks() { |
There was a problem hiding this comment.
[bug] accept_retries_a_full_fd_table_and_stops_on_a_dead_listener and shutdown_finishes_without_the_dial_grace are on master in rbitcoin-net/src/service.rs and are not in this suite. The rebase deleted master's mod tests and ported only proxied_dns_seed_dials_by_name_through_socks.
Suggestion: Move both tests into tests/service.rs with the same assertions.
There was a problem hiding this comment.
Both tests are back in tests/service.rs. accept_error_ends_listen is public so the suite can call it. The fd-table cases and the shutdown deadline match master.
| /// back to the tip's own time, and a coin median is not | ||
| /// `bad-txns-nonfinal`. | ||
| #[test] | ||
| fn check_block_proposal_reads_medians_from_confirmed_headers() { |
There was a problem hiding this comment.
[bug] reconsider_skips_a_branch_that_fails (master chain.rs, from net: reconsiderblock activates the most-work chain) is not in tests/chain.rs. The median proposal test was ported; this one was not.
Suggestion: Move the test into tests/chain.rs.
There was a problem hiding this comment.
reconsider_skips_a_branch_that_fails is in tests/chain.rs again, including the invalid branch and the store-fault arm.
| } | ||
|
|
||
| #[test] | ||
| fn pending_sendcmpct_from_u8_maps_wire_payload() { |
There was a problem hiding this comment.
[bug] external_ip_uses_the_clearnet_bogon_filter (master peers.rs, from the CJDNS/bogon self-announce change) is not in tests/peers.rs.
Suggestion: Move the test into tests/peers.rs.
There was a problem hiding this comment.
external_ip_uses_the_clearnet_bogon_filter is in tests/peers.rs. It still rejects CGNAT, documentation, and benchmark ranges, and it still allows 1.2.3.4.
| @@ -0,0 +1,10 @@ | |||
| #![allow(unused_imports, dead_code, unused_variables)] | |||
| mod accept; | |||
There was a problem hiding this comment.
[bug] Seven master tests in rbitcoin-mempool/src/accept.rs are absent: promote_keeps_the_announcer_of_a_multi_parent_orphan, disconnect_while_the_orphan_is_out_is_not_reparked, older_promote_forget_does_not_clear_a_newer_take, package_lift_disconnect_is_not_reparked, package_admit_drops_the_inflight_row, package_rollback_admit_drops_the_inflight_row, and package_rollback_reject_drops_the_inflight_row. Name diff of every #[test] on e16642338 vs this head is 3003 vs 2991 (twelve missing, zero new).
Suggestion: Move the seven into tests/accept.rs.
There was a problem hiding this comment.
All seven accept tests are in tests/accept.rs with the same assertions, including the in-flight announcer checks.
| mod inhibit; | ||
| mod lock; | ||
| mod regtest_rpc; | ||
| mod run; |
There was a problem hiding this comment.
[bug] max_run_deadline_wakes_without_the_perf_tick (master rbitcoin-node/src/run.rs) is not in tests/run.rs.
Suggestion: Move the tokio test into tests/run.rs.
There was a problem hiding this comment.
max_run_deadline_wakes_without_the_perf_tick is in tests/run.rs. A 40ms deadline still returns Stop in under 200ms.
| let thread = std::thread::current(); | ||
| let thread = thread.name().unwrap_or("unnamed"); | ||
| let reason = format!("drain hard cap pending={pending} thread={thread}"); | ||
| #[cfg(test)] | ||
| #[cfg(debug_assertions)] |
There was a problem hiding this comment.
[bug] finish_drain_hard_cap used to abort outside cfg(test) and return StoreError::Corrupt("invariant: io_uring undrained") only under cfg(test). It now returns that error for every debug_assertions build and aborts only in release. drop_drain and inflight_enter_failure call it. A debug node no longer aborts when SQEs still own caller buffers. The comment above still says abort outside tests.
Suggestion: Abort on every build except an explicit test override. Do not treat debug_assertions as cfg(test) here.
There was a problem hiding this comment.
finish_drain_hard_cap aborts again on a debug node. The suite still sees the Corrupt error, but only inside with_drain_hard_cap. A normal debug build never sets that hook.
| pub const OUTBOUND_CONNECT_TIMEOUT: Duration = Duration::from_secs(8); | ||
| #[cfg(debug_assertions)] | ||
| #[doc(hidden)] | ||
| pub const OUTBOUND_CONNECT_TIMEOUT: Duration = Duration::from_secs(2); |
There was a problem hiding this comment.
[bug] OUTBOUND_CONNECT_TIMEOUT was 8s under cfg(not(test)) and 2s only under cfg(test). The rlib, including a debug node and integration tests, used 8s. It is now 2s whenever debug_assertions is on, and prepare_outbound_session passes it to every outbound dial. The comment still says the bound is 8s. Release stays 8s.
Suggestion: Keep 8s in the rlib. If the suite needs 2s, arm that from a hook the test sets.
There was a problem hiding this comment.
OUTBOUND_CONNECT_TIMEOUT is 8 seconds in every build, including debug. The short redial interval is unchanged. outbound_connect_timeout_is_eight_seconds pins the constant.
| @@ -11,17 +11,19 @@ autotests = false | |||
|
|
|||
| [lib] | |||
| doctest = false | |||
| test = false | |||
There was a problem hiding this comment.
[bug] [lib] test = false means cargo test --test suite never compiles #[cfg(test)] inside this library. These tests were not moved: jsonrpc.rs (try_jsonrpc_returns_the_connect_error, try_jsonrpc_panics_when_the_body_is_not_json), mine.rs (bip34_and_coinbase_height_zero_and_pow, mine_regtest_block_keeps_fixed_coinbase_and_skips_witness_commitment), overlay.rs (jsonrpc_try_panics_when_the_body_is_not_json, i2p_b32_rejects_unknown_base64, i2p_b32_rejects_a_short_destination, i2p_b32_matches_the_address_host). They still exist only under #[cfg(test)].
Suggestion: Move them into the suite (overlay's own [[test]] for the overlay ones) and delete the #[cfg(test)] blocks.
There was a problem hiding this comment.
The jsonrpc and mine tests now run in the default suite. The four overlay helper tests run in the overlay test binary, which is what compiles that module. i2p_b32_matches_the_address_host builds the same 387-byte destination as the net test and encodes it with the I2P alphabet. The old standard-base64 literal no longer parses.
| @@ -65,7 +73,7 @@ pub fn inclusive_u8x8_times_8(st: &[u8; 8], no: &[u8; 8]) -> ([u32; 8], [u32; 8] | |||
| /// # Safety | |||
| /// `p` must be readable for 16 bytes. | |||
| #[cfg(target_arch = "x86_64")] | |||
| unsafe fn sse2_deinterleave_pairs_u8x8(p: *const u8) -> ([u8; 8], [u8; 8], bool) { | |||
| pub unsafe fn sse2_deinterleave_pairs_u8x8(p: *const u8) -> ([u8; 8], [u8; 8], bool) { | |||
There was a problem hiding this comment.
[suggestion] sse2_deinterleave_pairs_u8x8 and the sibling SIMD helpers were private and are now pub unsafe with no #[doc(hidden)]. Call sites are unchanged, so node behavior is the same, but the crate surface now exports the raw pointers. The scalar twins stay #[doc(hidden)].
Suggestion: Keep them private, or #[doc(hidden)] pub if the suite must call them.
There was a problem hiding this comment.
The four SIMD helpers are private again. Only loc_simd.rs calls them.
| @@ -12,14 +12,14 @@ if ! command -v cargo >/dev/null 2>&1; then | |||
| fi | |||
|
|
|||
| echo "== structure, header, finality, sigops ==" | |||
| cargo test -p rbitcoin-consensus --lib -- \ | |||
| cargo test -p rbitcoin-consensus --test suite -- \ | |||
There was a problem hiding this comment.
[suggestion] The consensus command filters structure_rule_tests and assemble_second_block_rejects_stale_nversion, which are not in rbitcoin-consensus. They live in the query suite. Libtest ORs filters, and the other three names match, so the script prints hornet-mapped pins OK without running those two. The same miss existed at the base under --lib. This edit only swapped --lib for --test suite.
Suggestion: Run those two filters as cargo test -p rbitcoin-query --test suite.
There was a problem hiding this comment.
The script now runs structure_rule_tests and assemble_second_block_rejects_stale_nversion in the query suite. The other three filters stay on consensus.
There was a problem hiding this comment.
Summary
Follow-up from the test-move equivalence pass, after the first comment review. Three moved tests are still present but weaker than master. The twelve deleted tests and the two debug_assertions behavior bugs in that review stand.
Short debug redial (250 ms), I2P SAM retry (50 ms), and SV2 constraints cooldown (100 ms) are the intended debug_assertions intervals. run_p2p already warns about those three. They are not additional bugs. The io_uring abort and the 8 s outbound connect timeout are.
Disposition is unchanged: do not merge until the deleted tests run again, these three assertions are restored, and the two debug-node arms match release.
Issue counts by severity
- bugs: 3
- suggestions: 0
- nits: 0
| } | ||
| let accepted = hub | ||
| .accept_package(&[parent.clone(), child.clone()]) | ||
| .expect("hub package contains the parked child"); |
There was a problem hiding this comment.
[bug] hub_package_admits_its_parked_child no longer calls add_orphan_announcer before accept_package, and it no longer asserts orphanage.has_announcer(4) is false after admit. Master locked both: "the parked child takes the peer" and "admitting the lifted child must drop its in-flight peer". No other test in this file keeps those two strings. This is the hub-level twin of the deleted package_admit_drops_the_inflight_row case.
Suggestion: Restore both announcer assertions around this accept_package call.
There was a problem hiding this comment.
hub_package_admits_its_parked_child again checks that the parked child takes peer 4 and that admit drops that announcer.
| assert_eq!( | ||
| side_status(&hub).as_deref(), | ||
| Some("valid-headers"), | ||
| "a held side block outranks a header-only label" |
There was a problem hiding this comment.
[bug] After this valid-headers check, master called hub.reconsider_block(side.block_hash()) and asserted the status stayed "valid-headers" ("a block that never connected is not a valid fork after reconsider"). That call and assert are gone. Other reconsider_block calls remain in this file. reconsider_skips_a_branch_that_fails is still missing entirely, as the first review said.
Suggestion: Put the reconsider status assert back here, and restore reconsider_skips_a_branch_that_fails.
There was a problem hiding this comment.
The reconsider of the never-connected side block is back, and the status stays valid-headers. reconsider_skips_a_branch_that_fails is restored in the same file.
| .as_nanos(); | ||
| let dir = std::env::temp_dir().join(format!("rbitcoin-node-signet-gen-{n}")); | ||
| std::fs::create_dir_all(&dir).unwrap(); | ||
| let params = ChainParams::custom_signet(ScriptBuf::from_bytes(vec![0x51]), 10 * 60).unwrap(); |
There was a problem hiding this comment.
[bug] hub_op_true_signet_generate_one used to start from ChainParams::regtest() with signet_challenge = Some(OP_TRUE) and assert the mined tip bits equal REGTEST_POW_BITS ("the signet miner must grind the parent target, which is regtest here"). It now uses ChainParams::custom_signet, which is signet network parameters and a signet PoW limit, and only checks that the tip height becomes 1. signet_challenge is still a public field and REGTEST_POW_BITS still exists, so the rewrite was not forced by an API removal.
Suggestion: Keep the regtest-plus-challenge setup and the bits assertion from master.
There was a problem hiding this comment.
The test is back on regtest params with an OP_TRUE signet challenge, and it again checks that the mined bits equal REGTEST_POW_BITS.
|
The coverage job comment in |
There was a problem hiding this comment.
Summary
Re-review of ac0d77c0d against e16642338 in four passes: equivalence, goal completeness, safety, and production performance.
Release behavior matches master. Outbound connect stays 8s in every build. An undrained io_uring drain still aborts unless the thread is inside with_drain_hard_cap. Debug builds only shorten the three known intervals (redial 250 ms, SAM retry 50 ms, SV2 cooldown 100 ms). cargo tree -p rbitcoin-node shows rbitcoin-consensus with no features, so the corpus assembler stays out of the node. The consensus suite runs under cargo test --workspace, and after this commit its CRAP set is clean. IBD moved as one crate, so per-header #[inline] helpers did not become cross-crate calls. Release confirm, script, store, mempool accept, and dial match master. No production runtime regression.
The suite-on-rlib layout, the IBD split, the 89% floor, and the overlay feature gate match the goals. On this commit the line gate passed at 91.26% (93410/102361). Coverage is still red: Dialer::connect_isolated_net is the only function over the CRAP 30 gate (43.2).
Two tests that ran in the default suite on master are no longer selected. One Electrum file was already outside the module tree and is still unwired. Two #[cfg(test)] copies can no longer compile. One doc command names a test target that is now only a suite filter.
Issue counts by severity
- bugs: 4
- suggestions: 2
- nits: 1
| &self, | ||
| addr: crate::NetAddr, | ||
| ) -> Result<TcpStream, NetError> { | ||
| pub async fn connect_isolated_net(&self, addr: crate::NetAddr) -> Result<TcpStream, NetError> { |
There was a problem hiding this comment.
[bug] Coverage on ac0d77c0d passes the 89% line gate at 91.26% (93410/102361) and then fails CRAP. The only function over 30 is Dialer::connect_isolated_net (CRAP 43.2, complexity 9). The body matches master aside from pub(crate) becoming pub. The only production caller is still ephemeral.rs. Nothing in the suite calls this method; the socks tests call dial_isolated. The onion, i2p, and cjdns arms stay dark, which is enough to push CRAP from under 30 on master to 43.2 once the suite links the normal rlib.
Suggestion: Exercise the NetAddr arms through the ephemeral caller (IP, onion with and without SOCKS, i2p, cjdns) from the net suite. Do not add the function to .cargo-crap.toml. A test whose only job is to move the CRAP input is not the fix.
There was a problem hiding this comment.
Isolated broadcast now drives the onion and i2p refusals through send_tx_isolated. The same test also dials a clearnet address, an onion host through SOCKS, and a cjdns address to a local listener. Those are the arms connect_isolated_net has. It is not on the CRAP allowlist.
| use std::path::{Path, PathBuf}; | ||
|
|
||
| #[test] | ||
| fn legacy_names_rename_and_both_exist_errors() { |
There was a problem hiding this comment.
[bug] legacy_names_rename_and_both_exist_errors ran on master inside the library #[cfg(test)] module. It is now the only test in this file, and tests/store/mod.rs never declares mod legacy_rename_tests. With autotests = false and [lib] test = false, cargo test --workspace does not compile this file. The open-path renames still run. The "both exist" StoreError::Layout assert does not.
Suggestion: Add mod legacy_rename_tests; to tests/store/mod.rs.
There was a problem hiding this comment.
legacy_names_rename_and_both_exist_errors is a module of the store suite again. It still renames the legacy stems and still returns StoreError::Layout when both the old and new files exist.
|
|
||
| #[test] | ||
| #[should_panic(expected = "i2p destination base64 contains")] | ||
| fn i2p_b32_rejects_unknown_base64() { |
There was a problem hiding this comment.
[bug] i2p_b32_rejects_unknown_base64, i2p_b32_rejects_a_short_destination, and i2p_b32_matches_the_address_host were #[cfg(test)] tests on the rbitcoin-test library, so cargo test --workspace ran them. They now live only in tests/overlay.rs, whose [[test]] has required-features = ["overlay"]. That feature is not default, and i2p_b32_from_dest itself sits behind #[cfg(feature = "overlay")]. Default CI therefore skips the unknown-base64 panic, the short-destination panic, and the 387-byte identity host assert.
Suggestion: Move i2p_b32_from_dest and these three tests onto the default suite. Leave the live Tor, i2p, and cjdns journeys on the overlay target. Linking the tests from suite.rs without moving the helper will not compile.
There was a problem hiding this comment.
The three i2p b32 checks run in the default suite. i2p_b32_from_dest moved out of the overlay feature so they compile without it. The live Tor, i2p, and cjdns journeys stay on the overlay target. The host assert is unchanged.
| @@ -328,7 +337,8 @@ fn connect_script_block( | |||
| /// `from_height` above 0 is that window and is served above the create cap. | |||
| /// Open history, and a window that still starts at height 0, stay capped. | |||
| #[test] | |||
There was a problem hiding this comment.
[bug] from_height_window_is_served_above_the_create_cap, from_height_ignores_a_spend_of_another_output, and electrum_sh_join live in this file. src/lib.rs does not declare the module, and no tests file includes it, so they never compile. This was already true on the base. This PR made the items pub and #[doc(hidden)] and still did not attach the file. The coverage job lists this file as analyzed source with no LCOV entry. tests/server covers a smaller from_height parser, not these three journeys.
Suggestion: include! the file from the electrum suite, or delete it if those pins are intentionally dropped.
There was a problem hiding this comment.
electrum_sh_journey.rs is part of the electrum suite now. The from_height window, the spend of another output, and the join journey all run. The file is no longer a source module the suite never included.
| @@ -2130,7 +2140,7 @@ fn check_meta(dir: &Path) -> Result<u16, StoreError> { | |||
| } | |||
|
|
|||
| /// One wipe-and-IBD line for every store `meta` older than schema 22. | |||
| const PRE22_WIPE: &str = "schema before 22 refuses this datadir; wipe datadir and redo IBD"; | |||
| pub const PRE22_WIPE: &str = "schema before 22 refuses this datadir; wipe datadir and redo IBD"; | |||
|
|
|||
| #[cfg(test)] | |||
There was a problem hiding this comment.
[suggestion] [lib] test = false strips #[cfg(test)] mod tests here and the same module in crates/rbitcoin-node/src/config.rs (line 1663). The function names match the suite copies, so the live tests still run. Rustc and clippy do not typecheck these copies, so they can rot beside the suite.
Suggestion: Delete both #[cfg(test)] modules. Keep the tests/ copies.
There was a problem hiding this comment.
The cfg(test) copies in store.rs and config.rs are gone. The suite already had the same cases, and those are the ones that compile. Nothing those modules tested was dropped.
| cargo test --manifest-path script-oracle/Cargo.toml --quiet | ||
| cargo test -p rbitcoin-consensus --lib block_866342 -- --nocapture | ||
| cargo test -p rbitcoin-consensus --test suite block_866342 -- --nocapture | ||
| cargo test -p rbitcoin-consensus --test script_edge_fixtures |
There was a problem hiding this comment.
[suggestion] cargo test -p rbitcoin-consensus --test script_edge_fixtures is not a target. The only consensus [[test]] is suite. script_edge_fixtures is a module of that suite. The other commands in this block already use --test suite. An operator following this line gets "no test target" and does not run that subset.
Suggestion: Change the line to cargo test -p rbitcoin-consensus --test suite script_edge.
There was a problem hiding this comment.
That line is now cargo test -p rbitcoin-consensus --test suite script_edge. script_edge_fixtures is a filter of the suite, not its own target.
| @@ -0,0 +1,4 @@ | |||
| Changed | |||
|
|
|||
| - **Library tests compile against the normal rlib.** Each library package has one `tests/suite` target (`[lib] test = false`), so `cargo test` no longer rebuilds the crate under `--cfg test`. `test-hooks` is gone. Hooks a test arms stay `cfg(debug_assertions)`. A debug node still uses the 8 s outbound connect timeout and still aborts when an io_uring drain leaves SQEs. Initial block download is `rbitcoin-ibd`, and RPC, Electrum, Esplora, and SV2 do not depend on it. The consensus suite is the package's default `corpus` feature (the Core script assembler). Product crates depend on consensus with `default-features = false`, so a node build leaves that assembler out. `rbitcoin-test`'s default scenarios are one `suite` binary; `overlay` stays feature-gated. `NODE_COMPACT_FILTERS` is advertised per peer hub, so one process can run more than one node. | |||
There was a problem hiding this comment.
[nit] The fragment says each library package has one tests/suite and [lib] test = false. rbitcoin-bench still has #[cfg(test)] modules under src/ and no suite target. CI still runs cargo test -p rbitcoin-bench --lib, which is intentional. rbitcoin-ibd sets test = false but not doctest = false. It has no doc-test fences, so the extra harness is empty.
Suggestion: Name bench as the remaining --lib crate, and set doctest = false on rbitcoin-ibd or narrow the sentence.
There was a problem hiding this comment.
The note now says bench is the library that still tests with --lib, and rbitcoin-ibd sets doctest = false like the other libraries. The floor in that note is 91%. The last full run measured 91.26% (93410/102361), and the unrounded gate still leaves room for the usual line jitter.
Move each library's tests into one suite target so cargo test links the dev rlib, and split initial block download into rbitcoin-ibd so RPC and Electrum do not build it. Co-authored-by: Cursor <cursoragent@cursor.com>
The suite target dropped the inner tests module, and rbitcoin-test compiles its journeys from suite.rs. The old paths failed qc and the Windows smoke.
Suite targets compile the normal rlib, so doc(hidden) items are public to clippy. Add is_empty and Default, allow the () sentinels callers already map to a miss, and drop the test-hooks dev-deps. Those hooks are debug_assertions items on the rlib, and the features are gone.
Suite tests link the normal rlib, so cfg(test) lines leave the production LCOV. This run is 89.71% (91583/102091), short of 90%. 89% still clears with about 720 lines of jitter room.
Twelve master tests never landed in the suite, and three moved tests lost assertions. The rbitcoin-test helper tests now compile in the suite and the overlay binary.
Mapping cfg(test) onto debug_assertions made a debug node dial with a 2s connect timeout and return from an undrained io_uring instead of aborting. The suite still observes that error inside with_drain_hard_cap.
structure_rule_tests and the stale-nversion pin live in the query suite. The coverage job comment matches the 89% floor.
The suite needs the corpus feature, so workspace test skipped it. Script lines then scored as uncovered and the CRAP gate failed. corpus is the consensus package default. Product crates keep default-features off, so a node build still omits the assembler.
send_tx_isolated refuses onion without SOCKS and i2p without SAM. connect_isolated_net also dials a clearnet address, an onion host through SOCKS, and a cjdns address.
The both-exist layout error is part of the store suite again. The cfg(test) modules in store and node config no longer compile, and the suite already has those cases.
i2p_b32_from_dest is no longer behind the overlay feature. The three host checks run with cargo test. Live mesh journeys stay on overlay.
The from_height window, the other-output spend, and the join scenario compile in the electrum suite. They were a source file the suite never included.
script_edge_fixtures is a module of the suite target, not its own cargo test binary.
The suite-on-rlib run measured 91.26% (93410/102361). The gate stays unrounded LH*100 >= LF*91, which still leaves room for llvm-cov jitter. rbitcoin-ibd turns doctests off. Bench remains the --lib exception.
042c6b1 to
351591d
Compare
There was a problem hiding this comment.
Summary
Ready to merge. Head 351591d75 is mergeable onto d850f041d. Required checks on that commit are green: qc, test, windows, macos, and coverage.
This pass checked that every issue from the earlier reviews is still fixed after the rebase onto the empty catch-up pool RPC change. No new issue.
Disposition
Closed:
- Isolated dial coverage.
connect_isolated_net_dials_ip_and_onion_and_rejects_the_restdrives onion and i2p refusals throughsend_tx_isolated, and dials clearnet, onion-via-SOCKS, and cjdns. The function is not on the CRAP allowlist. Coverage on this commit passed, so that gate is clear. - Legacy rename.
legacy_names_rename_and_both_exist_errorsismod legacy_rename_testsin the store suite. - I2P b32 checks. They live in
rbitcoin-test's default suite.i2p_b32_from_destis no longer behind the overlay feature. Live mesh journeys stay onoverlay. - Electrum scripthash journey.
electrum_sh_journeyis a module of the electrum suite. - Dead
#[cfg(test)]copies instore.rsandconfig.rsare gone. docs/consensus-tests.mdrunsscript_edgeas a suite filter.- Line-coverage floor is 91% (
FLOOR_PCT = 91). The changelog names bench as the remaining--libcrate, andrbitcoin-ibdsetsdoctest = false. - Earlier fixes still hold: outbound connect is 8s in every build, an undrained io_uring drain aborts unless the thread is inside
with_drain_hard_cap, and the short redial, SAM retry, and SV2 cooldown stay debug-only (compiled_with_test_hooksiscfg!(debug_assertions)). - The rebase kept the master behavior that an empty catch-up pool is served.
relay_off_empty_mempool_is_listed,relay_off_empty_pool_serves_template, andblocksonly_lists_mempool_mines_and_refuses_templateare in the RPC suite.
Not a merge blocker:
- The three high CodeQL
uncontrolled-allocation-sizealerts onreconstruct.rs(329, 330, 331) are false positives. Eachwith_capacityuses the length of records already loaded from this node's store. Dismiss them. The cleartext-logging hits in test files are the same codec-name false positive already dismissed elsewhere.
Issue counts by severity
- bugs: 0
- suggestions: 0
- nits: 0
#1097 mapped cfg(test) onto cfg(debug_assertions). Two of those gates landed on items production code also uses, so `cargo check --release` failed in rbitcoin-store (lib) and, once that was patched, rbitcoin-query: - scripthash_head.rs gated `use crate::open_address::{self, MAX_LOAD_DEN, MAX_LOAD_NUM}` while `hash_slot` and the bulk-fill load cap use it. - confirm_run/mod.rs re-exported `expected_bits_extending` and `ensure_spend_abs_layouts`, both debug-only, from ungated `pub use`. No PR job compiles the release profile; release.yml and nix do. `qc` now runs `cargo check --release --workspace --exclude rbitcoin-bench` after clippy, and coverage.test.sh pins that step the way it pins the test job's commands.
Every tests/suite.rs opened with `#![allow(unused_imports, dead_code, unused_variables)]`, which switched the compiler's warnings off for all test code. With the allow gone the workspace reported ~2000 unused imports (all from the src/ -> tests/ move; `cargo fix` removed them), ~20 unused trait imports (by hand), and one dead helper: the `RBITCOIN_BLOCK_QUEUE_*` env lock in ibd/assign.rs, which #1097 replaced with a config field. No unused variables. Imports used only inside a Windows or Darwin cfg block were checked by hand; none were removed. Co-authored-by: Cursor <cursoragent@cursor.com>
…out move #1097 set the floor to 89 and then 91; the badge kept `--gate 89` in coverage.sh and as the coverage-badge.py default, so a run between 89 and 91 would draw a passing badge on a failing gate. coverage.test.sh now reads FLOOR_PCT from coverage-gate.py and pins both to it. The IGNORE regex fixtures in coverage.test.sh name current files (tests/ and rbitcoin-ibd) instead of the src/*_tests.rs they described before the move. docs/concurrency.md links perf_log.rs where it lives now. Five rustdoc links to ChainHub and IndexMode, which rpc and node cannot resolve since IBD moved to rbitcoin-ibd, are plain code spans. Co-authored-by: Cursor <cursoragent@cursor.com>
…in hooks After #1097 the suite reaches internals through `#[doc(hidden)] pub` and test hooks are `cfg(debug_assertions)`, but CONTRIBUTING principle 11 still said to drop to `pub(crate)`, that `-D warnings` would report dead `pub`, and that a `cfg(test)` hook is a smell. The rule now states the convention and its two limits: a hook is off until a test arms it, and a debug gate never covers an import production code uses (the release check in `qc` pins that). code-shape points at it from the lint section and notes suite roots carry no blanket allow. TESTING.md records that the suites build in dev/test only. rbitcoin-ibd gets the AGENTS.md the other crates have. Co-authored-by: Cursor <cursoragent@cursor.com>
#1097 mapped cfg(test) onto cfg(debug_assertions). Two of those gates landed on items production code also uses, so `cargo check --release` failed in rbitcoin-store (lib) and, once that was patched, rbitcoin-query: - scripthash_head.rs gated `use crate::open_address::{self, MAX_LOAD_DEN, MAX_LOAD_NUM}` while `hash_slot` and the bulk-fill load cap use it. - confirm_run/mod.rs re-exported `expected_bits_extending` and `ensure_spend_abs_layouts`, both debug-only, from ungated `pub use`. No PR job compiles the release profile; release.yml and nix do. `qc` now runs `cargo check --release --workspace --exclude rbitcoin-bench` after clippy, and coverage.test.sh pins that step the way it pins the test job's commands.
Every tests/suite.rs opened with `#![allow(unused_imports, dead_code, unused_variables)]`, which switched the compiler's warnings off for all test code. With the allow gone the workspace reported ~2000 unused imports (all from the src/ -> tests/ move; `cargo fix` removed them), ~20 unused trait imports (by hand), and one dead helper: the `RBITCOIN_BLOCK_QUEUE_*` env lock in ibd/assign.rs, which #1097 replaced with a config field. No unused variables. Imports used only inside a Windows or Darwin cfg block were checked by hand; none were removed. Co-authored-by: Cursor <cursoragent@cursor.com>
…out move #1097 set the floor to 89 and then 91; the badge kept `--gate 89` in coverage.sh and as the coverage-badge.py default, so a run between 89 and 91 would draw a passing badge on a failing gate. coverage.test.sh now reads FLOOR_PCT from coverage-gate.py and pins both to it. The IGNORE regex fixtures in coverage.test.sh name current files (tests/ and rbitcoin-ibd) instead of the src/*_tests.rs they described before the move. docs/concurrency.md links perf_log.rs where it lives now. Five rustdoc links to ChainHub and IndexMode, which rpc and node cannot resolve since IBD moved to rbitcoin-ibd, are plain code spans. Co-authored-by: Cursor <cursoragent@cursor.com>
…in hooks After #1097 the suite reaches internals through `#[doc(hidden)] pub` and test hooks are `cfg(debug_assertions)`, but CONTRIBUTING principle 11 still said to drop to `pub(crate)`, that `-D warnings` would report dead `pub`, and that a `cfg(test)` hook is a smell. The rule now states the convention and its two limits: a hook is off until a test arms it, and a debug gate never covers an import production code uses (the release check in `qc` pins that). code-shape points at it from the lint section and notes suite roots carry no blanket allow. TESTING.md records that the suites build in dev/test only. rbitcoin-ibd gets the AGENTS.md the other crates have. Co-authored-by: Cursor <cursoragent@cursor.com>
fix: release build, opt-in spindle trace, suite lint after the #1097 layout move
2 participants
Summary
tests/suitetarget and[lib] test = false, socargo testlinks the dev rlib instead of rebuilding the crate under--cfg test.test-hooksis gone. The same hooks arecfg(debug_assertions), which dev and test builds already share.rbitcoin-ibd. RPC, Electrum, Esplora, and SV2 depend onrbitcoin-netonly. The node depends on IBD.rbitcoin-testdefault scenarios are onesuitebinary. Overlay stays feature-gated.NODE_COMPACT_FILTERSis advertised per peer hub so one process can run more than one node.Test plan
cargo test --workspace --exclude rbitcoin-benchlocallyqc,test,windows,macos, andcoverageon this PRMade with Cursor