diff --git a/src/net_processing.cpp b/src/net_processing.cpp index f42a26ca3ea4..c5e9cab5689c 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -430,9 +430,18 @@ static void PushNodeVersion(CNode *pnode, CConnman* connman, int64_t nTime) } } -// Returns a bool indicating whether we requested this block. -// Also used if a block was /not/ received and timed out or started with another peer -static bool MarkBlockAsReceived(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { +/** + * Mark a block as no longer 'in flight' from any node. Called when the block + * has been received, if it's timed out, of if we're requesting it from another + * peer. + * + * Clear the block from the global mapBlocksInFlight map and any node's + * vBlocksInFlight vector. + * + * @param[in] hash The block hash + * @return bool Whether this block was already in flight from a peer. + */ +static bool MarkBlockAsNotInFlight(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { std::map::iterator> >::iterator itInFlight = mapBlocksInFlight.find(hash); if (itInFlight != mapBlocksInFlight.end()) { CNodeState *state = State(itInFlight->second.first); @@ -455,8 +464,23 @@ static bool MarkBlockAsReceived(const uint256& hash) EXCLUSIVE_LOCKS_REQUIRED(cs return false; } -// returns false, still setting pit, if the block was already in flight from the same peer -// pit will only be valid as long as the same cs_main lock is being held +/** + * Mark a blocks as 'in flight' from a specific peer. This can be called either when + * we send a GETDATA(BLOCK) or GETDATA(CMPCTBLOCK) to the peer, or when we receive + * a CMPCTBLOCK from a peer. + * + * Add the block to the global mapBlocksInFlight map and to the node's vBlocksInFlight vector + * + * @param[in] nodeid The node that we send the GETDATA to or received the CMPCTBLOCKFROM + * @param[in] hash The block hash + * @param[in] pindex The block's BlockIndex + * @param[in/out] pit Pointer to iterator to QueuedBlock. Only used when this function + * is called during compact block handling. pit is only valid for + * as long as cs_main is held. + * @return bool Only used when this function is called during compact block processing + * False indicates the block was already in flight from the same node. + * Returns true otherwise. + */ static bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlockIndex* pindex = nullptr, std::list::iterator** pit = nullptr) EXCLUSIVE_LOCKS_REQUIRED(cs_main) { CNodeState *state = State(nodeid); assert(state != nullptr); @@ -471,7 +495,7 @@ static bool MarkBlockAsInFlight(NodeId nodeid, const uint256& hash, const CBlock } // Make sure it's not listed somewhere already. - MarkBlockAsReceived(hash); + MarkBlockAsNotInFlight(hash); std::list::iterator it = state->vBlocksInFlight.insert(state->vBlocksInFlight.end(), {hash, pindex, pindex != nullptr, std::unique_ptr(pit ? new PartiallyDownloadedBlock(&mempool) : nullptr)}); @@ -1129,8 +1153,24 @@ PeerLogicValidation::PeerLogicValidation(CConnman* connmanIn, BanMan* banman, CS * block. Also save the time of the last tip update. */ void PeerLogicValidation::BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex, const std::vector& vtxConflicted) { - LOCK(g_cs_orphans); + { + LOCK(cs_main); + const uint256 hash(pblock->GetHash()); + std::map>::iterator it = mapBlockSource.find(hash); + + if (it != mapBlockSource.end()) { + // If we're not in IBD, mark the peer that sent us this block a compact block source + if (!::ChainstateActive().IsInitialBlockDownload()) { + MaybeSetPeerAsAnnouncingHeaderAndIDs(it->second.first, connman); + } + + // We no longer need to track the originator of this block + mapBlockSource.erase(it); + } + } + + LOCK(g_cs_orphans); std::vector vOrphanErase; for (const CTransactionRef& ptx : pblock->vtx) { @@ -1249,37 +1289,30 @@ void PeerLogicValidation::UpdatedBlockTip(const CBlockIndex *pindexNew, const CB } /** - * Handle invalid block rejection and consequent peer banning, maintain which - * peers announce compact blocks. + * Handle invalid block rejection and consequent peer banning. + * + * Called both in case of cursory DoS checks failing (implying the peer is likely + * sending us bogus data) and after full validation of the block fails (which may + * be OK if it was sent over compact blocks). */ -void PeerLogicValidation::BlockChecked(const CBlock& block, const BlockValidationState& state) { +static void BlockChecked(const CBlock& block, const BlockValidationState& state, CConnman* connman) { LOCK(cs_main); const uint256 hash(block.GetHash()); std::map>::iterator it = mapBlockSource.find(hash); - // If the block failed validation, we know where it came from and we're still connected - // to that peer, maybe punish. - if (state.IsInvalid() && - it != mapBlockSource.end() && - State(it->second.first)) { + // If we know where the block came from and we're still connected to that + // peer, maybe punish. + if (it != mapBlockSource.end()) { + if (State(it->second.first)) { MaybePunishNodeForBlock(/*nodeid=*/ it->second.first, state, /*via_compact_block=*/ !it->second.second); - } - // Check that: - // 1. The block is valid - // 2. We're not in initial block download - // 3. This is currently the best block we're aware of. We haven't updated - // the tip yet so we have no way to check this directly here. Instead we - // just check that there are currently no other blocks in flight. - else if (state.IsValid() && - !::ChainstateActive().IsInitialBlockDownload() && - mapBlocksInFlight.count(hash) == mapBlocksInFlight.size()) { - if (it != mapBlockSource.end()) { - MaybeSetPeerAsAnnouncingHeaderAndIDs(it->second.first, connman); } - } - if (it != mapBlockSource.end()) mapBlockSource.erase(it); + } +} + +void PeerLogicValidation::BlockFailedConnection(const CBlock& block, const BlockValidationState& state) { + ::BlockChecked(block, state, connman); } ////////////////////////////////////////////////////////////////////////////// @@ -1880,6 +1913,27 @@ void static ProcessOrphanTx(CConnman* connman, std::set& orphan_work_se } } +/** + * A block has been processed. If the block was failed anti-dos / mutation checks, then + * call BlockChecked() to maybe punish peer. If this is the first time we've seen the block, + * update the node's nLastBlockTime. Otherwise erase it from mapBlockSource. + */ +void static BlockProcessed(CNode* pfrom, CConnman* connman, std::shared_ptr pblock, BlockValidationState& state, bool new_block) +{ + if (!state.IsValid()) { + // The block failed anti-dos / mutation checks. Call BlockChecked() callback here. + // This clears the block from mapBlockSource. + BlockChecked(*pblock, state, connman); + } else if (!new_block) { + // Block was valid but we've seen it before. Clear it from mapBlockSource. + LOCK(cs_main); + ::mapBlockSource.erase(pblock->GetHash()); + } else { + // Block is valid and we haven't seen it before. set nLastBlockTime for this peer. + pfrom->nLastBlockTime = GetTime(); + } +} + bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStream& vRecv, int64_t nTimeReceived, const CChainParams& chainparams, CConnman* connman, BanMan* banman, const std::atomic& interruptMsgProc) { LogPrint(BCLog::NET, "received: %s (%u bytes) peer=%d\n", SanitizeString(strCommand), vRecv.size(), pfrom->GetId()); @@ -2730,7 +2784,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr PartiallyDownloadedBlock& partialBlock = *(*queuedBlockIt)->partialBlock; ReadStatus status = partialBlock.InitData(cmpctblock, vExtraTxnForCompact); if (status == READ_STATUS_INVALID) { - MarkBlockAsReceived(pindex->GetBlockHash()); // Reset in-flight state in case of whitelist + MarkBlockAsNotInFlight(pindex->GetBlockHash()); // Reset in-flight state in case of whitelist Misbehaving(pfrom->GetId(), 100, strprintf("Peer %d sent us invalid compact block\n", pfrom->GetId())); return true; } else if (status == READ_STATUS_FAILED) { @@ -2818,20 +2872,20 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr // we have a chain with at least nMinimumChainWork), and we ignore // compact blocks with less work than our tip, it is safe to treat // reconstructed compact blocks as having been requested. - ProcessNewBlock(chainparams, pblock, /*fForceProcessing=*/true, &fNewBlock); - if (fNewBlock) { - pfrom->nLastBlockTime = GetTime(); - } else { + BlockValidationState dos_state; + ProcessNewBlock(chainparams, pblock, dos_state, /*fForceProcessing=*/true, &fNewBlock); + BlockProcessed(pfrom, connman, pblock, dos_state, fNewBlock); + + if (dos_state.IsValid()) { + // This block has passed anti-dos / mutation checks in + // ProcessNewBlock(). Clear the download state for the block, + // which is in flight from some other peer. + // + // We do this after checking for mutation so that a mutated + // cmpctblock announcement can't be used to interfere with + // block relay. LOCK(cs_main); - mapBlockSource.erase(pblock->GetHash()); - } - LOCK(cs_main); // hold cs_main for CBlockIndex::IsValid() - if (pindex->IsValid(BLOCK_VALID_TRANSACTIONS)) { - // Clear download state for this block, which is in - // process from some other peer. We do this after calling - // ProcessNewBlock so that a malleated cmpctblock announcement - // can't be used to interfere with block relay. - MarkBlockAsReceived(pblock->GetHash()); + MarkBlockAsNotInFlight(pblock->GetHash()); } } return true; @@ -2863,7 +2917,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr PartiallyDownloadedBlock& partialBlock = *it->second.second->partialBlock; ReadStatus status = partialBlock.FillBlock(*pblock, resp.txn); if (status == READ_STATUS_INVALID) { - MarkBlockAsReceived(resp.blockhash); // Reset in-flight state in case of whitelist + MarkBlockAsNotInFlight(resp.blockhash); // Reset in-flight state in case of whitelist Misbehaving(pfrom->GetId(), 100, strprintf("Peer %d sent us invalid compact block/non-matching block transactions\n", pfrom->GetId())); return true; } else if (status == READ_STATUS_FAILED) { @@ -2889,7 +2943,7 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr // though the block was successfully read, and rely on the // handling in ProcessNewBlock to ensure the block index is // updated, etc. - MarkBlockAsReceived(resp.blockhash); // it is now an empty pointer + MarkBlockAsNotInFlight(resp.blockhash); // it is now an empty pointer fBlockRead = true; // mapBlockSource is used for potentially punishing peers and // updating which peers send us compact blocks, so the race @@ -2908,13 +2962,9 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr // disk-space attacks), but this should be safe due to the // protections in the compact block handler -- see related comment // in compact block optimistic reconstruction handling. - ProcessNewBlock(chainparams, pblock, /*fForceProcessing=*/true, &fNewBlock); - if (fNewBlock) { - pfrom->nLastBlockTime = GetTime(); - } else { - LOCK(cs_main); - mapBlockSource.erase(pblock->GetHash()); - } + BlockValidationState dos_state; + ProcessNewBlock(chainparams, pblock, dos_state, /*fForceProcessing=*/true, &fNewBlock); + BlockProcessed(pfrom, connman, pblock, dos_state, fNewBlock); } return true; } @@ -2964,20 +3014,16 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr LOCK(cs_main); // Also always process if we requested the block explicitly, as we may // need it even though it is not a candidate for a new best tip. - forceProcessing |= MarkBlockAsReceived(hash); + forceProcessing |= MarkBlockAsNotInFlight(hash); // mapBlockSource is only used for punishing peers and setting // which peers send us compact blocks, so the race between here and // cs_main in ProcessNewBlock is fine. mapBlockSource.emplace(hash, std::make_pair(pfrom->GetId(), true)); } bool fNewBlock = false; - ProcessNewBlock(chainparams, pblock, forceProcessing, &fNewBlock); - if (fNewBlock) { - pfrom->nLastBlockTime = GetTime(); - } else { - LOCK(cs_main); - mapBlockSource.erase(pblock->GetHash()); - } + BlockValidationState dos_state; + ProcessNewBlock(chainparams, pblock, dos_state, forceProcessing, &fNewBlock); + BlockProcessed(pfrom, connman, pblock, dos_state, fNewBlock); return true; } diff --git a/src/net_processing.h b/src/net_processing.h index 4adb7d3a2151..9a34e2e55f75 100644 --- a/src/net_processing.h +++ b/src/net_processing.h @@ -40,7 +40,7 @@ class PeerLogicValidation final : public CValidationInterface, public NetEventsI /** * Overridden from CValidationInterface. */ - void BlockChecked(const CBlock& block, const BlockValidationState& state) override; + void BlockFailedConnection(const CBlock& block, const BlockValidationState& state) override; /** * Overridden from CValidationInterface. */ diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index ab22155651e1..c5e7e1ceb77c 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -135,7 +135,8 @@ static UniValue generateBlocks(const CScript& coinbase_script, int nGenerate, ui continue; } std::shared_ptr shared_pblock = std::make_shared(*pblock); - if (!ProcessNewBlock(Params(), shared_pblock, true, nullptr)) + BlockValidationState state; + if (!ProcessNewBlock(Params(), shared_pblock, state, true, nullptr) || !state.IsValid()) throw JSONRPCError(RPC_INTERNAL_ERROR, "ProcessNewBlock, block not accepted"); ++nHeight; blockHashes.push_back(pblock->GetHash().GetHex()); @@ -717,12 +718,17 @@ class submitblock_StateCatcher : public CValidationInterface explicit submitblock_StateCatcher(const uint256 &hashIn) : hash(hashIn), found(false), state() {} protected: - void BlockChecked(const CBlock& block, const BlockValidationState& stateIn) override { + void BlockFailedConnection(const CBlock& block, const BlockValidationState& stateIn) override { if (block.GetHash() != hash) return; found = true; state = stateIn; } + + void BlockConnected(const std::shared_ptr& pblock, const CBlockIndex* pindex, const std::vector& vtxConflicted) override { + if (pblock->GetHash() != hash) return; + found = true; + } }; static UniValue submitblock(const JSONRPCRequest& request) @@ -777,11 +783,18 @@ static UniValue submitblock(const JSONRPCRequest& request) bool new_block; submitblock_StateCatcher sc(block.GetHash()); RegisterValidationInterface(&sc); - bool accepted = ProcessNewBlock(Params(), blockptr, /* fForceProcessing */ true, /* fNewBlock */ &new_block); + BlockValidationState dos_state; + bool accepted = ProcessNewBlock(Params(), blockptr, dos_state, /* fForceProcessing */ true, /* fNewBlock */ &new_block); + + // Make sure the validation interface queue has drained (so we get any relevant BlockConnected/BlockFailedConnection callbacks) + SyncWithValidationInterfaceQueue(); UnregisterValidationInterface(&sc); if (!new_block && accepted) { return "duplicate"; } + if (!dos_state.IsValid()) { + return BIP22ValidationResult(dos_state); + } if (!sc.found) { return "inconclusive"; } diff --git a/src/test/blockfilter_index_tests.cpp b/src/test/blockfilter_index_tests.cpp index 2b616f4c2ba7..e85234a2040e 100644 --- a/src/test/blockfilter_index_tests.cpp +++ b/src/test/blockfilter_index_tests.cpp @@ -164,7 +164,9 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, TestChain100Setup) uint256 chainA_last_header = last_header; for (size_t i = 0; i < 2; i++) { const auto& block = chainA[i]; - BOOST_REQUIRE(ProcessNewBlock(Params(), block, true, nullptr)); + BlockValidationState dos_state; + BOOST_REQUIRE(ProcessNewBlock(Params(), block, dos_state, true, nullptr)); + BOOST_REQUIRE(dos_state.IsValid()); } for (size_t i = 0; i < 2; i++) { const auto& block = chainA[i]; @@ -182,7 +184,9 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, TestChain100Setup) uint256 chainB_last_header = last_header; for (size_t i = 0; i < 3; i++) { const auto& block = chainB[i]; - BOOST_REQUIRE(ProcessNewBlock(Params(), block, true, nullptr)); + BlockValidationState dos_state; + BOOST_REQUIRE(ProcessNewBlock(Params(), block, dos_state, true, nullptr)); + BOOST_REQUIRE(dos_state.IsValid()); } for (size_t i = 0; i < 3; i++) { const auto& block = chainB[i]; @@ -211,10 +215,12 @@ BOOST_FIXTURE_TEST_CASE(blockfilter_index_initial_sync, TestChain100Setup) } // Reorg back to chain A. - for (size_t i = 2; i < 4; i++) { - const auto& block = chainA[i]; - BOOST_REQUIRE(ProcessNewBlock(Params(), block, true, nullptr)); - } + for (size_t i = 2; i < 4; i++) { + const auto& block = chainA[i]; + BlockValidationState dos_state; + BOOST_REQUIRE(ProcessNewBlock(Params(), block, dos_state, true, nullptr)); + BOOST_REQUIRE(dos_state.IsValid()); + } // Check that chain A and B blocks can be retrieved. chainA_last_header = last_header; diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index 632151793ec0..21c8c34323bb 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -98,11 +98,11 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) // Test starts here { - LOCK2(cs_main, dummyNode1.cs_sendProcessing); + LOCK(dummyNode1.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); // should result in getheaders } { - LOCK2(cs_main, dummyNode1.cs_vSend); + LOCK(dummyNode1.cs_vSend); BOOST_CHECK(dummyNode1.vSendMsg.size() > 0); dummyNode1.vSendMsg.clear(); } @@ -111,17 +111,17 @@ BOOST_AUTO_TEST_CASE(outbound_slow_chain_eviction) // Wait 21 minutes SetMockTime(nStartTime+21*60); { - LOCK2(cs_main, dummyNode1.cs_sendProcessing); + LOCK(dummyNode1.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); // should result in getheaders } { - LOCK2(cs_main, dummyNode1.cs_vSend); + LOCK(dummyNode1.cs_vSend); BOOST_CHECK(dummyNode1.vSendMsg.size() > 0); } // Wait 3 more minutes SetMockTime(nStartTime+24*60); { - LOCK2(cs_main, dummyNode1.cs_sendProcessing); + LOCK(dummyNode1.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); // should result in disconnect } BOOST_CHECK(dummyNode1.fDisconnect == true); @@ -235,7 +235,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning) Misbehaving(dummyNode1.GetId(), 100); // Should get banned } { - LOCK2(cs_main, dummyNode1.cs_sendProcessing); + LOCK(dummyNode1.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); } BOOST_CHECK(banman->IsBanned(addr1)); @@ -252,7 +252,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning) Misbehaving(dummyNode2.GetId(), 50); } { - LOCK2(cs_main, dummyNode2.cs_sendProcessing); + LOCK(dummyNode2.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode2)); } BOOST_CHECK(!banman->IsBanned(addr2)); // 2 not banned yet... @@ -262,7 +262,7 @@ BOOST_AUTO_TEST_CASE(DoS_banning) Misbehaving(dummyNode2.GetId(), 50); } { - LOCK2(cs_main, dummyNode2.cs_sendProcessing); + LOCK(dummyNode2.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode2)); } BOOST_CHECK(banman->IsBanned(addr2)); @@ -291,7 +291,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore) Misbehaving(dummyNode1.GetId(), 100); } { - LOCK2(cs_main, dummyNode1.cs_sendProcessing); + LOCK(dummyNode1.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); } BOOST_CHECK(!banman->IsBanned(addr1)); @@ -300,7 +300,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore) Misbehaving(dummyNode1.GetId(), 10); } { - LOCK2(cs_main, dummyNode1.cs_sendProcessing); + LOCK(dummyNode1.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); } BOOST_CHECK(!banman->IsBanned(addr1)); @@ -309,7 +309,7 @@ BOOST_AUTO_TEST_CASE(DoS_banscore) Misbehaving(dummyNode1.GetId(), 1); } { - LOCK2(cs_main, dummyNode1.cs_sendProcessing); + LOCK(dummyNode1.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode1)); } BOOST_CHECK(banman->IsBanned(addr1)); @@ -341,7 +341,7 @@ BOOST_AUTO_TEST_CASE(DoS_bantime) Misbehaving(dummyNode.GetId(), 100); } { - LOCK2(cs_main, dummyNode.cs_sendProcessing); + LOCK(dummyNode.cs_sendProcessing); BOOST_CHECK(peerLogic->SendMessages(&dummyNode)); } BOOST_CHECK(banman->IsBanned(addr)); diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 6ed7350ea237..b1451420df95 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -7,6 +7,7 @@ #include #include #include +#include #include #include #include