Repository navigation
Conversation
OP_CHECKDATASIGPQC and OP_CHECKDATASIGADDPQC verified their signature directly, so no check could reuse an earlier result: mempool acceptance verified a valid data signature in both its policy and its consensus pass, and block connection could not reuse either result. - Data signatures verify through a new BaseSignatureChecker method. The default verifies directly, as before. - CachingTransactionSignatureChecker looks the check up in, and stores a verified data signature in, a separate 'D' domain of the signature cache, so a data signature never matches a transaction-signature entry for the same bytes. - As for transaction signatures, a failed verification is never cached. - The interpreter's size, hash type and validation weight checks run before the checker is called and are unchanged. Failed verifications stay uncached on purpose. A failing signature can be changed without changing the cost of verifying it, so a cache of failures keyed on the signature would not reduce repeated verification work. A failed PQC verification costs about 0.45 us per witness byte, close to the about 0.34 us per byte of a failed Schnorr check in tapscript. Script validity is unchanged. Refs #259 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
@codex review |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
Closing in favour of #265, which implements the same fix for #259. Both PRs add a We're consolidating to one PR per v1.0.1 release-gate finding (see #174):
#265 was opened first and Codex approved it. An independent adversarial review found no consensus, cache-soundness, checker-forwarding or concurrency problems. #265 also adds a release-notes fragment that records the remaining denial-of-service limitation. If #267 has anything #265 lacks, please comment on #265 and we'll fold it in. |
Summary
OP_CHECKDATASIGPQCandOP_CHECKDATASIGADDPQCverified their signature directly, so no check could reuse an earlier result. Mempool acceptance verified a valid data signature in both its policy and its consensus pass, and block connection could not reuse either result. This routes data signatures through the signature checker so that verified ones are cached like transaction signatures.What changed:
src/script/interpreter.{h,cpp}: data signatures verify through a new virtualBaseSignatureChecker::VerifyDataSigPQC(sig, pubkey, hash). The default verifies directly, as before, so every other checker behaves as it did.DeferringSignatureCheckerforwards it.EvalCheckDataSigPQCtakes the checker and calls it.src/script/sigcache.{h,cpp}:CachingTransactionSignatureCheckerlooks the check up in, and stores a verified data signature in, a separate'D'domain of the signature cache (ComputeEntryDataSigPQC, its own salted hasher). A data signature never matches a transaction-signature ('P') entry for the same bytes.Why failed verifications stay uncached: a failing signature can be changed without changing the cost of verifying it, so a cache of failures keyed on the signature would not reduce repeated verification work. A failed PQC verification costs about 0.45 µs per witness byte, close to the about 0.34 µs per byte of a failed Schnorr check in tapscript. Reducing the cost of repeated failed verifications is a separate, local-policy question (for example, per-peer accounting of failed verification time in mempool acceptance).
Script validity is unchanged.
Refs #259
Testing
Built with
cmake -B build -G Ninja -DCMAKE_BUILD_TYPE=Release -DBUILD_GUI=OFF -DBUILD_BENCH=OFF -DBUILD_FUZZ_BINARY=OFF(targetstest_qbit qbitd qbit-cli). Results at this head,8e5f5b809d:Changed and new unit tests:
pqc_sigcache_tests/data_signatures_use_the_cachereplacesdata_signatures_bypass_the_cache, which asserted the old direct verification. It checks that a verified data signature is stored in its own domain and emits no transaction-signature cache events; that a failed one is verified on every check and never stored; that a transaction-signature entry for the same bytes does not satisfy it; that a consuming checker (as in block connection) uses the stored entry; and that a non-storing checker adds nothing. Results, script errors and the remaining validation weight match the uncached checker.txvalidationcache_tests/pqc_data_signature_reuse: mempool acceptance verifies a validOP_CHECKDATASIGPQCsignature once (the consensus pass reuses the policy pass). A failed data signature is verified on every submission and the transaction is rejected each time.Unit tests:
build/bin/test_qbit --run_test='pqc_tests:pqc_sigcache_tests:txvalidationcache_tests:script_tests:net_tests:net_peer_connection_tests:net_peer_eviction_tests:netbase_tests:denialofservice_tests:peerman_tests:headers_sync_chainwork_tests:blockencodings_tests': 134 cases, 597,759 assertions, no errors.Functional tests:
build/test/functional/test_runner.py --jobs=4: 20 of 20 runs passed.feature_p2mr(data-signature spends)feature_auxpow,feature_auxpow_commitment_activation,feature_minchainwork,feature_minchainwork_requested_blockp2p_block_sync(v1, v2),p2p_compactblocks,p2p_compactblocks_blocksonly,p2p_compactblocks_hb(v1, v2),p2p_fingerprint,p2p_getdatap2p_headers_sync_with_minchainwork,p2p_ibd_stalling(v1, v2),p2p_initial_headers_sync,p2p_invalid_messages,p2p_sendheaders,p2p_unrequested_blocksNot run: the fuzz and bench binaries were not built.
src/test/fuzz/script_sigcache.cppandsrc/bench/checkinputs_p2mr_pqc.cppuse interfaces this PR does not change.Target Branch
mainor a maintainer-requested release branch such as0.1.x.This targets
1.x.xfor v1.0.1 (#181 prepares that release), and then 1.1.0-rc2.Risk / Review Notes
Notes:
VerifyDataSigPQCon every non-caching checker is the original directpubkey.Verify(hash, sig).SHA256(nonce || 'D' || 31 zero bytes || hash || pubkey || sig). The interpreter enforces the 32-byte public key and the fixed signature size before every call, so the concatenation is unambiguous.CPQCPubKey::Verifyreads only the key, the hash and the signature, and no script flag reaches it, so a cached result cannot differ from a fresh verification.storeis set, and consume (erase) on a hit when not storing.Docs / Process Impact
Choose exactly one:
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.