Skip to content

Commit bac9ee4

Browse files
instagibbsinstagibbs
committed
p2p: Add witness mutation check inside FillBlock
Since bitcoin#29412, we have not allowed mutated blocks to continue being processed immediately the block is received, but this is only done for the legacy BLOCK message. Extend these checks as belt-and-suspenders to not allow similar mutation strategies to affect relay by honest peers by applying the check inside PartiallyDownloadedBlock::FillBlock, immediately before returning READ_STATUS_OK. This also removes the extraneous CheckBlock call.
1 parent 4b1d48a commit bac9ee4

File tree

5 files changed

+37
-51
lines changed

5 files changed

+37
-51
lines changed

‎src/blockencodings.cpp‎

Lines changed: 6 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -179,7 +179,7 @@ bool PartiallyDownloadedBlock::IsTxAvailable(size_t index) const
179179
return txn_available[index] != nullptr;
180180
}
181181

182-
ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing)
182+
ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing, bool segwit_active)
183183
{
184184
if (header.IsNull()) return READ_STATUS_INVALID;
185185

@@ -206,16 +206,11 @@ ReadStatus PartiallyDownloadedBlock::FillBlock(CBlock& block, const std::vector<
206206
if (vtx_missing.size() != tx_missing_offset)
207207
return READ_STATUS_INVALID;
208208

209-
BlockValidationState state;
210-
CheckBlockFn check_block = m_check_block_mock ? m_check_block_mock : CheckBlock;
211-
if (!check_block(block, state, Params().GetConsensus(), /*fCheckPoW=*/true, /*fCheckMerkleRoot=*/true)) {
212-
// TODO: We really want to just check merkle tree manually here,
213-
// but that is expensive, and CheckBlock caches a block's
214-
// "checked-status" (in the CBlock?). CBlock should be able to
215-
// check its own merkle root and cache that check.
216-
if (state.GetResult() == BlockValidationResult::BLOCK_MUTATED)
217-
return READ_STATUS_FAILED; // Possible Short ID collision
218-
return READ_STATUS_CHECKBLOCK_FAILED;
209+
// Check for possible mutations early now that we have a seemingly good block
210+
IsBlockMutatedFn check_mutated{m_check_block_mutated_mock ? m_check_block_mutated_mock : IsBlockMutated};
211+
if (check_mutated(/*block=*/block,
212+
/*check_witness_root=*/segwit_active)) {
213+
return READ_STATUS_FAILED; // Possible Short ID collision
219214
}
220215

221216
LogDebug(BCLog::CMPCTBLOCK, "Successfully reconstructed block %s with %u txn prefilled, %u txn from mempool (incl at least %u from extra pool) and %u txn (%u bytes) requested\n", hash.ToString(), prefilled_count, mempool_count, extra_count, vtx_missing.size(), tx_missing_size);

‎src/blockencodings.h‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -141,15 +141,16 @@ class PartiallyDownloadedBlock {
141141
CBlockHeader header;
142142

143143
// Can be overridden for testing
144-
using CheckBlockFn = std::function<bool(const CBlock&, BlockValidationState&, const Consensus::Params&, bool, bool)>;
145-
CheckBlockFn m_check_block_mock{nullptr};
144+
using IsBlockMutatedFn = std::function<bool(const CBlock&, bool)>;
145+
IsBlockMutatedFn m_check_block_mutated_mock{nullptr};
146146

147147
explicit PartiallyDownloadedBlock(CTxMemPool* poolIn) : pool(poolIn) {}
148148

149149
// extra_txn is a list of extra orphan/conflicted/etc transactions to look at
150150
ReadStatus InitData(const CBlockHeaderAndShortTxIDs& cmpctblock, const std::vector<CTransactionRef>& extra_txn);
151151
bool IsTxAvailable(size_t index) const;
152-
ReadStatus FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing);
152+
// segwit_active enforces witness mutation checks just before reporting a healthy status
153+
ReadStatus FillBlock(CBlock& block, const std::vector<CTransactionRef>& vtx_missing, bool segwit_active);
153154
};
154155

155156
#endif // BITCOIN_BLOCKENCODINGS_H

‎src/net_processing.cpp‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3360,7 +3360,11 @@ void PeerManagerImpl::ProcessCompactBlockTxns(CNode& pfrom, Peer& peer, const Bl
33603360
}
33613361

33623362
PartiallyDownloadedBlock& partialBlock = *range_flight.first->second.second->partialBlock;
3363-
ReadStatus status = partialBlock.FillBlock(*pblock, block_transactions.txn);
3363+
3364+
// We should not have gotten this far in compact block processing unless it's attached to a known header
3365+
const CBlockIndex* prev_block{Assume(m_chainman.m_blockman.LookupBlockIndex(partialBlock.header.hashPrevBlock))};
3366+
ReadStatus status = partialBlock.FillBlock(*pblock, block_transactions.txn,
3367+
/*segwit_active=*/DeploymentActiveAfter(prev_block, m_chainman, Consensus::DEPLOYMENT_SEGWIT));
33643368
if (status == READ_STATUS_INVALID) {
33653369
RemoveBlockRequest(block_transactions.blockhash, pfrom.GetId()); // Reset in-flight state in case Misbehaving does not result in a disconnect
33663370
Misbehaving(peer, "invalid compact block/non-matching block transactions");
@@ -4529,7 +4533,9 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
45294533
return;
45304534
}
45314535
std::vector<CTransactionRef> dummy;
4532-
status = tempBlock.FillBlock(*pblock, dummy);
4536+
const CBlockIndex* prev_block{Assume(m_chainman.m_blockman.LookupBlockIndex(cmpctblock.header.hashPrevBlock))};
4537+
status = tempBlock.FillBlock(*pblock, dummy,
4538+
/*segwit_active=*/DeploymentActiveAfter(prev_block, m_chainman, Consensus::DEPLOYMENT_SEGWIT));
45334539
if (status == READ_STATUS_OK) {
45344540
fBlockReconstructed = true;
45354541
}

‎src/test/blockencodings_tests.cpp‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -95,21 +95,21 @@ BOOST_AUTO_TEST_CASE(SimpleRoundTripTest)
9595
CBlock block2;
9696
{
9797
PartiallyDownloadedBlock tmp = partialBlock;
98-
BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_INVALID); // No transactions
98+
BOOST_CHECK(partialBlock.FillBlock(block2, {}, /*segwit_active=*/true) == READ_STATUS_INVALID); // No transactions
9999
partialBlock = tmp;
100100
}
101101

102102
// Wrong transaction
103103
{
104104
PartiallyDownloadedBlock tmp = partialBlock;
105-
partialBlock.FillBlock(block2, {block.vtx[2]}); // Current implementation doesn't check txn here, but don't require that
105+
partialBlock.FillBlock(block2, {block.vtx[2]}, /*segwit_active=*/true); // Current implementation doesn't check txn here, but don't require that
106106
partialBlock = tmp;
107107
}
108108
bool mutated;
109109
BOOST_CHECK(block.hashMerkleRoot != BlockMerkleRoot(block2, &mutated));
110110

111111
CBlock block3;
112-
BOOST_CHECK(partialBlock.FillBlock(block3, {block.vtx[1]}) == READ_STATUS_OK);
112+
BOOST_CHECK(partialBlock.FillBlock(block3, {block.vtx[1]}, /*segwit_active=*/true) == READ_STATUS_OK);
113113
BOOST_CHECK_EQUAL(block.GetHash().ToString(), block3.GetHash().ToString());
114114
BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block3, &mutated).ToString());
115115
BOOST_CHECK(!mutated);
@@ -182,14 +182,14 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest)
182182
CBlock block2;
183183
{
184184
PartiallyDownloadedBlock tmp = partialBlock;
185-
BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_INVALID); // No transactions
185+
BOOST_CHECK(partialBlock.FillBlock(block2, {}, /*segwit_active=*/true) == READ_STATUS_INVALID); // No transactions
186186
partialBlock = tmp;
187187
}
188188

