From b1178f9eb149a9b9620c347c89e8013967339b72 Mon Sep 17 00:00:00 2001 From: pasta Date: Thu, 29 Oct 2026 02:14:38 +0000 Subject: [PATCH 01/51] fix(llmq): judge every sig-share sender by its own verified shares CSigSharesManager::ProcessPendingSigShares let a peer's sig shares be stored without their own signature check by two routes. 1. Message-id aliasing. Shares were batch-verified with CBLSBatchVerifier. The verifier stores messages in a std::map keyed by message id, and SigShareKey (signHash, quorumMember) does not include the signature or the sender. QSIGSHARE is accepted from any peer, so two peers can push shares with the same key in one batch; the second PushMessage is then a silent no-op and that peer's messagesBySource entry points at the first peer's message: - forged share first: the batch fails, the per-source fallback re-checks the forged share for both peers, and the honest sender lands in badSources and is banned; - honest share first: the batch passes on the honest signature, the forged share is never checked, and ProcessPendingSigShares can store it. Key batch entries by (NodeId, SigShareKey) so every sender is judged by its own signature. pendingIncomingSigShares already deduplicates by SigShareKey per node, so for a single sender, and for distinct keys across senders, the new keys map one-to-one onto the old ones and the batch is unchanged; the only difference is that a key pushed by several peers now produces one entry per peer instead of an aliased one. The verifier itself is unchanged and its other users (ISLOCK, recovered sigs) keep their ids, which already cover the signature. 2. Malformed-share break. A share whose lazy signature fails to deserialize called BanNode() and stopped pushing that peer's shares, but did not add the peer to badSources. The post-verification loop therefore still passed all of that peer's shares, the malformed one and every never-pushed one after it, to ProcessPendingSigShares, which stored them and filled their member slots so the honest shares were later dropped as already known. Record the peer in badSources instead, as develop does; the post-verification loop then bans it and skips its shares. Test: sig_share_batch_verifier_judges_each_sender_by_its_own_share pushes the same key from two peers in both orders, plus a same-share relay by two honest peers, through PushSigShareToBatchVerifier and checks badSources. With the pre-fix keying (SigShareKey only) the honest-first row yields no bad sources and the attacker-first row yields both peers. Route 2 is not unit-tested: it lives inside ProcessPendingSigShares, which needs a full CSigSharesManager with a quorum and peer manager; it was verified by inspection against the develop fix. Co-Authored-By: Claude Opus 5.5 --- src/llmq/signing_shares.cpp | 12 +++++++--- src/llmq/signing_shares.h | 9 ++++++++ src/test/llmq_utils_tests.cpp | 43 +++++++++++++++++++++++++++++++++++ 3 files changed, 61 insertions(+), 3 deletions(-) diff --git a/src/llmq/signing_shares.cpp b/src/llmq/signing_shares.cpp index 6537f9975d36..f6038e9148e8 100644 --- a/src/llmq/signing_shares.cpp +++ b/src/llmq/signing_shares.cpp @@ -71,6 +71,12 @@ void CSigShare::UpdateKey() key.second = quorumMember; } +void PushSigShareToBatchVerifier(SigShareBatchVerifier& batchVerifier, NodeId from, const CSigShare& sigShare, + const CBLSSignature& sig, const CBLSPublicKey& pubKeyShare) +{ + batchVerifier.PushMessage(from, {from, sigShare.GetKey()}, sigShare.GetSignHash(), sig, pubKeyShare); +} + std::string CSigSesAnn::ToString() const { return strprintf("sessionId=%d, llmqType=%d, quorumHash=%s, id=%s, msgHash=%s", @@ -809,7 +815,7 @@ bool CSigSharesManager::ProcessPendingSigShares() // It's ok to perform insecure batched verification here as we verify against the quorum public key shares, // which are not craftable by individual entities, making the rogue public key attack impossible - CBLSBatchVerifier batchVerifier(false, true); + SigShareBatchVerifier batchVerifier(false, true); cxxtimer::Timer prepareTimer(true); size_t verifyCount = 0; @@ -825,7 +831,7 @@ bool CSigSharesManager::ProcessPendingSigShares() // we didn't check this earlier because we use a lazy BLS signature and tried to avoid doing the expensive // deserialization in the message thread if (!sig.IsValid()) { - BanNode(nodeId); + batchVerifier.badSources.emplace(nodeId); // don't process any additional shares from this node break; } @@ -840,7 +846,7 @@ bool CSigSharesManager::ProcessPendingSigShares() assert(false); } - batchVerifier.PushMessage(nodeId, sigShare.GetKey(), sigShare.GetSignHash(), sig, pubKeyShare); + PushSigShareToBatchVerifier(batchVerifier, nodeId, sigShare, sig, pubKeyShare); verifyCount++; } } diff --git a/src/llmq/signing_shares.h b/src/llmq/signing_shares.h index 873f93384b42..b693100c4744 100644 --- a/src/llmq/signing_shares.h +++ b/src/llmq/signing_shares.h @@ -6,6 +6,7 @@ #define BITCOIN_LLMQ_SIGNING_SHARES_H #include +#include #include #include #include @@ -91,6 +92,14 @@ class CSigShare : virtual public CSigBase } }; +// Batch entries are keyed by sender as well as SigShareKey. QSIGSHARE is accepted from any peer, so two peers can +// push the same key; with a sender-less key the second push would alias the first entry, letting one peer's share be +// judged in place of the other's (banning an honest sender, or never checking a forged share). +using SigShareBatchVerifier = CBLSBatchVerifier>; + +void PushSigShareToBatchVerifier(SigShareBatchVerifier& batchVerifier, NodeId from, const CSigShare& sigShare, + const CBLSSignature& sig, const CBLSPublicKey& pubKeyShare); + // Nodes will first announce a signing session with a sessionId to be used in all future P2P messages related to that // session. We locally keep track of the mapping for each node. We also assign new sessionIds for outgoing sessions // and send QSIGSESANN messages appropriately. All values except the max value for uint32_t are valid as sessionId diff --git a/src/test/llmq_utils_tests.cpp b/src/test/llmq_utils_tests.cpp index a3f80e5aa1f9..1eb066fc5bd1 100644 --- a/src/test/llmq_utils_tests.cpp +++ b/src/test/llmq_utils_tests.cpp @@ -5,6 +5,7 @@ #include #include +#include #include #include #include @@ -18,6 +19,7 @@ #include #include +#include using namespace llmq; using namespace llmq::testutils; @@ -173,6 +175,47 @@ BOOST_AUTO_TEST_CASE(pending_sig_shares_session_removal_updates_count) BOOST_CHECK_EQUAL(node_state.pendingIncomingSigShares.Size(), 1U); } +BOOST_AUTO_TEST_CASE(sig_share_batch_verifier_judges_each_sender_by_its_own_share) +{ + const bool legacy_scheme{bls::bls_legacy_scheme.load()}; + const CSigShare sig_share{MakeSigShare(1)}; + + CBLSSecretKey sk_share; + sk_share.MakeNewKey(); + const CBLSPublicKey pk_share{sk_share.GetPublicKey()}; + const CBLSSignature valid_sig{sk_share.Sign(sig_share.GetSignHash(), legacy_scheme)}; + + CBLSSecretKey sk_forger; + sk_forger.MakeNewKey(); + const CBLSSignature forged_sig{sk_forger.Sign(sig_share.GetSignHash(), legacy_scheme)}; + + constexpr NodeId honest{1}; + constexpr NodeId attacker{2}; + constexpr NodeId other_honest{3}; + + struct Push { + NodeId from; + CBLSSignature sig; + }; + const std::vector, std::set>> cases{ + // Honest share first: the forged copy of the same key must still be checked, not shadowed. + {{{honest, valid_sig}, {attacker, forged_sig}}, {attacker}}, + // Forged share first: the honest sender must be judged by its own share, not the attacker's. + {{{attacker, forged_sig}, {honest, valid_sig}}, {attacker}}, + // The same valid share relayed by two peers is not misbehavior. + {{{honest, valid_sig}, {other_honest, valid_sig}}, {}}, + }; + + for (const auto& [pushes, expected_bad_sources] : cases) { + SigShareBatchVerifier verifier(/*_secureVerification=*/false, /*_perMessageFallback=*/true); + for (const auto& push : pushes) { + PushSigShareToBatchVerifier(verifier, push.from, sig_share, push.sig, pk_share); + } + verifier.Verify(); + BOOST_CHECK(verifier.badSources == expected_bad_sources); + } +} + BOOST_AUTO_TEST_CASE(batched_sig_shares_rejects_oversized_inner_vector) { CDataStream stream{SER_NETWORK, PROTOCOL_VERSION}; From 07494dd628a996debc772f11d96cbff68d56928f Mon Sep 17 00:00:00 2001 From: pasta Date: Wed, 28 Oct 2026 22:47:22 +0000 Subject: [PATCH 02/51] fix(consensus): reject negative transaction nVersion after DIP3 A tx version word in 0x8000-0xFFFF is read into int16 nVersion as negative and sign-extends on re-serialization to type 0xFFFF, so a node fed the miner's original wire bytes accepts a block that every relayed/compact/IBD/reindex copy rejects -> permanent consensus split. Reject negative nVersion in ContextualCheckTransaction once DIP3 is active (consensus-enforced for mempool and block connection), and route version/type word packing through a single PackVersionAndType() helper. Valid-tx txids are unchanged (no fork); no activation height needed. Backport of knst fix-signed-nversion. Co-Authored-By: Claude Opus 4.8 --- src/primitives/transaction.h | 11 +++++++++-- src/script/interpreter.cpp | 2 +- src/test/sighash_tests.cpp | 2 +- src/test/transaction_tests.cpp | 20 +++++++++++++++++++ src/test/txvalidation_tests.cpp | 34 +++++++++++++++++++++++++++++++++ src/validation.cpp | 5 +++++ 6 files changed, 70 insertions(+), 4 deletions(-) diff --git a/src/primitives/transaction.h b/src/primitives/transaction.h index b45e81fa6e88..206d8562656c 100644 --- a/src/primitives/transaction.h +++ b/src/primitives/transaction.h @@ -205,6 +205,13 @@ class CTxOut struct CMutableTransaction; +/** nVersion and nType share one 32-bit wire field. nVersion fills the low half as raw bits: + * sign-extending a negative nVersion would overwrite nType. */ +constexpr int32_t PackVersionAndType(int16_t nVersion, uint16_t nType) +{ + return static_cast(static_cast(nVersion) | (static_cast(nType) << 16)); +} + /** The basic transaction that is broadcasted on the network and contained in * blocks. A transaction can contain multiple inputs and outputs. */ @@ -241,7 +248,7 @@ class CTransaction template inline void Serialize(Stream& s) const { - int32_t n32bitVersion = this->nVersion | (this->nType << 16); + int32_t n32bitVersion = PackVersionAndType(this->nVersion, this->nType); s << n32bitVersion; s << vin; s << vout; @@ -320,7 +327,7 @@ struct CMutableTransaction SERIALIZE_METHODS(CMutableTransaction, obj) { int32_t n32bitVersion; - SER_WRITE(obj, n32bitVersion = obj.nVersion | (obj.nType << 16)); + SER_WRITE(obj, n32bitVersion = PackVersionAndType(obj.nVersion, obj.nType)); READWRITE(n32bitVersion); SER_READ(obj, obj.nVersion = (int16_t) (n32bitVersion & 0xffff)); SER_READ(obj, obj.nType = (uint16_t) ((n32bitVersion >> 16) & 0xffff)); diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp index 4dc0b7e63ec8..1dac934c232f 100644 --- a/src/script/interpreter.cpp +++ b/src/script/interpreter.cpp @@ -1420,7 +1420,7 @@ class CTransactionSignatureSerializer template void Serialize(S &s) const { // Serialize nVersion - int32_t n32bitVersion = txTo.nVersion | (txTo.nType << 16); + int32_t n32bitVersion = PackVersionAndType(txTo.nVersion, txTo.nType); ::Serialize(s, n32bitVersion); // Serialize vin unsigned int nInputs = fAnyoneCanPay ? 1 : txTo.vin.size(); diff --git a/src/test/sighash_tests.cpp b/src/test/sighash_tests.cpp index f1d8e6d0ad90..62625c9fc121 100644 --- a/src/test/sighash_tests.cpp +++ b/src/test/sighash_tests.cpp @@ -89,7 +89,7 @@ void static RandomScript(CScript &script) { void static RandomTransaction(CMutableTransaction &tx, bool fSingle) { - tx.nVersion = int(InsecureRandRange(2)) + 1; + tx.nVersion = static_cast(InsecureRandBits(16)); tx.vin.clear(); tx.vout.clear(); tx.nLockTime = (InsecureRandBool()) ? InsecureRand32() : 0; diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 6e117ad5117b..fc8601b15220 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -329,6 +329,26 @@ BOOST_AUTO_TEST_CASE(basic_transaction_tests) BOOST_CHECK_MESSAGE(!CheckTransaction(CTransaction(tx), state) || !state.IsValid(), "Transaction with duplicate txins should be invalid."); } +BOOST_AUTO_TEST_CASE(tx_version_type_roundtrip) +{ + // nVersion and nType share one 32-bit wire field; a negative nVersion must not sign-extend over nType + for (const auto& [version, type] : std::vector>{{-1, TRANSACTION_NORMAL}, {-1, 0xffff}}) { + CMutableTransaction tx; + tx.nVersion = version; + tx.nType = type; + CDataStream mutable_stream(SER_NETWORK, PROTOCOL_VERSION); + mutable_stream << tx; + CDataStream stream(SER_NETWORK, PROTOCOL_VERSION); + stream << CTransaction(tx); + BOOST_CHECK_EQUAL(HexStr(stream), HexStr(mutable_stream)); + + CMutableTransaction tx_read; + stream >> tx_read; + BOOST_CHECK_EQUAL(tx_read.nVersion, version); + BOOST_CHECK_EQUAL(tx_read.nType, type); + } +} + BOOST_AUTO_TEST_CASE(test_Get) { FillableSigningProvider keystore; diff --git a/src/test/txvalidation_tests.cpp b/src/test/txvalidation_tests.cpp index 36042431f85c..5941e2a75619 100644 --- a/src/test/txvalidation_tests.cpp +++ b/src/test/txvalidation_tests.cpp @@ -8,6 +8,8 @@ #include #include #include