LLMQ: Bound InstantSend lock intake #1939
+229
−5
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -704,6 +704,11 @@ void CInstantSendManager::ProcessMessage(CNode* pfrom, const std::string& strCom | |
| if (strCommand == NetMsgType::ISLOCK) { | ||
| CInstantSendLock islock; | ||
| vRecv >> islock; | ||
| if (islock.inputs.size() > CInstantSendLock::MAX_INPUTS) { | ||
| LOCK(cs_main); | ||
| Misbehaving(pfrom->id, 100); | ||
| return; | ||
| } | ||
|
Comment on lines
+707
to
+711
There was a problem hiding this comment. Suggestion: The limit is checked only after deserialization, so a peer can still make the parser allocate and process a network-sized input vector before rejection. [resource leak] Assessment: 🟠
Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/llmq/quorums_instantsend.cpp
**Line:** 707:711
**Comment:**
*Resource Leak: The limit is checked only after deserialization, so a peer can still make the parser allocate and process a network-sized input vector before rejection.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
| ProcessMessageInstantSendLock(pfrom, islock, connman); | ||
| } | ||
| } | ||
|
|
@@ -728,6 +733,11 @@ void CInstantSendManager::ProcessMessageInstantSendLock(CNode* pfrom, const llmq | |
| if (pendingInstantSendLocks.count(hash)) { | ||
| return; | ||
| } | ||
| if (pendingInstantSendLocks.size() >= MAX_PENDING_INSTANTSEND_LOCKS) { | ||
| LogPrint("instantsend", "CInstantSendManager::%s -- pending islock queue full (%d), dropping islock=%s, peer=%d\n", | ||
| __func__, pendingInstantSendLocks.size(), hash.ToString(), pfrom->id); | ||
| return; | ||
|
Comment on lines
+736
to
+739
There was a problem hiding this comment.
A connected peer can fill this global first-come queue with distinct one-input locks because AGENTS.md reference: AGENTS.md:L238-L245 Useful? React with 👍 / 👎. |
||
| } | ||
|
|
||
| LogPrint("instantsend", "CInstantSendManager::%s -- txid=%s, islock=%s: received islock, peer=%d\n", __func__, | ||
| islock.txid.ToString(), hash.ToString(), pfrom->id); | ||
|
|
@@ -739,7 +749,7 @@ bool CInstantSendManager::PreVerifyInstantSendLock(NodeId nodeId, const llmq::CI | |
| { | ||
| retBan = false; | ||
|
|
||
| if (islock.txid.IsNull() || islock.inputs.empty() || islock.inputs.size() > CInstantSendLock::MAX_INPUTS) { | ||
| retBan = true; | ||
| return false; | ||
| } | ||
|
|
@@ -761,7 +771,18 @@ bool CInstantSendManager::ProcessPendingInstantSendLocks() | |
|
|
||
| { | ||
| LOCK(cs); | ||
| // Only process 32 locks at a time to avoid duplicate verification of recovered signatures which have been | ||
| // verified by CSigningManager in parallel. | ||
| const size_t maxCount = 32; | ||
|
Comment on lines
+774
to
+776
There was a problem hiding this comment.
When one peer has more than 128 locks that fail verification because they are old enough not to match either active quorum set, this cap splits them across at least five invocations. The existing verification path applies AGENTS.md reference: AGENTS.md:L238-L245 Useful? React with 👍 / 👎. |
||
| if (pendingInstantSendLocks.size() <= maxCount) { | ||
| pend = std::move(pendingInstantSendLocks); | ||
| } else { | ||
| while (pend.size() < maxCount) { | ||
| auto it = pendingInstantSendLocks.begin(); | ||
| pend.emplace(it->first, std::move(it->second)); | ||
| pendingInstantSendLocks.erase(it); | ||
| } | ||
|
Comment on lines
+780
to
+784
There was a problem hiding this comment. Suggestion: Extracted locks are lost when InstantSend is disabled or quorum selection fails, because this partial drain never reinserts Assessment: 🟠
Prompt for AI Agent 🤖This is a comment left during a code review.
**Path:** src/llmq/quorums_instantsend.cpp
**Line:** 780:784
**Comment:**
*Incomplete Implementation: Extracted locks are lost when InstantSend is disabled or quorum selection fails, because this partial drain never reinserts `pend` into `pendingInstantSendLocks`.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix |
||
| } | ||
| } | ||
|
|
||
| if (pend.empty()) { | ||
|
|
@@ -834,8 +855,10 @@ std::unordered_set<uint256> CInstantSendManager::ProcessPendingInstantSendLocks( | |
|
|
||
| auto id = islock.GetRequestId(); | ||
|
|
||
| // no need to verify an ISLOCK if we already have verified the exact recovered sig that belongs to it | ||
| CRecoveredSig recoveredSig; | ||
| if (quorumSigningManager->GetRecoveredSigForId(llmqType, id, recoveredSig) && | ||
| recoveredSig.msgHash == islock.txid && recoveredSig.sig == islock.sig) { | ||
| continue; | ||
| } | ||
|
|
||
|
|
||
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,190 @@ | ||
| // Copyright (c) 2026 The Firo developers | ||
| // Distributed under the MIT software license, see the accompanying | ||
| // file COPYING or http://www.opensource.org/licenses/mit-license.php. | ||
|
|
||
| #include "dbwrapper.h" | ||
| #include "llmq/quorums_instantsend.h" | ||
| #include "net.h" | ||
| #include "test/test_bitcoin.h" | ||
| #include "validation.h" | ||
|
|
||
| #include <boost/test/unit_test.hpp> | ||
|
|
||
| namespace llmq | ||
| { | ||
|
|
||
| struct CInstantSendManagerTestAccess { | ||
| static bool PreVerify(CInstantSendManager& manager, const CInstantSendLock& islock, bool& ban) | ||
| { | ||
| return manager.PreVerifyInstantSendLock(1, islock, ban); | ||
| } | ||
|
|
||
| static void ProcessMessage(CInstantSendManager& manager, CNode& peer, const CInstantSendLock& islock, CConnman& connman) | ||
| { | ||
| manager.ProcessMessageInstantSendLock(&peer, islock, connman); | ||
| } | ||
|
|
||
| static size_t PendingCount(CInstantSendManager& manager) | ||
| { | ||
| LOCK(manager.cs); | ||
| return manager.pendingInstantSendLocks.size(); | ||
| } | ||
|
|
||
| static void ProcessPending(CInstantSendManager& manager) | ||
| { | ||
| manager.ProcessPendingInstantSendLocks(); | ||
| } | ||
|
|
||
| static void ProcessPending(CInstantSendManager& manager, NodeId nodeId, const CInstantSendLock& islock) | ||
| { | ||
| std::unordered_map<uint256, std::pair<NodeId, CInstantSendLock>, StaticSaltedHasher> pending; | ||
| pending.emplace(::SerializeHash(islock), std::make_pair(nodeId, islock)); | ||
| manager.ProcessPendingInstantSendLocks(0, pending, false); | ||
| } | ||
| }; | ||
|
|
||
| } // namespace llmq | ||
|
|
||
| namespace | ||
| { | ||
|
|
||
| uint256 HashFromNonce(size_t nonce) | ||
| { | ||
| return uint256S(strprintf("%064x", static_cast<unsigned int>(nonce))); | ||
| } | ||
|
|
||
| llmq::CInstantSendLock MakeInstantSendLock(size_t nonce) | ||
| { | ||
| llmq::CInstantSendLock islock; | ||
| islock.txid = HashFromNonce(nonce + 1); | ||
| islock.inputs.emplace_back(HashFromNonce(1), static_cast<uint32_t>(nonce)); | ||
| return islock; | ||
| } | ||
|
|
||
| struct InstantSendSetup : BasicTestingSetup { | ||
| CDBWrapper db; | ||
| llmq::CSigningManager signingManager; | ||
| llmq::CInstantSendManager manager; | ||
| CNode peer; | ||
| llmq::CSigningManager* previousSigningManager; | ||
| bool previousTxIndex; | ||
|
|
||
| InstantSendSetup() : db(boost::filesystem::temp_directory_path() / boost::filesystem::unique_path(), 1 << 20, true, false), | ||
| signingManager(db, true), | ||
| manager(db), | ||
| peer(1, NODE_NETWORK, 0, INVALID_SOCKET, CAddress(), 0, 0, "", true), | ||
| previousSigningManager(llmq::quorumSigningManager), | ||
| previousTxIndex(fTxIndex) | ||
| { | ||
| llmq::quorumSigningManager = &signingManager; | ||
| fTxIndex = false; | ||
| g_connman = std::make_unique<CConnman>(0x1337, 0x1337); | ||
| } | ||
|
|
||
| ~InstantSendSetup() | ||
| { | ||
| llmq::quorumSigningManager = previousSigningManager; | ||
| fTxIndex = previousTxIndex; | ||
| } | ||
| }; | ||
|
|
||
| } // namespace | ||
|
|
||
| BOOST_FIXTURE_TEST_SUITE(quorums_instantsend_tests, InstantSendSetup) | ||
|
|
||
| BOOST_AUTO_TEST_CASE(input_limit_is_protocol_derived) | ||
| { | ||
| llmq::CInstantSendLock islock; | ||
| islock.txid = HashFromNonce(1); | ||
| islock.inputs.reserve(llmq::CInstantSendLock::MAX_INPUTS + 1); | ||
|
|
||
| BOOST_CHECK_EQUAL(llmq::CInstantSendLock::MAX_INPUTS, MAX_BLOCK_BASE_SIZE / 41); | ||
| for (size_t i = 0; i < llmq::CInstantSendLock::MAX_INPUTS - 1; ++i) { | ||
| islock.inputs.emplace_back(HashFromNonce(i + 1), 0); | ||
| } | ||
|
|
||
| bool ban = false; | ||
| BOOST_CHECK(llmq::CInstantSendManagerTestAccess::PreVerify(manager, islock, ban)); | ||
| BOOST_CHECK(!ban); | ||
|
|
||
| islock.inputs.emplace_back(HashFromNonce(llmq::CInstantSendLock::MAX_INPUTS), 0); | ||
| BOOST_CHECK(llmq::CInstantSendManagerTestAccess::PreVerify(manager, islock, ban)); | ||
| BOOST_CHECK(!ban); | ||
|
|
||
| islock.inputs.emplace_back(HashFromNonce(llmq::CInstantSendLock::MAX_INPUTS + 1), 0); | ||
| BOOST_CHECK(!llmq::CInstantSendManagerTestAccess::PreVerify(manager, islock, ban)); | ||
| BOOST_CHECK(ban); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(pending_queue_and_processing_are_bounded) | ||
| { | ||
| constexpr size_t maxPending{1024}; | ||
| constexpr size_t maxPerPass{32}; | ||
|
|
||
| for (size_t i = 0; i < maxPending - 1; ++i) { | ||
| const auto islock = MakeInstantSendLock(i); | ||
| llmq::CInstantSendManagerTestAccess::ProcessMessage(manager, peer, islock, *g_connman); | ||
| } | ||
| BOOST_CHECK_EQUAL(llmq::CInstantSendManagerTestAccess::PendingCount(manager), maxPending - 1); | ||
|
|
||
| auto islock = MakeInstantSendLock(maxPending - 1); | ||
| llmq::CInstantSendManagerTestAccess::ProcessMessage(manager, peer, islock, *g_connman); | ||
| BOOST_CHECK_EQUAL(llmq::CInstantSendManagerTestAccess::PendingCount(manager), maxPending); | ||
|
|
||
| islock = MakeInstantSendLock(maxPending); | ||
| llmq::CInstantSendManagerTestAccess::ProcessMessage(manager, peer, islock, *g_connman); | ||
| BOOST_CHECK_EQUAL(llmq::CInstantSendManagerTestAccess::PendingCount(manager), maxPending); | ||
|
|
||
| llmq::CInstantSendManagerTestAccess::ProcessPending(manager); | ||
| BOOST_CHECK_EQUAL(llmq::CInstantSendManagerTestAccess::PendingCount(manager), maxPending - maxPerPass); | ||
|
|
||
| for (size_t i = maxPending; i < maxPending + maxPerPass; ++i) { | ||
| islock = MakeInstantSendLock(i); | ||
| llmq::CInstantSendManagerTestAccess::ProcessMessage(manager, peer, islock, *g_connman); | ||
| } | ||
| BOOST_CHECK_EQUAL(llmq::CInstantSendManagerTestAccess::PendingCount(manager), maxPending); | ||
|
|
||
| islock = MakeInstantSendLock(maxPending + maxPerPass); | ||
| llmq::CInstantSendManagerTestAccess::ProcessMessage(manager, peer, islock, *g_connman); | ||
| BOOST_CHECK_EQUAL(llmq::CInstantSendManagerTestAccess::PendingCount(manager), maxPending); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_CASE(recovered_signature_shortcut_requires_exact_signature) | ||
| { | ||
| auto islock = MakeInstantSendLock(1); | ||
| CBLSSecretKey recoveredKey; | ||
| recoveredKey.MakeNewKey(); | ||
| islock.sig.Set(recoveredKey.Sign(HashFromNonce(100))); | ||
|
|
||
| const auto llmqType = Params().GetConsensus().llmqForInstantSend; | ||
| llmq::CRecoveredSig recoveredSig; | ||
| recoveredSig.llmqType = llmqType; | ||
| recoveredSig.quorumHash = HashFromNonce(101); | ||
| recoveredSig.id = islock.GetRequestId(); | ||
| recoveredSig.msgHash = islock.txid; | ||
| recoveredSig.sig = islock.sig; | ||
| recoveredSig.UpdateHash(); | ||
|
|
||
| llmq::CRecoveredSigsDb recoveredSigsDb(db); | ||
| recoveredSigsDb.WriteRecoveredSig(recoveredSig); | ||
|
|
||
| const auto islockHash = ::SerializeHash(islock); | ||
| llmq::CInstantSendManagerTestAccess::ProcessPending(manager, peer.GetId(), islock); | ||
| BOOST_CHECK_EQUAL(manager.GetInstantSendLockCount(), 1); | ||
|
|
||
| llmq::CInstantSendLock storedLock; | ||
| BOOST_CHECK(db.Read(std::make_tuple(std::string("is_i"), islockHash), storedLock)); | ||
|
|
||
| auto alteredIslock = islock; | ||
| CBLSSecretKey alteredKey; | ||
| alteredKey.MakeNewKey(); | ||
| alteredIslock.sig.Set(alteredKey.Sign(HashFromNonce(102))); | ||
| const auto alteredIslockHash = ::SerializeHash(alteredIslock); | ||
| BOOST_REQUIRE(islockHash != alteredIslockHash); | ||
|
|
||
| llmq::CInstantSendManagerTestAccess::ProcessPending(manager, peer.GetId(), alteredIslock); | ||
| BOOST_CHECK_EQUAL(manager.GetInstantSendLockCount(), 1); | ||
| BOOST_CHECK(!db.Read(std::make_tuple(std::string("is_i"), alteredIslockHash), storedLock)); | ||
| } | ||
|
|
||
| BOOST_AUTO_TEST_SUITE_END() |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an unauthenticated peer sends an
ISLOCKwhose CompactSize input count exceedsMAX_INPUTS,vRecv >> islockhas already allocated and parsed the entire vector before this check runs. The generic vector deserializer accepts counts up toMAX_SIZE, so a maximum-sized network message can force allocation and parsing of hundreds of thousands ofCOutPoints before the peer is banned, leaving the intended intake bound ineffective; read and validate the count before resizing the vector.AGENTS.md reference: AGENTS.md:L238-L245
Useful? React with 👍 / 👎.