189189
// Wrong transaction
190190
{
191191
PartiallyDownloadedBlock tmp = partialBlock;
192-
partialBlock.FillBlock(block2, {block.vtx[1]}); // Current implementation doesn't check txn here, but don't require that
192+
partialBlock.FillBlock(block2, {block.vtx[1]}, /*segwit_active=*/true); // Current implementation doesn't check txn here, but don't require that
193193
partialBlock = tmp;
194194
}
195195
BOOST_CHECK_EQUAL(pool.get(block.vtx[2]->GetHash()).use_count(), SHARED_TX_OFFSET + 2); // +2 because of partialBlock and block2
@@ -198,7 +198,7 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest)
198198

199199
CBlock block3;
200200
PartiallyDownloadedBlock partialBlockCopy = partialBlock;
201-
BOOST_CHECK(partialBlock.FillBlock(block3, {block.vtx[0]}) == READ_STATUS_OK);
201+
BOOST_CHECK(partialBlock.FillBlock(block3, {block.vtx[0]}, /*segwit_active=*/true) == READ_STATUS_OK);
202202
BOOST_CHECK_EQUAL(block.GetHash().ToString(), block3.GetHash().ToString());
203203
BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block3, &mutated).ToString());
204204
BOOST_CHECK(!mutated);
@@ -252,7 +252,7 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest)
252252

