fix: harden BTQ audit issues across Dilithium, P2MR, and constants#57
Open
Oskii wants to merge 10 commits into
Open
fix: harden BTQ audit issues across Dilithium, P2MR, and constants#57Oskii wants to merge 10 commits into
Oskii wants to merge 10 commits into
Conversation
Oskii
changed the title
| static bool EvalChecksigDilithium(const valtype& sig, const valtype& pubkey, CScript::const_iterator pbegincodehash, CScript::const_iterator pend, ScriptExecutionData& execdata, unsigned int flags, const BaseSignatureChecker& checker, SigVersion sigversion, ScriptError* serror, bool& success) | ||
| { | ||
| if (!sig.empty() && sig.size() != BTQ_DILITHIUM_SIGNATURE_SIZE) { | ||
| if (!sig.empty() && sig.size() != BTQ_DILITHIUM_SIGNATURE_SIZE + 1) { |
Collaborator
There was a problem hiding this comment.
Just flagging that this is a consensus change therefore a hardfork required
|
|
||
| #include <consensus/amount.h> | ||
| #include <consensus/consensus.h> | ||
| #include <consensus/consensus.h> |
Collaborator
There was a problem hiding this comment.
Duplicate include, we can remove this
| m_map_signing_providers[index] = *out_keys; | ||
| } | ||
|
|
||
| // Always add Dilithium pubkeys for fee estimation (even if include_private=false) |
Collaborator
There was a problem hiding this comment.
Now the fee estimator can't recognize the Dilithium inputs. It either fails to identify them or treats them as something smaller, so it under-estimates the signature weight (assuming a ~72-byte sig instead of ~2,421). The transaction's estimated size is too small, so the fee is set too low.
Collaborator
There was a problem hiding this comment.
Suggested fix
Decouple pubkey exposure (needed for estimation, not secret) from private-key decryption (needs unlock). For example, populate dilithium_pubkeys from a non-secret source regardless of lock state
Harden the wallet_dilithium_send regression so it proves the property issue #41 is about: sendtoaddress/sendmany spending a Dilithium UTXO must return a valid, broadcastable transaction, not merely relay one. - Force coin selection onto the Dilithium UTXO and assert it is the input actually spent (exercises the OP_CHECKSIGDILITHIUM signing path). - Mine the broadcast tx and assert it confirms, proving the Dilithium signature verifies under consensus rather than only being accepted to the mempool. - Cover sendmany spending a Dilithium UTXO in addition to sendtoaddress. Co-authored-by: Cursor <cursoragent@cursor.com>
Collaborator
Author
Stage 0 Mainnet Audit TrancheCommit: This tranche starts the staged component-by-component mainnet audit program. It does not claim mainnet readiness. It closes audit-harness gaps and several concrete defects that would have made later evidence unreliable, then records the remaining component matrix in What changed
New or restored test coverage
Validation run after rebaseBuilds:
Focused unit tranche:
Functional tranche:
Audit/lint/registration:
Remaining no-go itemsThe new matrix intentionally keeps the branch no-go for mainnet until later stages close the remaining evidence gaps, especially:
Note: |
Collaborator
Author
Stage 1 component audit tranchePushed commit What this tranche closes
Validation runPassed:
Validation boundary still open:
|
BarneyChambers
added a commit
that referenced
this pull request
Jun 9, 2026
… interpreter PR #57 changed the GetTxSigOpCost P2WPKH/P2SH-P2WPKH assertions to expect SCRIPT_ERR_WITNESS_PUBKEYTYPE, matching its own (now superseded) witness program handling. The merge adopted master/#59's VerifyWitnessProgram, which gates the witness-pubkeytype check behind SCRIPT_VERIFY_WITNESS_PUBKEYTYPE (not in the test's flags), so a 33-byte non-compressed witness pubkey now fails the OP_EQUALVERIFY (HASH160 mismatch) instead. Restore the upstream/ master expectation of SCRIPT_ERR_EQUALVERIFY so the test matches the interpreter we kept. Fixes a sigopcount_tests abort + arg-pollution cascade.
Open
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Audit Basis
This PR is the remediation branch for a deep audit pass against the local audit materials:
AUDIT_REPORT_2026-05-19.mdAudit Scope of Work BTQ.pdfThe audit was treated as a test-driven hardening exercise. The work here does not rely on visual inspection alone: every confirmed issue was either covered by an existing failing/insufficient test that was corrected, a new regression test, a new lint guard, or a focused functional test run that exercises the affected behavior end to end.
This branch also preserves the earlier P2MR metadata idempotency fix already present on
fix/p2mr-metadata-idempotent, then extends the branch with the broader audit remediation commit.Summary
Fixes #58 by accepting signer-produced Dilithium script signatures that carry the required trailing sighash byte (
raw Dilithium signature || sighash type) beforeCheckDilithiumSignature()strips that byte for raw Dilithium verification. The regression coverage now proves that raw 2,420-byte stack elements are rejected, 2,421-byte stack elements are accepted, and the wallet signing path emits verifier-accepted 2,421-byte P2PK and P2PKH Dilithium satisfactions.This PR hardens BTQ consensus/test constants, Dilithium wallet handling, P2MR signing behavior, raw transaction RPC behavior, standardness limits, and wallet fee-bump sizing. It also expands the test surface so the BTQ-specific values and post-quantum wallet paths are covered by automated checks instead of relying on inherited Bitcoin defaults.
The broad theme is removing hidden Bitcoin assumptions from BTQ-specific paths and making Dilithium/P2MR behavior explicit where generic ECDSA/Schnorr code paths were previously too narrow.
Consensus, Policy, and Test Constant Alignment
The audit found that several Python functional-test framework values and C++ expectations still reflected Bitcoin defaults or partially migrated BTQ values. This is risky because functional tests can pass for the wrong protocol when the harness is not using the same constants as consensus/policy code.
Changes include:
8,000,000weight units.16.70,000,000bytes.15 * 60seconds.test/lint/lint-btq-consensus-constants.pyso C++ and Python BTQ constants cannot silently drift apart again.This affects both the implementation-adjacent tests and the reusable functional test framework files under
test/functional/test_framework/.Standardness and Script Limit Hardening
The audit identified places where standardness limits were not consistently derived from the BTQ script element limit. This can create contradictory behavior between consensus-like script handling, standardness policy, and tests.
Changes include:
MAX_SCRIPT_ELEMENT_SIZEinstead of duplicating stale constants.transaction_testscoverage for standardness limits across legacy, P2WSH, taproot, and P2MR-style witness programs.Dilithium Signing and Script Verification
The audit found several Dilithium-specific paths that were not as explicit as the ECDSA/Schnorr paths they extend. The fixes make Dilithium behavior visible to the signing, deferring, and extraction layers.
Changes include:
DeferringSignatureChecker.SignatureExtractorCheckerso callers that estimate signature cost can observe them.BTQ_DILITHIUM_SIGNATURE_SIZE + 1OP_CHECKSIGDILITHIUM signatures at the interpreter size gate, matching the existing downstream sighash-byte stripping logic.ProduceSignature()-generated P2PK/P2PKH Dilithium script satisfactions.Dilithium Wallet Encryption and Locked Wallet Behavior
The audit identified wallet encryption and unlock behavior as a high-risk area because Dilithium private-key handling is separate from the historical Bitcoin key flow. A wallet can appear to support a key type while failing to retrieve, decrypt, migrate, back up, or sign with it correctly under encrypted/locked states.
Changes include:
signmessagewithdilithiumcan access private Dilithium material.Dilithium Descriptor, Import, Backup, and Migration Paths
The audit found that supporting Dilithium addresses is not enough by itself; persistence and migration paths also need explicit handling or keys can be omitted from backup/import flows.
Changes include:
DILITHIUM_LEGACYentropy from private descriptor material plus descriptor id/index/type, avoiding weak or repeated entropy sources.WITNESS_V2_P2MRis rejected as unrecognized by the legacy import switch instead of being accidentally misclassified.P2MR Signing and RPC Completion Semantics
The audit found that P2MR signing behavior could report completion too optimistically in some fallback cases. That is dangerous because callers may treat
complete=trueas spendable even if the assembled witness does not actually satisfy the script.Changes include:
WITNESS_V2_P2MRwitness stacks inProduceSignature()instead of clearing them through a generic path.signp2mrtransaction, verify assembled fallback witnesses before marking a transaction complete.OP_DROP OP_TRUEsingle-leaf P2MR script cannot be satisfied by an empty stack and must returncomplete=false.VerifyScript.Raw Transaction and Output-Type RPC Behavior
The audit found RPC surfaces where P2MR/Dilithium script types were either too implicitly handled or omitted from generic output-type enumeration.
Changes include:
decodescriptsoWITNESS_V2_P2MRis explicitly treated as non-wrappable and does not receive inappropriate P2SH/segwit wrapper suggestions.rpc_rawtransaction.pyregression coverage for P2MRdecodescriptno-wrapper behavior.GetAllOutputTypes()explicit and complete for the BTQ/Dilithium output types.Fee Bumping and Signature Weight Estimation
The audit found that wallet fee bumping needed better accounting for non-ECDSA signature families. Underestimating signature weight is a wallet correctness and UX risk because fee calculations can be wrong for larger post-quantum signatures.
Changes include:
SignatureWeightCheckerto record ECDSA, Schnorr, and Dilithium signatures.feebumper_testscoverage for BTQ weight expectations and Schnorr/Dilithium weight recording.P2P and Functional Test Framework Updates
The functional test harness was updated where protocol-level values changed or where the previous assumptions made tests validate the wrong thing.
Changes include:
70 MBmax protocol message size.sendmsgtopeertests realistic when oversized payload hex cannot fit within the RPC body limit.Test Strategy
The validation strategy was intentionally layered:
Test Evidence
Style/build/lint:
git diff --cached --checkpython3 test/lint/lint-btq-consensus-constants.pymake -C src -j2 test/test_btq btqd btq-cliFocused C++ tests:
src/test/test_btq --run_test=p2mr_tests/*src/test/test_btq --run_test=feebumper_tests/*src/test/test_btq --run_test=rpc_tests/rpc_all_output_types_includes_btq_typessrc/test/test_btq --run_test=scriptpubkeyman_tests/*src/test/test_btq --run_test=transaction_tests/test_IsStandardsrc/test/test_btq --run_test=transaction_tests/test_IsWitnessStandard_stack_item_size_limitssrc/test/test_btq --run_test=dilithium_basic_tests/*src/test/test_btq --run_test=dilithium_wallet_tests/*src/test/test_btq --run_test=pow_tests/*src/test/test_btq --run_test=chainparams_genesis_tests/*src/test/test_btq --run_test=sigopcount_tests/*Functional tests:
python3 test/functional/test_runner.py --portseed=31112 -t /tmp/btq_func_block5 feature_block.py -j 1python3 test/functional/test_runner.py --portseed=31113 -t /tmp/btq_func_p2mrrpc feature_p2mr_rpc.py -j 1python3 test/functional/test_runner.py --portseed=31115 -t /tmp/btq_func_rawtx2 rpc_rawtransaction.py -j 1python3 test/functional/test_runner.py --portseed=31120 -t /tmp/btq_func_segwit5 feature_segwit.py -j 1python3 test/functional/test_runner.py --portseed=31122 -t /tmp/btq_func_rpcnet2 rpc_net.py -j 1python3 test/functional/test_runner.py --portseed=31123 -t /tmp/btq_func_p2pmsg p2p_invalid_messages.py -j 1python3 test/functional/test_runner.py --portseed=31124 -t /tmp/btq_func_wallet wallet_dilithium_send.py -j 1python3 test/functional/test_runner.py --portseed=31129 -t /tmp/btq_func_dilsigops feature_dilithium_sigops.py -j 1python3 test/functional/btq_chain_identity.py --cachedir=/home/o/btq-core/test/cache --configfile=/home/o/btq-core/test/config.ini --portseed=31126 --tmpdir=/tmp/btq_chain_identity_directpython3 test/functional/btq_regtest_mining.py --cachedir=/home/o/btq-core/test/cache --configfile=/home/o/btq-core/test/config.ini --portseed=31128 --tmpdir=/tmp/btq_regtest_mining_direct_2Residual Closure Update
The previous validation boundaries have now been closed in commit
d6ddba6.What changed in this closure pass:
wallet_tests/*unit sweep no longer tripstime-too-new.TestChain100Setupnow seeds mock time from the BTQ regtest genesis time and advances by the configured BTQ target spacing instead of using stale Bitcoin-era one-second mock-time increments.GetBlockSubsidy()instead of hard-coded50 * COINvalues.feature_segwit.pyimport setup now imports each concrete witness script form explicitly, including P2SH-P2WSH redeem/witness scripts.IsMinecache entries after successful script/key/pubkey/scriptPubKey imports. This fixes the concrete P2SH-P2WSH case whereimportmulticould cacheISMINE_NObefore adding the material that made the output spendable.feature_segwit.pylistunspent matrix outputs were scaled from0.1 BTQto0.01 BTQso the large synthetic output matrix fits BTQ's5 BTQregtest subsidy while preserving the same recognition/spendability assertions.btq_chain_identity.pyandbtq_regtest_mining.pyare now first-classtest_runner.pyentries, and the runner accepts thebtq_prefix.Additional residual-closure validation:
make -C src -j2 test/test_btq btqd btq-clisrc/test/test_btq --run_test=wallet_tests/*src/test/test_btq --run_test=ismine_tests/ismine_standardsrc/test/test_btq --run_test=feebumper_tests/*python3 test/lint/lint-btq-consensus-constants.pypython3 test/functional/test_runner.py --portseed=31216 -t /tmp/btq_func_segwit_legacy_amountfix "feature_segwit.py --legacy-wallet" -j 1python3 test/functional/test_runner.py --portseed=31223 -t /tmp/btq_func_segwit_desc_seq "feature_segwit.py --descriptors" -j 1This runner invocation completed both
feature_segwit.py --descriptorsandfeature_segwit.py --descriptors --v2transport.python3 test/functional/test_runner.py --portseed=31222 -t /tmp/btq_func_segwit_desc_v2_seq2 "feature_segwit.py --descriptors --v2transport" -j 1python3 test/functional/test_runner.py --portseed=31220 -t /tmp/btq_runner_btq_residual_final_seq btq_chain_identity.py btq_regtest_mining.py -j 1git diff --checkEnvironment note: BDB was built locally through
dependsand the working tree was configured with BDB and SQLite enabled for the legacy-wallet validation. The generated depends/build artifacts are ignored and are not part of this PR.Issue #58 targeted validation:
make -C src -j2 test/test_btqsrc/test/test_btq --run_test=dilithium_basic_tests/dilithium_script_signature_with_sighash_bytesrc/test/test_btq --run_test=dilithium_basic_tests/*src/test/test_btq --run_test=dilithium_address_script_tests/*src/test/test_btq --run_test=dilithium_network_policy_tests/*src/test/test_btq --run_test=dilithium_descriptor_tests/*src/test/test_btq --run_test=dilithium_key_tests/*src/test/test_btq --run_test=script_standard_tests/script_standard_Solver_successsrc/test/test_btq --run_test=script_standard_tests/script_standard_Solver_failuresrc/test/test_btq --run_test=script_standard_tests/script_standard_ExtractDestinationsrc/test/test_btq --run_test=script_standard_tests/script_standard_GetScriptFor_*git diff --checkBoundary observed during issue #58 validation:
src/test/test_btq --run_test=script_standard_tests/*was not counted as passing evidence because unrelated upstream taproot address-vector cases still compare BTQqbtc...encoded destinations against Bitcoinbc1...expected vectors. The solver and script-construction subsets relevant to this issue passed.Review Notes
Suggested review order:
The riskiest areas are wallet private-key handling, P2MR completion semantics, and protocol constant changes. Those areas have the densest new regression coverage in this PR.