diff --git a/qa/rpc-tests/p2p-compactblocks.py b/qa/rpc-tests/p2p-compactblocks.py index 6b5d47713..6d1fb3fd9 100755 --- a/qa/rpc-tests/p2p-compactblocks.py +++ b/qa/rpc-tests/p2p-compactblocks.py @@ -708,6 +708,33 @@ class CompactBlocksTest(BitcoinTestFramework): l.last_cmpctblock.header_and_shortids.header.calc_sha256() assert_equal(l.last_cmpctblock.header_and_shortids.header.sha256, block.sha256) + # Test that we don't get disconnected if we relay a compact block with valid header, + # but invalid transactions. + def test_invalid_tx_in_compactblock(self, node, test_node, use_segwit): + assert(len(self.utxos)) + utxo = self.utxos[0] + + block = self.build_block_with_transactions(node, utxo, 5) + del block.vtx[3] + block.hashMerkleRoot = block.calc_merkle_root() + if use_segwit: + # If we're testing with segwit, also drop the coinbase witness, + # but include the witness commitment. + add_witness_commitment(block) + block.vtx[0].wit.vtxinwit = [] + block.solve() + + # Now send the compact block with all transactions prefilled, and + # verify that we don't get disconnected. + comp_block = HeaderAndShortIDs() + comp_block.initialize_from_block(block, prefill_list=[0, 1, 2, 3, 4], use_witness=use_segwit) + msg = msg_cmpctblock(comp_block.to_p2p()) + test_node.send_and_ping(msg) + + # Check that the tip didn't advance + assert(int(node.getbestblockhash(), 16) is not block.sha256) + test_node.sync_with_ping() + # Helper for enabling cb announcements # Send the sendcmpct request and sync headers def request_cb_announcements(self, peer, node, version): @@ -798,6 +825,11 @@ class CompactBlocksTest(BitcoinTestFramework): self.test_end_to_end_block_relay(self.nodes[0], [self.segwit_node, self.test_node, self.old_node]) self.test_end_to_end_block_relay(self.nodes[1], [self.segwit_node, self.test_node, self.old_node]) + print("\tTesting handling of invalid compact blocks...") + self.test_invalid_tx_in_compactblock(self.nodes[0], self.test_node, False) + self.test_invalid_tx_in_compactblock(self.nodes[1], self.segwit_node, False) + self.test_invalid_tx_in_compactblock(self.nodes[1], self.old_node, False) + # Advance to segwit activation print ("\nAdvancing to segwit activation\n") self.activate_segwit(self.nodes[1]) @@ -844,6 +876,11 @@ class CompactBlocksTest(BitcoinTestFramework): self.request_cb_announcements(self.segwit_node, self.nodes[1], 2) self.test_end_to_end_block_relay(self.nodes[1], [self.segwit_node, self.test_node, self.old_node]) + print("\tTesting handling of invalid compact blocks...") + self.test_invalid_tx_in_compactblock(self.nodes[0], self.test_node, False) + self.test_invalid_tx_in_compactblock(self.nodes[1], self.segwit_node, True) + self.test_invalid_tx_in_compactblock(self.nodes[1], self.old_node, True) + print("\tTesting invalid index in cmpctblock message...") self.test_invalid_cmpctblock_message() diff --git a/src/blockencodings.cpp b/src/blockencodings.cpp index 93d3fa372..737102f16 100644 --- a/src/blockencodings.cpp +++ b/src/blockencodings.cpp @@ -167,7 +167,7 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector< // check its own merkle root and cache that check. if (state.CorruptionPossible()) return READ_STATUS_FAILED; // Possible Short ID collision - return READ_STATUS_INVALID; + return READ_STATUS_CHECKBLOCK_FAILED; } LogPrint("cmpctblock", "Successfully reconstructed block %s with %lu txn prefilled, %lu txn from mempool and %lu txn requested\n", header.GetHash().ToString(), prefilled_count, mempool_count, vtx_missing.size()); diff --git a/src/blockencodings.h b/src/blockencodings.h index 99b1cb140..705eaf28a 100644 --- a/src/blockencodings.h +++ b/src/blockencodings.h @@ -124,6 +124,8 @@ typedef enum ReadStatus_t READ_STATUS_OK, READ_STATUS_INVALID, // Invalid object, peer is sending bogus crap READ_STATUS_FAILED, // Failed to process object + READ_STATUS_CHECKBLOCK_FAILED, // Used only by FillBlock to indicate a + // failure in CheckBlock. } ReadStatus; class CBlockHeaderAndShortTxIDs { diff --git a/src/main.cpp b/src/main.cpp index 2fb143e6a..7e5b9528b 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -180,8 +180,10 @@ namespace { * Sources of received blocks, saved to be able to send them reject * messages or ban them when processing happens afterwards. Protected by * cs_main. + * Set mapBlockSource[hash].second to false if the node should not be + * punished if the block is invalid. */ - map mapBlockSource; + map> mapBlockSource; /** * Filter for transactions that were recently rejected by @@ -3785,7 +3787,7 @@ static bool AcceptBlock(const CBlock& block, CValidationState& state, const CCha return true; } -bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp) +bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp, bool fMayBanPeerIfInvalid) { { LOCK(cs_main); @@ -3795,7 +3797,7 @@ bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, C bool fNewBlock = false; bool ret = AcceptBlock(*pblock, state, chainparams, &pindex, fForceProcessing, dbp, &fNewBlock); if (pindex && pfrom) { - mapBlockSource[pindex->GetBlockHash()] = pfrom->GetId(); + mapBlockSource[pindex->GetBlockHash()] = std::make_pair(pfrom->GetId(), fMayBanPeerIfInvalid); if (fNewBlock) pfrom->nLastBlockTime = GetTime(); } CheckBlockIndex(chainparams.GetConsensus()); @@ -4775,16 +4777,16 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const CValidationSta LOCK(cs_main); const uint256 hash(block.GetHash()); - std::map::iterator it = mapBlockSource.find(hash); + std::map>::iterator it = mapBlockSource.find(hash); int nDoS = 0; if (state.IsInvalid(nDoS)) { - if (it != mapBlockSource.end() && State(it->second)) { + if (it != mapBlockSource.end() && State(it->second.first)) { assert (state.GetRejectCode() < REJECT_INTERNAL); // Blocks are never rejected with internal reject codes CBlockReject reject = {(unsigned char)state.GetRejectCode(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), hash}; - State(it->second)->rejects.push_back(reject); - if (nDoS > 0) - Misbehaving(it->second, nDoS); + State(it->second.first)->rejects.push_back(reject); + if (nDoS > 0 && it->second.second) + Misbehaving(it->second.first, nDoS); } } if (it != mapBlockSource.end()) @@ -5893,6 +5895,23 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, invs.push_back(CInv(MSG_BLOCK | GetFetchFlags(pfrom, chainActive.Tip(), chainparams.GetConsensus()), resp.blockhash)); connman.PushMessage(pfrom, NetMsgType::GETDATA, invs); } else { + // Block is either okay, or possibly we received + // READ_STATUS_CHECKBLOCK_FAILED. + // Note that CheckBlock can only fail for one of a few reasons: + // 1. bad-proof-of-work (impossible here, because we've already + // accepted the header) + // 2. merkleroot doesn't match the transactions given (already + // caught in FillBlock with READ_STATUS_FAILED, so + // impossible here) + // 3. the block is otherwise invalid (eg invalid coinbase, + // block is too big, too many legacy sigops, etc). + // So if CheckBlock failed, #3 is the only possibility. + // Under BIP 152, we don't DoS-ban unless proof of work is + // invalid (we don't require all the stateless checks to have + // been run). This is handled below, so just treat this as + // though the block was successfully read, and rely on the + // handling in ProcessNewBlock to ensure the block index is + // updated, reject messages go out, etc. MarkBlockAsReceived(resp.blockhash); // it is now an empty pointer fBlockRead = true; } @@ -5901,16 +5920,15 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, CValidationState state; // Since we requested this block (it was in mapBlocksInFlight), force it to be processed, // even if it would not be a candidate for new tip (missing previous block, chain not long enough, etc) - ProcessNewBlock(state, chainparams, pfrom, &block, true, NULL); + // BIP 152 permits peers to relay compact blocks after validating + // the header only; we should not punish peers if the block turns + // out to be invalid. + ProcessNewBlock(state, chainparams, pfrom, &block, true, NULL, false); int nDoS; if (state.IsInvalid(nDoS)) { assert (state.GetRejectCode() < REJECT_INTERNAL); // Blocks are never rejected with internal reject codes connman.PushMessage(pfrom, NetMsgType::REJECT, strCommand, (unsigned char)state.GetRejectCode(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), block.GetHash()); - if (nDoS > 0) { - LOCK(cs_main); - Misbehaving(pfrom->GetId(), nDoS); - } } } } @@ -6081,7 +6099,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv, // need it even though it is not a candidate for a new best tip. forceProcessing |= MarkBlockAsReceived(block.GetHash()); } - ProcessNewBlock(state, chainparams, pfrom, &block, forceProcessing, NULL); + ProcessNewBlock(state, chainparams, pfrom, &block, forceProcessing, NULL, true); int nDoS; if (state.IsInvalid(nDoS)) { assert (state.GetRejectCode() < REJECT_INTERNAL); // Blocks are never rejected with internal reject codes diff --git a/src/main.h b/src/main.h index e80314a64..934333058 100644 --- a/src/main.h +++ b/src/main.h @@ -223,7 +223,7 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024; * @param[out] dbp The already known disk position of pblock, or NULL if not yet stored. * @return True if state.IsValid() */ -bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp); +bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp, bool fMayBanPeerIfInvalid); /** Check whether enough disk space is available for an incoming block */ bool CheckDiskSpace(uint64_t nAdditionalBytes = 0); /** Open a block file (blk?????.dat) */ diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index f418262f0..3e91a79a6 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -132,7 +132,7 @@ UniValue generateBlocks(boost::shared_ptr coinbaseScript, int nG continue; } CValidationState state; - if (!ProcessNewBlock(state, Params(), NULL, pblock, true, NULL)) + if (!ProcessNewBlock(state, Params(), NULL, pblock, true, NULL, false)) throw JSONRPCError(RPC_INTERNAL_ERROR, "ProcessNewBlock, block not accepted"); ++nHeight; blockHashes.push_back(pblock->GetHash().GetHex()); @@ -757,7 +757,7 @@ UniValue submitblock(const JSONRPCRequest& request) CValidationState state; submitblock_StateCatcher sc(block.GetHash()); RegisterValidationInterface(&sc); - bool fAccepted = ProcessNewBlock(state, Params(), NULL, &block, true, NULL); + bool fAccepted = ProcessNewBlock(state, Params(), NULL, &block, true, NULL, false); UnregisterValidationInterface(&sc); if (fBlockPresent) { diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index a94979fd7..2762cafa3 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -223,7 +223,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) pblock->hashMerkleRoot = BlockMerkleRoot(*pblock); pblock->nNonce = blockinfo[i].nonce; CValidationState state; - BOOST_CHECK(ProcessNewBlock(state, chainparams, NULL, pblock, true, NULL)); + BOOST_CHECK(ProcessNewBlock(state, chainparams, NULL, pblock, true, NULL, false)); BOOST_CHECK(state.IsValid()); pblock->hashPrevBlock = pblock->GetHash(); } diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp index 98f4ed939..3da0be8ca 100644 --- a/src/test/test_bitcoin.cpp +++ b/src/test/test_bitcoin.cpp @@ -127,7 +127,7 @@ TestChain100Setup::CreateAndProcessBlock(const std::vector& while (!CheckProofOfWork(block.GetHash(), block.nBits, chainparams.GetConsensus())) ++block.nNonce; CValidationState state; - ProcessNewBlock(state, chainparams, NULL, &block, true, NULL); + ProcessNewBlock(state, chainparams, NULL, &block, true, NULL, false); CBlock result = block; return result; diff --git a/src/version.h b/src/version.h index 87bd65506..87fb1a3a7 100644 --- a/src/version.h +++ b/src/version.h @@ -9,7 +9,7 @@ * network protocol versioning */ -static const int PROTOCOL_VERSION = 70014; +static const int PROTOCOL_VERSION = 70015; //! initial proto version, to be increased after version/verack negotiation static const int INIT_PROTO_VERSION = 209; @@ -42,4 +42,7 @@ static const int FEEFILTER_VERSION = 70013; //! short-id-based block download starts with this version static const int SHORT_IDS_BLOCKS_VERSION = 70014; +//! not banning for invalid compact blocks starts with this version +static const int INVALID_CB_NO_BAN_VERSION = 70015; + #endif // BITCOIN_VERSION_H