phantom/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-04-29 14:59:39 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

tx download: add wtxid to reject filter even if witness was stripped

Browse source 
We previously didn't for witness-stripped transactions due to compatibility concerns with
non-wtxid-relay peers.

The issue we were trying to prevent is attacker node A stripping the witness of a transaction
broadcast by honest node B and submitting it stripped to a large part of the network. It would
result in those nodes including the txid of the transaction (== witness-stripped wtxid) to their
recent reject filter, which would significantly hinder the propagation of the honest non-stripped
transaction if most connections on the network are txid based relay.

Support for wtxid based relay was released with 0.21 in January 2021 and is now ubiquitous on the
network. Therefore this concern does not apply anymore and we can get rid of this special case.
This commit is contained in:
Antoine Poinsot 2025-04-28 15:37:51 -04:00
parent 3a29ba33dc
commit 07018a44f8
4 changed files with 6 additions and 24 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

16
src/node/txdownloadman_impl.cpp
View file

@ -446,22 +446,10 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction
m_txrequest.ForgetTxHash(tx.GetWitnessHash());
}
}
} else if (state.GetResult() == TxValidationResult::TX_WITNESS_STRIPPED) {
add_extra_compact_tx = false;
} else {
// We can add the wtxid of this transaction to our reject filter.
// Do not add txids of witness transactions or witness-stripped
// transactions to the filter, as they can have been malleated;
// adding such txids to the reject filter would potentially
// interfere with relay of valid transactions from peers that
// do not support wtxid-based relay. See
// https://github.com/bitcoin/bitcoin/issues/8279 for details.
// We can remove this restriction (and always add wtxids to
// the filter even for witness stripped transactions) once
// wtxid-based relay is broadly deployed.
// See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034
// for concerns around weakening security of unupgraded nodes
// if we start doing this too early.
// Do not add txids of witness transactions as those may have been malleated. See
// https://github.com/bitcoin/bitcoin/issues/8279 for a discussion on this topic.
if (state.GetResult() == TxValidationResult::TX_RECONSIDERABLE) {
// If the result is TX_RECONSIDERABLE, add it to m_lazy_recent_rejects_reconsiderable
// because we should not download or submit this transaction by itself again, but may

1
src/test/fuzz/txdownloadman.cpp
View file

@ -38,7 +38,6 @@ static TxValidationResult TESTED_TX_RESULTS[] = {
TxValidationResult::TX_MISSING_INPUTS,
TxValidationResult::TX_PREMATURE_SPEND,
TxValidationResult::TX_WITNESS_MUTATED,
TxValidationResult::TX_WITNESS_STRIPPED,
TxValidationResult::TX_CONFLICT,
TxValidationResult::TX_MEMPOOL_POLICY,
// Skip TX_NO_MEMPOOL

1
src/test/txdownload_tests.cpp
View file

@ -63,7 +63,6 @@ static std::map<TxValidationResult, Behaviors> expected_behaviors{
{TxValidationResult::TX_MISSING_INPUTS, { 0, 0, 0, 0, 1, 0, 1}},
{TxValidationResult::TX_PREMATURE_SPEND, { 0, 1, 0, 0, 1, 0, 1}},
{TxValidationResult::TX_WITNESS_MUTATED, { 0, 1, 0, 0, 1, 0, 1}},
{TxValidationResult::TX_WITNESS_STRIPPED, { 0, 0, 0, 0, 0, 0, 0}},
{TxValidationResult::TX_CONFLICT, { 0, 1, 0, 0, 1, 0, 1}},
{TxValidationResult::TX_MEMPOOL_POLICY, { 0, 1, 0, 0, 1, 0, 1}},
{TxValidationResult::TX_NO_MEMPOOL, { 0, 1, 0, 0, 1, 0, 1}},

12
test/functional/p2p_orphan_handling.py
View file

@ -257,16 +257,12 @@ class OrphanHandlingTest(BitcoinTestFramework):
# Relay the child. It should not be accepted because it has missing inputs.
self.relay_transaction(peer2, child_invalid_witness["tx"])
assert child_invalid_witness["txid"] not in node.getrawmempool()
assert tx_in_orphanage(node, child_invalid_witness["tx"])
# The parent should be requested since the unstripped wtxid would differ. Delayed because
# it's by txid and this is not a preferred relay peer.
self.nodes[0].bumpmocktime(NONPREF_PEER_TX_DELAY + TXID_RELAY_DELAY)
peer2.wait_for_getdata([int(parent_normal["tx"].rehash(), 16)])
# parent_normal can be relayed again even though parent1_witness_stripped was rejected
# The parent will not be requested because its txid was added to the reject filter above (as
# the wtxid of its witness-stripped version). But it can still be relayed again if it's not
# stripped.
self.relay_transaction(peer1, parent_normal["tx"])
assert_equal(set(node.getrawmempool()), set([parent_normal["txid"], child_invalid_witness["txid"]]))
assert_equal(set(node.getrawmempool()), set([parent_normal["txid"]]))
@cleanup
def test_orphan_multiple_parents(self):