feat(checksig-census): capture cumulative script evidence - #54
Conversation
Emit canonical context, journal, and operation streams with fixed cumulative counters and fail-closed native sink handling.
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe change adds 24-counter census instrumentation, streamed record, journal, and context outputs, filtered corpus capture with JSONL sidecars, and configurable kernel verification extraction with schema v2 reports. ChangesCensus verification pipeline
Sequence Diagram(s)sequenceDiagram
participant CaptureHarness
participant BitcoinKernel
participant CensusState
participant CensusArtifacts
participant ContextSidecar
CaptureHarness->>BitcoinKernel: verify selected block transactions
BitcoinKernel->>CensusState: record operation and verification outcome
CensusState->>CensusArtifacts: stream counters, records, and journals
BitcoinKernel->>ContextSidecar: write transaction and input context
CaptureHarness->>CensusArtifacts: flush census outputs
CaptureHarness->>CaptureHarness: write summary and artifact hashes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tools/checksig-census/instrumentation.diff (2)
891-943: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winNarrowing
size()touint8_tcorrupts records and can abort the downstream tool.
(uint8_t)vchSig.size()and(uint8_t)vchPubKey.size()truncate silently. Script stack elements go up to 520 bytes, and the C150 range 0..150000 is pre-BIP66, so non-DER oversized pushes do reachCheckECDSASignature. Two failure modes follow:
- A 300-byte pubkey becomes
pubkey_len == 44. The record looks valid and is wrong.tools/checksig-census/bare-secp/src/main.rsaccepts it and feeds 44 bytes of a 65-byte truncated buffer to the benchmark.- A 100-byte signature stays 100, which exceeds 72.
parse_recordintools/checksig-census/bare-secp/src/main.rs(Line 359) then aborts the entire run on a record the kernel itself wrote.Clamp explicitly at the emission site and mark the truncation so the analyzer can see it, instead of pretending the length fit.
🐛 Sketch: clamp instead of narrowing
++// Clamp oversized script elements; the record buffers hold 72/65 bytes. ++static inline uint8_t census_clamp_len(size_t n, size_t cap) { ++ return static_cast<uint8_t>(n > cap ? cap : n); ++}Then replace each
(uint8_t)vchSig.size()withcensus_clamp_len(vchSig.size(), 72)and each(uint8_t)vchPubKey.size()withcensus_clamp_len(vchPubKey.size(), 65). Document the clamp in the README records section so a clamped length is not read as an exact length.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/checksig-census/instrumentation.diff` around lines 891 - 943, Replace every direct uint8_t cast of vchSig.size() and vchPubKey.size() in the census_emit_record calls within CheckECDSASignature with explicit census_clamp_len limits of 72 and 65 respectively, and propagate a truncation indicator in the record format so analyzers can distinguish clamped lengths from exact lengths. Update the README records documentation to describe the clamping semantics.
151-215: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDo not let C++ exceptions escape this
extern "C"function.
countis a caller-supplieduint64_t.reserve(count)throwsstd::length_errororstd::bad_allocfor large or absurd values, andemplace_backcan throw too. This function has notry/catch. The rest of bitcoinkernel.cpp wraps exported bodies intry/catchfor exactly this reason. An exception unwinding across the C ABI into Rust is undefined behavior, not a clean error.Bound
countand catch everything.🐛 Proposed fix: fail closed instead of unwinding
+int btck_bare_verify_bench(const btck_bare_input* inputs, uint64_t count, + uint32_t warmup_rounds, uint32_t timed_rounds, + uint32_t mode, btck_bare_result* result) +{ + if (mode != 0 || inputs == nullptr || result == nullptr || count == 0) return -1; + if (timed_rounds == 0 || timed_rounds > 64) return -1; ++ if (count > (std::numeric_limits<size_t>::max() / sizeof(CPubKey))) return -1; ++ try { + // Prebuild CPubKey, DER vectors, and uint256 outside timing ... + result->ok_count = ok_count; + + return 0; ++ } catch (...) { ++ return -1; ++ } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/checksig-census/instrumentation.diff` around lines 151 - 215, Prevent exceptions from escaping btck_bare_verify_bench by validating count against a safe implementation limit before reserve or allocation, then wrapping the full body—including vector construction and verification—in try/catch. Return the existing failure code for oversized counts and any caught exception, preserving normal result population and return behavior on success.
🧹 Nitpick comments (1)
tools/checksig-census/instrumentation.diff (1)
306-335: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winThe 24-counter contract is a magic number on both sides of the FFI with nothing enforcing it. The kernel enum, the kernel constant, and the Rust snapshot buffer each spell out
24independently. Nothing fails when they diverge; the snapshot just truncates or pads with zeros and the invariant reports success on data it never read.
tools/checksig-census/instrumentation.diff#L306-L335: addstatic_assert(static_cast<size_t>(C_CENSUS_COUNT) == CENSUS_COUNTER_COUNT, ...)so the enum and the constant cannot drift at build time.tools/checksig-census/bare-secp/src/main.rs#L129-L131: size the buffer fromCOUNTER_NAMES.len(), passcounters.len()tobtck_census_snapshot, andbail!whenbtck_census_counter_count()disagrees.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tools/checksig-census/instrumentation.diff` around lines 306 - 335, Enforce the counter-count contract at both sites: in tools/checksig-census/instrumentation.diff lines 306-335, add a static assertion that C_CENSUS_COUNT equals CENSUS_COUNTER_COUNT; in tools/checksig-census/bare-secp/src/main.rs lines 129-131, size the snapshot buffer from COUNTER_NAMES.len(), pass counters.len() to btck_census_snapshot, and bail when btck_census_counter_count() differs from the expected count.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tools/checksig-census/bare-secp/src/main.rs`:
- Around line 129-131: Update the census snapshot logic in main to size counters
from COUNTER_NAMES.len() instead of hardcoding 24, query the native count with
btck_census_counter_count(), and assert that it matches the Rust buffer length
before calling btck_census_snapshot. Preserve the existing snapshot and
invariant checks after validating the count.
In `@tools/checksig-census/capture/src/main.rs`:
- Around line 76-87: Update the capture argument setup around args.counters and
args.journal to resolve each configured path from the corresponding BRS_CENSUS_*
environment variable when the CLI option is absent, validate and prepare the
effective path once, and use it for native sink configuration. Ensure the
summary serialization records these effective paths instead of null when
environment variables configure the sinks.
- Around line 69-87: In tools/checksig-census/capture/src/main.rs lines 69-87,
add canonical path resolution and validation before ensure_parent, native sink
setup, or any artifact creation: compare the corpus, summary, sidecar, counters,
and journal paths and reject aliases that resolve to the same location. In
crates/consensus/examples/kernel_verify_spike.rs lines 78-95, perform the
equivalent corpus-versus-report canonical path comparison before extraction or
report output; both sites must reject conflicting artifact paths before writing.
In `@tools/checksig-census/instrumentation.diff`:
- Around line 953-1000: Reconcile census classification across the Schnorr and
ECDSA instrumentation, using reject_reason and verification outcome rather than
sig_version to identify record types. Ensure only records corresponding to
actual VerifySchnorrSignature or ECDSA verification calls use outcome 0 or 1,
while rejected pre-verification records use the designated reject classification
(including reason 8) so total_records matches the required accounting.
In `@tools/checksig-census/README.md`:
- Around line 106-149: Update the later “Validate census + cross-check” section
to reference the Run A outputs under the c150 names, including
c150.counters.json and c150.journal.bin. Update the “Output artifacts” tree to
list c150.counters.json, c150.contexts.bin, c150.records.bin, c150.journal.bin,
c150.replay.json, and c150.classification.json, removing the obsolete
census-0-150k and census-replay names.
---
Outside diff comments:
In `@tools/checksig-census/instrumentation.diff`:
- Around line 891-943: Replace every direct uint8_t cast of vchSig.size() and
vchPubKey.size() in the census_emit_record calls within CheckECDSASignature with
explicit census_clamp_len limits of 72 and 65 respectively, and propagate a
truncation indicator in the record format so analyzers can distinguish clamped
lengths from exact lengths. Update the README records documentation to describe
the clamping semantics.
- Around line 151-215: Prevent exceptions from escaping btck_bare_verify_bench
by validating count against a safe implementation limit before reserve or
allocation, then wrapping the full body—including vector construction and
verification—in try/catch. Return the existing failure code for oversized counts
and any caught exception, preserving normal result population and return
behavior on success.
---
Nitpick comments:
In `@tools/checksig-census/instrumentation.diff`:
- Around line 306-335: Enforce the counter-count contract at both sites: in
tools/checksig-census/instrumentation.diff lines 306-335, add a static assertion
that C_CENSUS_COUNT equals CENSUS_COUNTER_COUNT; in
tools/checksig-census/bare-secp/src/main.rs lines 129-131, size the snapshot
buffer from COUNTER_NAMES.len(), pass counters.len() to btck_census_snapshot,
and bail when btck_census_counter_count() differs from the expected count.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e4be263-1692-45e5-b0c7-dccf941b0d49
📒 Files selected for processing (5)
crates/consensus/examples/kernel_verify_spike.rstools/checksig-census/README.mdtools/checksig-census/bare-secp/src/main.rstools/checksig-census/capture/src/main.rstools/checksig-census/instrumentation.diff
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: test
- GitHub Check: bench-smoke
🧰 Additional context used
🔍 Remote MCP Context7, Github Grep
Additional review context
- Bitcoin Core’s
bitcoinkerneltarget includes the script interpreter and consensus-validation sources; its documentation describes migration intolibbitcoin_kernelas ongoing. The new instrumentation therefore sits on an upstream-sensitive boundary where ABI/build compatibility merits verification. - Bitcoin Core’s secp256k1 contract accepts only low-S ECDSA signatures; normalization is explicitly described as potentially reintroducing malleability. Census classification should preserve this distinction between rejected and successfully verified signatures.
- GitHub search found no public matches for the PR-specific symbols (
btck_census_*,btck_bare_verify_bench,CHECKSIG_CENSUS, orschnorr_verify_ok), so there is no independent upstream implementation to use for compatibility comparison.
🔇 Additional comments (7)
tools/checksig-census/capture/src/main.rs (1)
177-229: LGTM!Also applies to: 335-383, 499-503
crates/consensus/examples/kernel_verify_spike.rs (2)
105-194: LGTM!Also applies to: 239-363, 608-636
198-202: 🎯 Functional CorrectnessDo not add a BIP16 height gate.
compute_verify_flagsintentionally enablesP2SHfor every block except the hash-pinned exception, and both helpers match this production behavior.> Likely an incorrect or invalid review comment.tools/checksig-census/instrumentation.diff (2)
236-273: LGTM!Also applies to: 1026-1080
68-106: 🗄️ Data Integrity & IntegrationNo census guard is needed. Run A builds with
kernelby default. Its Rayon workers callcheck_input, which callsbitcoinkernel::verifyfor each input. That native API entersbtck_script_pubkey_verify, wheretl_censusis initialized beforeVerifyScript.> Likely an incorrect or invalid review comment.tools/checksig-census/bare-secp/src/main.rs (1)
30-55: LGTM!Also applies to: 69-69
tools/checksig-census/README.md (1)
62-62: LGTM!Also applies to: 175-177, 224-224, 402-402, 435-455
| let mut counters = [0u64; 24]; | ||
| unsafe { | ||
| libbitcoinkernel_sys::btck_census_snapshot(counters.as_mut_ptr(), 22); | ||
| libbitcoinkernel_sys::btck_census_snapshot(counters.as_mut_ptr(), 24); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Stop hardcoding 24; ask the kernel.
btck_census_counter_count() exists in the ABI and this code ignores it. If the kernel grows to 26 counters, the snapshot silently reads 24 and INV-15 declares "all counters zero" for counters it never looked at. If the kernel shrinks, census_snapshot copies fewer values and the tail stays zero for the same reason. Either way the invariant lies.
Derive the buffer from COUNTER_NAMES.len() and assert against the native count.
🛡️ Proposed fix
// Verify counters stayed zero.
- let mut counters = [0u64; 24];
+ let native_count = unsafe { libbitcoinkernel_sys::btck_census_counter_count() };
+ if native_count != COUNTER_NAMES.len() {
+ bail!(
+ "census counter ABI mismatch: kernel reports {native_count}, tool knows {}",
+ COUNTER_NAMES.len()
+ );
+ }
+ let mut counters = [0u64; COUNTER_NAMES.len()];
unsafe {
- libbitcoinkernel_sys::btck_census_snapshot(counters.as_mut_ptr(), 24);
+ libbitcoinkernel_sys::btck_census_snapshot(counters.as_mut_ptr(), counters.len());
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let mut counters = [0u64; 24]; | |
| unsafe { | |
| libbitcoinkernel_sys::btck_census_snapshot(counters.as_mut_ptr(), 22); | |
| libbitcoinkernel_sys::btck_census_snapshot(counters.as_mut_ptr(), 24); | |
| // Verify counters stayed zero. | |
| let native_count = unsafe { libbitcoinkernel_sys::btck_census_counter_count() }; | |
| if native_count != COUNTER_NAMES.len() { | |
| bail!( | |
| "census counter ABI mismatch: kernel reports {native_count}, tool knows {}", | |
| COUNTER_NAMES.len() | |
| ); | |
| } | |
| let mut counters = [0u64; COUNTER_NAMES.len()]; | |
| unsafe { | |
| libbitcoinkernel_sys::btck_census_snapshot(counters.as_mut_ptr(), counters.len()); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/checksig-census/bare-secp/src/main.rs` around lines 129 - 131, Update
the census snapshot logic in main to size counters from COUNTER_NAMES.len()
instead of hardcoding 24, query the native count with
btck_census_counter_count(), and assert that it matches the Rust buffer length
before calling btck_census_snapshot. Preserve the existing snapshot and
invariant checks after validating the count.
| // Resolve the sidecar path before anything else so we can fail early. | ||
| let sidecar_path = args | ||
| .context_sidecar | ||
| .clone() | ||
| .unwrap_or_else(|| resolve_default_sidecar(&args.corpus, args.output.as_ref())); | ||
| ensure_parent(&sidecar_path)?; | ||
|
|
||
| // The harness is still single-threaded here. Configure the native sinks | ||
| // before the kernel or any worker pool can read the process environment. | ||
| if let Some(path) = &args.counters { | ||
| ensure_parent(path)?; | ||
| // SAFETY: no other thread can access the environment before this call. | ||
| unsafe { std::env::set_var(CENSUS_COUNTERS_ENV, path.as_os_str()) }; | ||
| } | ||
| if let Some(path) = &args.journal { | ||
| ensure_parent(path)?; | ||
| // SAFETY: no other thread can access the environment before this call. | ||
| unsafe { std::env::set_var(CENSUS_JOURNAL_ENV, path.as_os_str()) }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject artifact-path aliases before writing evidence. Both tools treat caller-provided artifact paths as independent. Aliases can overwrite the corpus or corrupt generated evidence.
tools/checksig-census/capture/src/main.rs#L69-L87: resolve and compare the corpus, summary, sidecar, counters, and journal paths before any create or native sink setup.crates/consensus/examples/kernel_verify_spike.rs#L78-L95: resolve and compare the corpus and report paths before extraction or report output.
📍 Affects 2 files
tools/checksig-census/capture/src/main.rs#L69-L87(this comment)crates/consensus/examples/kernel_verify_spike.rs#L78-L95
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/checksig-census/capture/src/main.rs` around lines 69 - 87, In
tools/checksig-census/capture/src/main.rs lines 69-87, add canonical path
resolution and validation before ensure_parent, native sink setup, or any
artifact creation: compare the corpus, summary, sidecar, counters, and journal
paths and reject aliases that resolve to the same location. In
crates/consensus/examples/kernel_verify_spike.rs lines 78-95, perform the
equivalent corpus-versus-report canonical path comparison before extraction or
report output; both sites must reject conflicting artifact paths before writing.
| // The harness is still single-threaded here. Configure the native sinks | ||
| // before the kernel or any worker pool can read the process environment. | ||
| if let Some(path) = &args.counters { | ||
| ensure_parent(path)?; | ||
| // SAFETY: no other thread can access the environment before this call. | ||
| unsafe { std::env::set_var(CENSUS_COUNTERS_ENV, path.as_os_str()) }; | ||
| } | ||
| if let Some(path) = &args.journal { | ||
| ensure_parent(path)?; | ||
| // SAFETY: no other thread can access the environment before this call. | ||
| unsafe { std::env::set_var(CENSUS_JOURNAL_ENV, path.as_os_str()) }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Record effective native sink paths.
When callers configure BRS_CENSUS_COUNTERS or BRS_CENSUS_JOURNAL directly, args.counters and args.journal remain None. The summary then reports null for artifacts that the kernel actually wrote. This breaks evidence provenance.
Resolve the environment fallback once, validate it, and serialize the effective paths in the summary.
Also applies to: 251-254
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/checksig-census/capture/src/main.rs` around lines 76 - 87, Update the
capture argument setup around args.counters and args.journal to resolve each
configured path from the corresponding BRS_CENSUS_* environment variable when
the CLI option is absent, validate and prepare the effective path once, and use
it for native sink configuration. Ensure the summary serialization records these
effective paths instead of null when environment variables configure the sinks.
| @@ -1723,21 +1791,43 @@ | ||
| // abort script execution). This is implemented in EvalChecksigTapscript, which won't invoke | ||
| // CheckSchnorrSignature in that case. In other contexts, they are invalid like every other signature with | ||
| // size different from 64 or 65. | ||
| if (sig.size() != 64 && sig.size() != 65) return set_error(serror, SCRIPT_ERR_SCHNORR_SIG_SIZE); | ||
| - if (sig.size() != 64 && sig.size() != 65) return set_error(serror, SCRIPT_ERR_SCHNORR_SIG_SIZE); | ||
| + | ||
| + const uint8_t sig_ver_byte = sigversion == SigVersion::TAPSCRIPT ? 2 : 3; | ||
| + const uint8_t captured_sig_len = sig.size() > 72 ? 72 : static_cast<uint8_t>(sig.size()); | ||
| + if (sig.size() != 64 && sig.size() != 65) { | ||
| + census_emit_record(tl_census.cur_op, sig_ver_byte, 2, captured_sig_len, 32, 0, 4, | ||
| + nullptr, sig.data(), pubkey_in.data()); | ||
| + return set_error(serror, SCRIPT_ERR_SCHNORR_SIG_SIZE); | ||
| + } | ||
|
|
||
| XOnlyPubKey pubkey{pubkey_in}; | ||
| @@ -1994,20 +2057,21 @@ static bool VerifyWitnessProgram(const CScriptWitness& witness, int witversion, | ||
| return set_error(serror, SCRIPT_ERR_DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM); | ||
| } | ||
| // Other version/size/p2sh combinations return true for future softfork compatibility | ||
| return true; | ||
|
|
||
| uint8_t hashtype = SIGHASH_DEFAULT; | ||
| if (sig.size() == 65) { | ||
| hashtype = SpanPopBack(sig); | ||
| - if (hashtype == SIGHASH_DEFAULT) return set_error(serror, SCRIPT_ERR_SCHNORR_SIG_HASHTYPE); | ||
| - } | ||
| + if (hashtype == SIGHASH_DEFAULT) { | ||
| + census_emit_record(tl_census.cur_op, sig_ver_byte, 2, 64, 32, hashtype, 5, | ||
| + nullptr, sig.data(), pubkey_in.data()); | ||
| + return set_error(serror, SCRIPT_ERR_SCHNORR_SIG_HASHTYPE); | ||
| + } | ||
| + } | ||
| + if (!this->txdata) { | ||
| + census_emit_record(tl_census.cur_op, sig_ver_byte, 2, 64, 32, hashtype, 6, | ||
| + nullptr, sig.data(), pubkey_in.data()); | ||
| + return HandleMissingData(m_mdb); | ||
| + } | ||
| + | ||
| uint256 sighash; | ||
| - if (!this->txdata) return HandleMissingData(m_mdb); | ||
| if (!SignatureHashSchnorr(sighash, execdata, *txTo, nIn, hashtype, sigversion, *this->txdata, m_mdb)) { | ||
| + census_emit_record(tl_census.cur_op, sig_ver_byte, 2, 64, 32, hashtype, 7, | ||
| + nullptr, sig.data(), pubkey_in.data()); | ||
| return set_error(serror, SCRIPT_ERR_SCHNORR_SIG_HASHTYPE); | ||
| } | ||
| // There is intentionally no return statement here, to be able to use "control reaches end of non-void function" warnings to detect gaps in the logic above. | ||
| - if (!VerifySchnorrSignature(sig, pubkey, sighash)) return set_error(serror, SCRIPT_ERR_SCHNORR_SIG); | ||
| + | ||
| + const bool schnorr_ok = VerifySchnorrSignature(sig, pubkey, sighash); | ||
| + census_emit_record(tl_census.cur_op, sig_ver_byte, schnorr_ok ? 1 : 0, 64, 32, hashtype, 0, | ||
| + sighash.begin(), sig.data(), pubkey_in.data()); | ||
| + if (!schnorr_ok) return set_error(serror, SCRIPT_ERR_SCHNORR_SIG); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether analyze.py invariants account for Schnorr records.
set -euo pipefail
rg -n -C6 'checkecdsa_entries|ecdsa_verify_calls|schnorr' tools/checksig-census/analyze.py
# Do any invariants partition records by sig_version or reject_reason?
rg -n -C4 'sig_version|reject_reason|outcome' tools/checksig-census/analyze.pyRepository: gosuda/bitcoin-rs
Length of output: 12962
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- analyzer counter and record definitions ---'
sed -n '1,210p' tools/checksig-census/analyze.py
printf '%s\n' '--- invariant implementation ---'
sed -n '300,390p' tools/checksig-census/analyze.py
printf '%s\n' '--- instrumentation counter updates and record emission ---'
rg -n -C5 'checkschnorr_entries|schnorr_verify_calls|census_emit_record|reject_reason|sig_ver_byte' tools/checksig-censusRepository: gosuda/bitcoin-rs
Length of output: 33129
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Schnorr entry and counter increments ---'
rg -n -C10 'C_CHECKSCHNORR_ENTRIES|C_SCHNORR_VERIFY_CALLS|C_SCHNORR_VERIFY_OK|C_SCHNORR_VERIFY_FAIL|CheckSchnorrSignature|EvalChecksigTapscript' tools/checksig-census/instrumentation.diff
printf '%s\n' '--- all record-emission sites ---'
rg -n -C3 'census_emit_record' tools/checksig-census/instrumentation.diffRepository: gosuda/bitcoin-rs
Length of output: 9287
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- tapscript dispatch and signature checker callers ---'
sed -n '730,810p' tools/checksig-census/instrumentation.diff
rg -n -C8 'CheckECDSASignature|CheckSchnorrSignature|EvalChecksigTapscript|pubkey.size\(\)' tools/checksig-census/instrumentation.diffRepository: gosuda/bitcoin-rs
Length of output: 7240
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
diff = Path("tools/checksig-census/instrumentation.diff").read_text()
analyzer = Path("tools/checksig-census/analyze.py").read_text()
required_source_facts = {
"ecdsa_entry": "census_inc(C_CHECKECDSA_ENTRIES);",
"schnorr_entry": "census_inc(C_CHECKSCHNORR_ENTRIES);",
"schnorr_verify": "census_inc(C_SCHNORR_VERIFY_CALLS);",
"skip_record": "tl_census.cur_op, 2, 2, 0, 32, 0, 8,",
}
for name, text in required_source_facts.items():
assert text in diff, f"missing source fact: {name}"
assert "outcome_01 == c.ecdsa_verify_calls" in analyzer
assert "total == c.checkecdsa_entries" in analyzer
# Model one mixed stream:
# ECDSA: 8 verification records + 2 pre-verification rejects.
# Schnorr: 2 verification records + 1 pre-verification reject.
# Tapscript skip: 1 reason-8 record emitted before CheckSchnorrSignature.
ecdsa_verify_calls = 8
checkecdsa_entries = 10
schnorr_verify_calls = 2
checkschnorr_entries = 3
reason8_records = 1
outcome_01 = ecdsa_verify_calls + schnorr_verify_calls
total_records = checkecdsa_entries + checkschnorr_entries + reason8_records
current_inv9 = (
outcome_01 == ecdsa_verify_calls
and total_records == checkecdsa_entries
)
corrected_inv9 = (
outcome_01 == ecdsa_verify_calls + schnorr_verify_calls
and total_records
== checkecdsa_entries + checkschnorr_entries + reason8_records
)
print({
"outcome_01": outcome_01,
"total_records": total_records,
"current_inv9_passes": current_inv9,
"corrected_inv9_passes": corrected_inv9,
})
assert not current_inv9
assert corrected_inv9
PYRepository: gosuda/bitcoin-rs
Length of output: 256
Reconcile all signature records in INV-9.
count(outcome in {0,1}) must equal ecdsa_verify_calls + schnorr_verify_calls. total_records must equal checkecdsa_entries + checkschnorr_entries + count(reject_reason == 8). Do not partition only by sig_version; ECDSA records also use versions 2 and 3.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/checksig-census/instrumentation.diff` around lines 953 - 1000,
Reconcile census classification across the Schnorr and ECDSA instrumentation,
using reject_reason and verification outcome rather than sig_version to identify
record types. Ensure only records corresponding to actual VerifySchnorrSignature
or ECDSA verification calls use outcome 0 or 1, while rejected pre-verification
records use the designated reject classification (including reason 8) so
total_records matches the required accounting.
| ### Run A — authoritative C150 cumulative evidence | ||
|
|
||
| ```bash | ||
| REPO=/home/alpha/exp/bitcoin-rs | ||
| EXP=$REPO/tools/checksig-census | ||
| C150=/home/alpha/bench-g14/corpora/c150 | ||
|
|
||
| mkdir -p "$EXP/out" | ||
|
|
||
| BRS_CENSUS_COUNTERS=$EXP/out/census-0-150k.counters.json \ | ||
| BRS_CENSUS_JOURNAL=$EXP/out/census-0-150k.journal.bin \ | ||
| BRS_CENSUS_LABEL=census-0-150k \ | ||
| BRS_CENSUS_COUNTERS=$EXP/out/c150.counters.json \ | ||
| BRS_CENSUS_CONTEXTS=$EXP/out/c150.contexts.bin \ | ||
| BRS_CENSUS_JOURNAL=$EXP/out/c150.journal.bin \ | ||
| BRS_CENSUS_RECORDS=$EXP/out/c150.records.bin \ | ||
| BRS_CENSUS_LABEL=c150 \ | ||
| taskset -c 0-31 \ | ||
| "$EXP/target/release/examples/mainnet_prefix_replay" \ | ||
| --stop-height 150000 \ | ||
| --rest-url 127.0.0.1:18443 \ | ||
| --blocks-file "$C150/blocks.dat" \ | ||
| --corpus-manifest "$C150/manifest.json" \ | ||
| --assume-valid-height 0 \ | ||
| --data-dir "$EXP/out/census-datadir" \ | ||
| --output "$EXP/out/census-replay.json" | ||
| --data-dir "$EXP/out/c150-datadir" \ | ||
| --output "$EXP/out/c150.replay.json" | ||
|
|
||
| cd "$EXP" | ||
| python3 analyze.py classify-corpus \ | ||
| --counters out/c150.counters.json \ | ||
| --contexts out/c150.contexts.bin \ | ||
| --records out/c150.records.bin \ | ||
| --journal out/c150.journal.bin \ | ||
| --replay out/c150.replay.json \ | ||
| --corpus-manifest "$C150/manifest.json" \ | ||
| --archive "$C150/blocks.dat" \ | ||
| --output out/c150.classification.json \ | ||
| --contract c150 | ||
| ``` | ||
|
|
||
| The replay produces the authoritative `c150.counters.json`, `c150.contexts.bin`, | ||
| `c150.journal.bin`, and `c150.records.bin` artifacts in one process. The strict | ||
| classifier validates each stream's magic and framing, the exact native count | ||
| equations, and every context, record, and journal join. | ||
|
|
||
| Expected: `ffi_verify_entries == 2,868,199`, all verdicts true, pre-taproot | ||
| counters (`op_checksigadd`, `checkschnorr_entries`, `schnorr_verify_calls`) zero. | ||
| counters (`op_checksigadd`, `checkschnorr_entries`, `schnorr_verify_calls`, | ||
| `schnorr_verify_ok`, `schnorr_verify_fail`) zero. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Run A was renamed to c150.* and the rest of the README was not.
This block writes out/c150.counters.json, out/c150.journal.bin, and out/c150.replay.json. Two later sections still name the old files:
- The "Validate census + cross-check" command reads
out/census-0-150k.counters.jsonandout/census-0-150k.journal.bin. - The "Output artifacts" tree lists
census-0-150k.counters.json Run A counters,census-0-150k.journal.bin, andcensus-replay.json, and lists no contexts, records, or classification output for Run A.
Anyone running the README top to bottom hits a missing file. Rename both sections and add c150.contexts.bin, c150.records.bin, and c150.classification.json to the artifact tree.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/checksig-census/README.md` around lines 106 - 149, Update the later
“Validate census + cross-check” section to reference the Run A outputs under the
c150 names, including c150.counters.json and c150.journal.bin. Update the
“Output artifacts” tree to list c150.counters.json, c150.contexts.bin,
c150.records.bin, c150.journal.bin, c150.replay.json, and
c150.classification.json, removing the obsolete census-0-150k and census-replay
names.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 974592e7f2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| --stop-height 150000 \ | ||
| --rest-url 127.0.0.1:18443 \ | ||
| --blocks-file "$C150/blocks.dat" \ | ||
| --corpus-manifest "$C150/manifest.json" \ |
There was a problem hiding this comment.
Following the documented authoritative Run A always stops here with unknown argument: --corpus-manifest. I checked the argument parser and usage string in crates/node/examples/mainnet_prefix_replay.rs:326-365; they support --blocks-file but contain no --corpus-manifest option. Either add that option to the example or remove it from this invocation so the evidence capture can start.
Useful? React with 👍 / 👎.
| --output "$EXP/out/c150.replay.json" | ||
|
|
||
| cd "$EXP" | ||
| python3 analyze.py classify-corpus \ |
There was a problem hiding this comment.
The prescribed post-replay step cannot run: analyze.py classify-corpus --help returns invalid choice, and the repo-wide analyzer parser in tools/checksig-census/analyze.py:1223-1263 registers only validate-capture, validate-census, and verdict. Consequently none of the newly claimed context/record/journal joins can be validated until this command is implemented or the workflow uses an existing command.
Useful? React with 👍 / 👎.
| **Counters JSON**: schema=1, label, 24 named u64 counters, record_count, | ||
| journal_count, context_count. |
There was a problem hiding this comment.
This changes the durable counter contract to 24 counters, while docs/solutions/performance/checksig-census-and-the-script-check-floor.md:11-15 and its INV-15 statement at line 54 still define the result as a 22-counter census. That leaves the documented project learning contradictory; update the overlapping solution and reconcile the affected script-check-floor vocabulary in CONCEPTS.md as required.
AGENTS.md reference: AGENTS.md:L5-L7
Useful? React with 👍 / 👎.
| + const bool schnorr_ok = VerifySchnorrSignature(sig, pubkey, sighash); | ||
| + census_emit_record(tl_census.cur_op, sig_ver_byte, schnorr_ok ? 1 : 0, 64, 32, hashtype, 0, | ||
| + sighash.begin(), sig.data(), pubkey_in.data()); |
There was a problem hiding this comment.
When a capture includes Taproot/Tapscript checks, this emits ordinary outcome 0/1 records containing a 64-byte Schnorr signature and a 32-byte x-only key. bare-secp/src/main.rs:68-90 filters only outcome 2 and ignores sig_version, so it passes these records to the ECDSA-only CPubKey::Verify benchmark; failed Schnorr records can silently appear as expected ECDSA failures and skew timing, while successful ones produce mismatches. Filter the bare input set to ECDSA signature versions or provide a separate Schnorr comparator.
Useful? React with 👍 / 👎.
| BRS_CENSUS_COUNTERS=$EXP/out/c150.counters.json \ | ||
| BRS_CENSUS_CONTEXTS=$EXP/out/c150.contexts.bin \ | ||
| BRS_CENSUS_JOURNAL=$EXP/out/c150.journal.bin \ | ||
| BRS_CENSUS_RECORDS=$EXP/out/c150.records.bin \ |
There was a problem hiding this comment.
Run A now writes c150.counters.json, but the later validate-census command at tools/checksig-census/README.md:296-300 still opens out/census-0-150k.counters.json and the old journal name. In a clean run those files do not exist, so the documented validation sequence fails with a file-not-found error; use the new C150 names consistently in downstream commands and the artifact inventory.
Useful? React with 👍 / 👎.
| let sidecar_path = args | ||
| .context_sidecar | ||
| .clone() | ||
| .unwrap_or_else(|| resolve_default_sidecar(&args.corpus, args.output.as_ref())); |
There was a problem hiding this comment.
The sidecar path is accepted without checking it against the corpus, summary, or native sink paths. If --context-sidecar equals the corpus, File::create at line 114 truncates the input corpus after loading it; if the default derived from an output such as capture.context.jsonl equals that output, the final summary write overwrites the completed sidecar and leaves the reported sidecar hash invalid. Validate that every input and output path is distinct before creating any file.
Useful? React with 👍 / 👎.
| "schnorr_verify_ok", | ||
| "schnorr_verify_fail", |
There was a problem hiding this comment.
After Run C starts emitting these two additional counters, the documented analyze.py verdict step rejects every new bare-secp result. Its COUNTER_NAMES at tools/checksig-census/analyze.py:36-59 still contains 22 names, and lines 995-1003 require the inv_15.counters key set to match that list exactly, so schnorr_verify_ok and schnorr_verify_fail are reported as unexpected extras. Update the analyzer's counter contract before producing 24-key results.
Useful? React with 👍 / 👎.
1 participant
Summary
Verification
Part of #42
Closes #58