Skip to content

Commit dc6b940

Browse files
sipasipa
committed
Merge bitcoin#9026: Fix handling of invalid compact blocks
d4833ff Bump the protocol version to distinguish new banning behavior. (Suhas Daftuar) 88c3549 Fix compact block handling to not ban if block is invalid (Suhas Daftuar) c93beac [qa] Test that invalid compactblocks don't result in ban (Suhas Daftuar)
2 parents 9f554e0 + d4833ff commit dc6b940

File tree

9 files changed

+81
-21
lines changed

9 files changed

+81
-21
lines changed

‎qa/rpc-tests/p2p-compactblocks.py‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -708,6 +708,33 @@ def test_end_to_end_block_relay(self, node, listeners):
708708
l.last_cmpctblock.header_and_shortids.header.calc_sha256()
709709
assert_equal(l.last_cmpctblock.header_and_shortids.header.sha256, block.sha256)
710710

711+
# Test that we don't get disconnected if we relay a compact block with valid header,
712+
# but invalid transactions.
713+
def test_invalid_tx_in_compactblock(self, node, test_node, use_segwit):
714+
assert(len(self.utxos))
715+
utxo = self.utxos[0]
716+
717+
block = self.build_block_with_transactions(node, utxo, 5)
718+
del block.vtx[3]
719+
block.hashMerkleRoot = block.calc_merkle_root()
720+
if use_segwit:
721+
# If we're testing with segwit, also drop the coinbase witness,
722+
# but include the witness commitment.
723+
add_witness_commitment(block)
724+
block.vtx[0].wit.vtxinwit = []
725+
block.solve()
726+
727+
# Now send the compact block with all transactions prefilled, and
728+
# verify that we don't get disconnected.
729+
comp_block = HeaderAndShortIDs()
730+
comp_block.initialize_from_block(block, prefill_list=[0, 1, 2, 3, 4], use_witness=use_segwit)
731+
msg = msg_cmpctblock(comp_block.to_p2p())
732+
test_node.send_and_ping(msg)
733+
734+
# Check that the tip didn't advance
735+
assert(int(node.getbestblockhash(), 16) is not block.sha256)
736+
test_node.sync_with_ping()
737+
711738
# Helper for enabling cb announcements
712739
# Send the sendcmpct request and sync headers
713740
def request_cb_announcements(self, peer, node, version):
@@ -798,6 +825,11 @@ def run_test(self):
798825
self.test_end_to_end_block_relay(self.nodes[0], [self.segwit_node, self.test_node, self.old_node])
799826
self.test_end_to_end_block_relay(self.nodes[1], [self.segwit_node, self.test_node, self.old_node])
800827

828+
print("\tTesting handling of invalid compact blocks...")
829+
self.test_invalid_tx_in_compactblock(self.nodes[0], self.test_node, False)
830+
self.test_invalid_tx_in_compactblock(self.nodes[1], self.segwit_node, False)
831+
self.test_invalid_tx_in_compactblock(self.nodes[1], self.old_node, False)
832+
801833
# Advance to segwit activation
802834
print ("\nAdvancing to segwit activation\n")
803835
self.activate_segwit(self.nodes[1])
@@ -844,6 +876,11 @@ def run_test(self):
844876
self.request_cb_announcements(self.segwit_node, self.nodes[1], 2)
845877
self.test_end_to_end_block_relay(self.nodes[1], [self.segwit_node, self.test_node, self.old_node])
846878

879+
print("\tTesting handling of invalid compact blocks...")
880+
self.test_invalid_tx_in_compactblock(self.nodes[0], self.test_node, False)
881+
self.test_invalid_tx_in_compactblock(self.nodes[1], self.segwit_node, True)
882+
self.test_invalid_tx_in_compactblock(self.nodes[1], self.old_node, True)
883+
847884
print("\tTesting invalid index in cmpctblock message...")
848885
self.test_invalid_cmpctblock_message()
849886

‎src/blockencodings.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -167,7 +167,7 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
167167
// check its own merkle root and cache that check.
168168
if (state.CorruptionPossible())
169169
return READ_STATUS_FAILED; // Possible Short ID collision
170-
return READ_STATUS_INVALID;
170+
return READ_STATUS_CHECKBLOCK_FAILED;
171171
}
172172

173173
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());

‎src/blockencodings.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -124,6 +124,8 @@ typedef enum ReadStatus_t
124124
READ_STATUS_OK,
125125
READ_STATUS_INVALID, // Invalid object, peer is sending bogus crap
126126
READ_STATUS_FAILED, // Failed to process object
127+
READ_STATUS_CHECKBLOCK_FAILED, // Used only by FillBlock to indicate a
128+
// failure in CheckBlock.
127129
} ReadStatus;
128130

