llmq: cover QBSIGSHARES aggregate sig-share bound with a testable decoder

The self-review of the batched sig-share intake bounds flagged one gap: the
running-total abort in the QBSIGSHARES handler — where many individually-valid
batches together exceed MAX_MSGS_TOTAL_BATCHED_SIGS — had no direct unit
coverage. The check lived inline in NetSigning::ProcessMessage, so the only way
to reach it was to drive a full P2P message through the handler.

Extract the QBSIGSHARES decode loop into a free function,
llmq::UnserializeBatchedSigShares(CDataStream&), that production calls inside
the existing try/catch. Wire format, peer banning, and logging semantics are
unchanged — the callsite still logs, bans, and rethrows on any
ios_base::failure. The extracted decoder is the exact code that runs in
production, so the new tests exercise the real path rather than a mirror.

Add three targeted cases in llmq_utils_tests:
- oversized outer batch count (built from empty batches so only the outer-count
  guard, not an end-of-stream artifact, can reject it),
- oversized running aggregate across batches each within the per-batch cap,
- an aggregate exactly at the cap, which must decode intact.

Both count guards were mutation-verified: neutralizing either guard makes its
corresponding test fail and no other.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: thepastaclaw

src/llmq/net_signing.cpp

@@ -23,6 +23,26 @@
 #include <unordered_map>
 
 namespace llmq {
+std::vector<CBatchedSigShares> UnserializeBatchedSigShares(CDataStream& vRecv)
+{
+    std::vector<CBatchedSigShares> msgs;
+    const uint64_t msgs_size{ReadCompactSize(vRecv, /*range_check=*/false)};
+    if (msgs_size > MAX_MSGS_TOTAL_BATCHED_SIGS) {
+        throw std::ios_base::failure("QBSIGSHARES batch count too large");
+    }
+    msgs.reserve(msgs_size);
+    size_t total_sigs_count{0};
+    while (msgs.size() < msgs_size) {
+        msgs.emplace_back();
+        vRecv >> msgs.back();
+        total_sigs_count += msgs.back().sigShares.size();
+        if (total_sigs_count > MAX_MSGS_TOTAL_BATCHED_SIGS) {
+            throw std::ios_base::failure("QBSIGSHARES sig share count too large");
+        }
+    }
+    return msgs;
+}
+
 void NetSigning::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDataStream& vRecv)
 {
     if (msg_type == NetMsgType::QSIGREC) {
@@ -99,27 +119,9 @@ void NetSigning::ProcessMessage(CNode& pfrom, const std::string& msg_type, CData
             return;
         }
     } else if (msg_type == NetMsgType::QBSIGSHARES) {
-        // The inner sigShares vector is bounded by CBatchedSigShares's SERIALIZE_METHODS
-        // via LIMITED_VECTOR, but many individually-valid batches could still exceed the
-        // total sig-share cap, so bound the outer count and check the running total as we
-        // decode so we stop reading before an attacker forces us through the full cross
-        // product of the per-vector limits.
         std::vector<CBatchedSigShares> msgs;
         try {
-            const uint64_t msgs_size{ReadCompactSize(vRecv, /*range_check=*/false)};
-            if (msgs_size > MAX_MSGS_TOTAL_BATCHED_SIGS) {
-                throw std::ios_base::failure("QBSIGSHARES batch count too large");
-            }
-            msgs.reserve(msgs_size);
-            size_t total_sigs_count{0};
-            while (msgs.size() < msgs_size) {
-                msgs.emplace_back();
-                vRecv >> msgs.back();
-                total_sigs_count += msgs.back().sigShares.size();
-                if (total_sigs_count > MAX_MSGS_TOTAL_BATCHED_SIGS) {
-                    throw std::ios_base::failure("QBSIGSHARES sig share count too large");
-                }
-            }
+            msgs = UnserializeBatchedSigShares(vRecv);
         } catch (const std::ios_base::failure& e) {
             LogPrint(BCLog::LLMQ_SIGS, "NetSigning::%s -- rejected %s from peer=%d: %s\n",
                      __func__, msg_type, pfrom.GetId(), e.what());

src/llmq/net_signing.h

@@ -24,6 +24,18 @@ namespace llmq {
 class CSigSharesManager;
 class CSigningManager;
 
+//! Decode a QBSIGSHARES payload into its vector of CBatchedSigShares.
+//!
+//! The inner sigShares vector of each batch is bounded by CBatchedSigShares's
+//! SERIALIZE_METHODS via LIMITED_VECTOR, but many individually-valid batches
+//! could still exceed the total sig-share cap, so this bounds the outer batch
+//! count and checks the running total of inner sig shares as it decodes,
+//! stopping before an attacker forces us through the full cross product of the
+//! per-vector limits. A wire count above the cap (outer batch count or running
+//! inner total) throws std::ios_base::failure before the offending element is
+//! decoded, leaving the caller to log, ban, and rethrow uniformly.
+std::vector<CBatchedSigShares> UnserializeBatchedSigShares(CDataStream& vRecv);
+
 class NetSigning final : public NetHandler, public CValidationInterface
 {
 public:

src/test/llmq_utils_tests.cpp

@@ -6,6 +6,7 @@
 #include <test/util/setup_common.h>
 
 #include <consensus/params.h>
+#include <llmq/net_signing.h>
 #include <llmq/params.h>
 #include <llmq/signing_shares.h>
 #include <llmq/utils.h>
@@ -197,6 +198,61 @@ BOOST_AUTO_TEST_CASE(batched_sig_shares_accepts_max_inner_vector)
     BOOST_CHECK_EQUAL(roundtripped.sigShares.size(), MAX_MSGS_TOTAL_BATCHED_SIGS);
 }
 
+static CBatchedSigShares MakeBatch(uint32_t session_id, size_t share_count)
+{
+    CBatchedSigShares batch;
+    batch.sessionId = session_id;
+    batch.sigShares.resize(share_count); // default pairs are enough to exercise the count invariant
+    return batch;
+}
+
+// Exercise the production QBSIGSHARES decoder directly: the outer batch count is
+// bounded regardless of the inner contents. Every batch here is empty, so the
+// aggregate running-total guard never fires; only the outer-count guard can
+// reject the stream, which keeps this case a genuine test of that guard rather
+// than an end-of-stream artifact.
+BOOST_AUTO_TEST_CASE(qbsigshares_rejects_oversized_batch_count)
+{
+    std::vector<CBatchedSigShares> msgs(MAX_MSGS_TOTAL_BATCHED_SIGS + 1); // count over cap, 0 inner shares each
+
+    CDataStream stream{SER_NETWORK, PROTOCOL_VERSION};
+    stream << msgs;
+
+    BOOST_CHECK_THROW(UnserializeBatchedSigShares(stream), std::ios_base::failure);
+}
+
+// The regression this targets: each batch is individually within the per-batch
+// LIMITED_VECTOR cap, but their running aggregate exceeds MAX_MSGS_TOTAL_BATCHED_SIGS,
+// so the decoder must abort mid-stream rather than accept the cross product.
+BOOST_AUTO_TEST_CASE(qbsigshares_rejects_oversized_aggregate_total)
+{
+    std::vector<CBatchedSigShares> msgs;
+    msgs.push_back(MakeBatch(1, 300));
+    msgs.push_back(MakeBatch(2, 200)); // 300 + 200 = 500 > 400, though each batch is <= the cap
+
+    CDataStream stream{SER_NETWORK, PROTOCOL_VERSION};
+    stream << msgs;
+
+    BOOST_CHECK_THROW(UnserializeBatchedSigShares(stream), std::ios_base::failure);
+}
+
+// Batches whose aggregate is exactly at the cap must be accepted and decoded intact.
+BOOST_AUTO_TEST_CASE(qbsigshares_accepts_aggregate_at_cap)
+{
+    std::vector<CBatchedSigShares> msgs;
+    msgs.push_back(MakeBatch(1, 200));
+    msgs.push_back(MakeBatch(2, MAX_MSGS_TOTAL_BATCHED_SIGS - 200)); // aggregate == cap
+
+    CDataStream stream{SER_NETWORK, PROTOCOL_VERSION};
+    stream << msgs;
+
+    std::vector<CBatchedSigShares> decoded;
+    BOOST_CHECK_NO_THROW(decoded = UnserializeBatchedSigShares(stream));
+    BOOST_CHECK_EQUAL(decoded.size(), 2U);
+    BOOST_CHECK_EQUAL(decoded[0].sigShares.size(), 200U);
+    BOOST_CHECK_EQUAL(decoded[1].sigShares.size(), MAX_MSGS_TOTAL_BATCHED_SIGS - 200);
+}
+
 BOOST_AUTO_TEST_CASE(deterministic_outbound_connection_test)
 {
     // Test deterministic behavior