253253
CBlock block2;
254254
PartiallyDownloadedBlock partialBlockCopy = partialBlock;
255-
BOOST_CHECK(partialBlock.FillBlock(block2, {}) == READ_STATUS_OK);
255+
BOOST_CHECK(partialBlock.FillBlock(block2, {}, /*segwit_active=*/true) == READ_STATUS_OK);
256256
BOOST_CHECK_EQUAL(block.GetHash().ToString(), block2.GetHash().ToString());
257257
bool mutated;
258258
BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block2, &mutated).ToString());
@@ -300,7 +300,7 @@ BOOST_AUTO_TEST_CASE(EmptyBlockRoundTripTest)
300300

301301
CBlock block2;
302302
std::vector<CTransactionRef> vtx_missing;
303-
BOOST_CHECK(partialBlock.FillBlock(block2, vtx_missing) == READ_STATUS_OK);
303+
BOOST_CHECK(partialBlock.FillBlock(block2, vtx_missing, /*segwit_active=*/true) == READ_STATUS_OK);
304304
BOOST_CHECK_EQUAL(block.GetHash().ToString(), block2.GetHash().ToString());
305305
BOOST_CHECK_EQUAL(block.hashMerkleRoot.ToString(), BlockMerkleRoot(block2, &mutated).ToString());
306306
BOOST_CHECK(!mutated);

‎src/test/fuzz/partially_downloaded_block.cpp‎

Lines changed: 11 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -36,14 +36,10 @@ void initialize_pdb()
3636
g_setup = testing_setup.get();
3737
}
3838

39-
PartiallyDownloadedBlock::CheckBlockFn FuzzedCheckBlock(std::optional<BlockValidationResult> result)
39+
PartiallyDownloadedBlock::IsBlockMutatedFn FuzzedIsBlockMutated(bool result)
4040
{
41-
return [result](const CBlock&, BlockValidationState& state, const Consensus::Params&, bool, bool) {
42-
if (result) {
43-
return state.Invalid(*result);
44-
}
45-
46-
return true;
41+
return [result](const CBlock& block, bool) {
42+
return result;
4743
};
4844
}
4945

@@ -115,35 +111,23 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb)
115111
skipped_missing |= (!pdb.IsTxAvailable(i) && skip);
116112
}
117113

118-
// Mock CheckBlock
119-
bool fail_check_block{fuzzed_data_provider.ConsumeBool()};
120-
auto validation_result =
121-
fuzzed_data_provider.PickValueInArray(
122-
{BlockValidationResult::BLOCK_RESULT_UNSET,
123-
BlockValidationResult::BLOCK_CONSENSUS,
124-
BlockValidationResult::BLOCK_CACHED_INVALID,
125-
BlockValidationResult::BLOCK_INVALID_HEADER,
126-
BlockValidationResult::BLOCK_MUTATED,
127-
BlockValidationResult::BLOCK_MISSING_PREV,
128-
BlockValidationResult::BLOCK_INVALID_PREV,
129-
BlockValidationResult::BLOCK_TIME_FUTURE,
130-
BlockValidationResult::BLOCK_HEADER_LOW_WORK});
131-
pdb.m_check_block_mock = FuzzedCheckBlock(
132-
fail_check_block ?
133-
std::optional<BlockValidationResult>{validation_result} :
134-
std::nullopt);
114+
bool segwit_active{fuzzed_data_provider.ConsumeBool()};
115+
116+
// Mock IsBlockMutated
117+
bool fail_block_mutated{fuzzed_data_provider.ConsumeBool()};
118+
pdb.m_check_block_mutated_mock = FuzzedIsBlockMutated(fail_block_mutated);
135119

136120
CBlock reconstructed_block;
137-
auto fill_status{pdb.FillBlock(reconstructed_block, missing)};
121+
auto fill_status{pdb.FillBlock(reconstructed_block, missing, segwit_active)};
138122
switch (fill_status) {
139123
case READ_STATUS_OK:
140124
assert(!skipped_missing);
141-
assert(!fail_check_block);
125+
assert(!fail_block_mutated);
142126
assert(block->GetHash() == reconstructed_block.GetHash());
143127
break;
144128
case READ_STATUS_CHECKBLOCK_FAILED: [[fallthrough]];
145129
case READ_STATUS_FAILED:
146-
assert(fail_check_block);
130+
assert(fail_block_mutated);
147131
break;
148132
case READ_STATUS_INVALID:
149133
break;

0 commit comments

Comments
 (0)