129131
class CBlockHeaderAndShortTxIDs {

‎src/main.cpp‎

Lines changed: 32 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -180,8 +180,10 @@ namespace {
180180
* Sources of received blocks, saved to be able to send them reject
181181
* messages or ban them when processing happens afterwards. Protected by
182182
* cs_main.
183+
* Set mapBlockSource[hash].second to false if the node should not be
184+
* punished if the block is invalid.
183185
*/
184-
map<uint256, NodeId> mapBlockSource;
186+
map<uint256, std::pair<NodeId, bool>> mapBlockSource;
185187

186188
/**
187189
* Filter for transactions that were recently rejected by
@@ -3785,7 +3787,7 @@ static bool AcceptBlock(const CBlock& block, CValidationState& state, const CCha
37853787
return true;
37863788
}
37873789

3788-
bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp)
3790+
bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp, bool fMayBanPeerIfInvalid)
37893791
{
37903792
{
37913793
LOCK(cs_main);
@@ -3795,7 +3797,7 @@ bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, C
37953797
bool fNewBlock = false;
37963798
bool ret = AcceptBlock(*pblock, state, chainparams, &pindex, fForceProcessing, dbp, &fNewBlock);
37973799
if (pindex && pfrom) {
3798-
mapBlockSource[pindex->GetBlockHash()] = pfrom->GetId();
3800+
mapBlockSource[pindex->GetBlockHash()] = std::make_pair(pfrom->GetId(), fMayBanPeerIfInvalid);
37993801
if (fNewBlock) pfrom->nLastBlockTime = GetTime();
38003802
}
38013803
CheckBlockIndex(chainparams.GetConsensus());
@@ -4775,16 +4777,16 @@ void PeerLogicValidation::BlockChecked(const CBlock& block, const CValidationSta
47754777
LOCK(cs_main);
47764778

47774779
const uint256 hash(block.GetHash());
4778-
std::map<uint256, NodeId>::iterator it = mapBlockSource.find(hash);
4780+
std::map<uint256, std::pair<NodeId, bool>>::iterator it = mapBlockSource.find(hash);
47794781

47804782
int nDoS = 0;
47814783
if (state.IsInvalid(nDoS)) {
4782-
if (it != mapBlockSource.end() && State(it->second)) {
4784+
if (it != mapBlockSource.end() && State(it->second.first)) {
47834785
assert (state.GetRejectCode() < REJECT_INTERNAL); // Blocks are never rejected with internal reject codes
47844786
CBlockReject reject = {(unsigned char)state.GetRejectCode(), state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), hash};
4785-
State(it->second)->rejects.push_back(reject);
4786-
if (nDoS > 0)
4787-
Misbehaving(it->second, nDoS);
4787+
State(it->second.first)->rejects.push_back(reject);
4788+
if (nDoS > 0 && it->second.second)
4789+
Misbehaving(it->second.first, nDoS);
47884790
}
47894791
}
47904792
if (it != mapBlockSource.end())
@@ -5893,6 +5895,23 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
58935895
invs.push_back(CInv(MSG_BLOCK | GetFetchFlags(pfrom, chainActive.Tip(), chainparams.GetConsensus()), resp.blockhash));
58945896
connman.PushMessage(pfrom, NetMsgType::GETDATA, invs);
58955897
} else {
5898+
// Block is either okay, or possibly we received
5899+
// READ_STATUS_CHECKBLOCK_FAILED.
5900+
// Note that CheckBlock can only fail for one of a few reasons:
5901+
// 1. bad-proof-of-work (impossible here, because we've already
5902+
// accepted the header)
5903+
// 2. merkleroot doesn't match the transactions given (already
5904+
// caught in FillBlock with READ_STATUS_FAILED, so
5905+
// impossible here)
5906+
// 3. the block is otherwise invalid (eg invalid coinbase,
5907+
// block is too big, too many legacy sigops, etc).
5908+
// So if CheckBlock failed, #3 is the only possibility.
5909+
// Under BIP 152, we don't DoS-ban unless proof of work is
5910+
// invalid (we don't require all the stateless checks to have
5911+
// been run). This is handled below, so just treat this as
5912+
// though the block was successfully read, and rely on the
5913+
// handling in ProcessNewBlock to ensure the block index is
5914+
// updated, reject messages go out, etc.
58965915
MarkBlockAsReceived(resp.blockhash); // it is now an empty pointer
58975916
fBlockRead = true;
58985917
}
@@ -5901,16 +5920,15 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
59015920
CValidationState state;
59025921
// Since we requested this block (it was in mapBlocksInFlight), force it to be processed,
59035922
// even if it would not be a candidate for new tip (missing previous block, chain not long enough, etc)
5904-
ProcessNewBlock(state, chainparams, pfrom, &block, true, NULL);
5923+
// BIP 152 permits peers to relay compact blocks after validating
5924+
// the header only; we should not punish peers if the block turns
5925+
// out to be invalid.
5926+
ProcessNewBlock(state, chainparams, pfrom, &block, true, NULL, false);
59055927
int nDoS;
59065928
if (state.IsInvalid(nDoS)) {
59075929
assert (state.GetRejectCode() < REJECT_INTERNAL); // Blocks are never rejected with internal reject codes
59085930
connman.PushMessage(pfrom, NetMsgType::REJECT, strCommand, (unsigned char)state.GetRejectCode(),
59095931
state.GetRejectReason().substr(0, MAX_REJECT_MESSAGE_LENGTH), block.GetHash());
5910-
if (nDoS > 0) {
5911-
LOCK(cs_main);
5912-
Misbehaving(pfrom->GetId(), nDoS);
5913-
}
59145932
}
59155933
}
59165934
}
@@ -6081,7 +6099,7 @@ bool static ProcessMessage(CNode* pfrom, string strCommand, CDataStream& vRecv,
60816099
// need it even though it is not a candidate for a new best tip.
60826100
forceProcessing |= MarkBlockAsReceived(block.GetHash());
60836101
}
6084-
ProcessNewBlock(state, chainparams, pfrom, &block, forceProcessing, NULL);
6102+
ProcessNewBlock(state, chainparams, pfrom, &block, forceProcessing, NULL, true);
60856103
int nDoS;
60866104
if (state.IsInvalid(nDoS)) {
60876105
assert (state.GetRejectCode() < REJECT_INTERNAL); // Blocks are never rejected with internal reject codes

‎src/main.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ static const uint64_t MIN_DISK_SPACE_FOR_BLOCK_FILES = 550 * 1024 * 1024;
223223
* @param[out] dbp The already known disk position of pblock, or NULL if not yet stored.
224224
* @return True if state.IsValid()
225225
*/
226-
bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp);
226+
bool ProcessNewBlock(CValidationState& state, const CChainParams& chainparams, CNode* pfrom, const CBlock* pblock, bool fForceProcessing, const CDiskBlockPos* dbp, bool fMayBanPeerIfInvalid);
227227
/** Check whether enough disk space is available for an incoming block */
228228
bool CheckDiskSpace(uint64_t nAdditionalBytes = 0);
229229
/** Open a block file (blk?????.dat) */

‎src/rpc/mining.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -132,7 +132,7 @@ UniValue generateBlocks(boost::shared_ptr<CReserveScript> coinbaseScript, int nG
132132
continue;
133133
}
134134
CValidationState state;
135-
if (!ProcessNewBlock(state, Params(), NULL, pblock, true, NULL))
135+
if (!ProcessNewBlock(state, Params(), NULL, pblock, true, NULL, false))
136136
throw JSONRPCError(RPC_INTERNAL_ERROR, "ProcessNewBlock, block not accepted");
137137
++nHeight;
138138
blockHashes.push_back(pblock->GetHash().GetHex());
@@ -757,7 +757,7 @@ UniValue submitblock(const JSONRPCRequest& request)
757757
CValidationState state;
758758
submitblock_StateCatcher sc(block.GetHash());
759759
RegisterValidationInterface(&sc);
760-
bool fAccepted = ProcessNewBlock(state, Params(), NULL, &block, true, NULL);
760+
bool fAccepted = ProcessNewBlock(state, Params(), NULL, &block, true, NULL, false);
761761
UnregisterValidationInterface(&sc);
762762
if (fBlockPresent)
763763
{

‎src/test/miner_tests.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -223,7 +223,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity)
223223
pblock->hashMerkleRoot = BlockMerkleRoot(*pblock);
224224
pblock->nNonce = blockinfo[i].nonce;
225225
CValidationState state;
226-
BOOST_CHECK(ProcessNewBlock(state, chainparams, NULL, pblock, true, NULL));
226+
BOOST_CHECK(ProcessNewBlock(state, chainparams, NULL, pblock, true, NULL, false));
227227
BOOST_CHECK(state.IsValid());
228228
pblock->hashPrevBlock = pblock->GetHash();
229229
}

‎src/test/test_bitcoin.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -127,7 +127,7 @@ TestChain100Setup::CreateAndProcessBlock(const std::vector<CMutableTransaction>&
127127
while (!CheckProofOfWork(block.GetHash(), block.nBits, chainparams.GetConsensus())) ++block.nNonce;
128128

129129
CValidationState state;
130-
ProcessNewBlock(state, chainparams, NULL, &block, true, NULL);
130+
ProcessNewBlock(state, chainparams, NULL, &block, true, NULL, false);
131131

132132
CBlock result = block;
133133
return result;

‎src/version.h‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@
99
* network protocol versioning
1010
*/
1111

12-
static const int PROTOCOL_VERSION = 70014;
12+
static const int PROTOCOL_VERSION = 70015;
1313

1414
//! initial proto version, to be increased after version/verack negotiation
1515
static const int INIT_PROTO_VERSION = 209;
@@ -42,4 +42,7 @@ static const int FEEFILTER_VERSION = 70013;
4242
//! short-id-based block download starts with this version
4343
static const int SHORT_IDS_BLOCKS_VERSION = 70014;
4444

45+
//! not banning for invalid compact blocks starts with this version
46+
static const int INVALID_CB_NO_BAN_VERSION = 70015;
47+
4548
#endif // BITCOIN_VERSION_H

0 commit comments

Comments
 (0)