Skip to content

Commit

Permalink
[p2p] only attempt 1p1c when both txns provided by the same peer
Browse files Browse the repository at this point in the history 
Now that we track all announcers of an orphan, it's not helpful to
consider an orphan provided by a peer that didn't send us this parent.
It can only hurt our chances of finding the right orphan when there are
multiple candidates.

Adapt the 2 tests in p2p_opportunistic_1p1c.py that looked at 1p1c
packages from different peers. Instead of checking that the right peer
is punished, we now check that the package is not submitted. We can't
use the functional test to see that the package was not considered
because the behavior is indistinguishable (except for the logs).
  • Loading branch information
@glozow
glozow committed Dec 6, 2024
1 parent 7412402 commit f96df3d
Show file tree
Hide file tree
Showing 6 changed files with 14 additions and 101 deletions.
34 changes: 4 additions & 30 deletions src/node/txdownloadman_impl.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -294,9 +294,11 @@ std::optional<PackageToValidate> TxDownloadManagerImpl::Find1P1CPackage(const CT

Assume(RecentRejectsReconsiderableFilter().contains(parent_wtxid.ToUint256()));

// Prefer children from this peer. This helps prevent censorship attempts in which an attacker
// Only consider children from this peer. This helps prevent censorship attempts in which an attacker
// sends lots of fake children for the parent, and we (unluckily) keep selecting the fake
// children instead of the real one provided by the honest peer.
// children instead of the real one provided by the honest peer. Since we track all announcers
// of an orphan, this does not exclude parent + orphan pairs that we happened to request from
// different peers.
const auto cpfp_candidates_same_peer{m_orphanage.GetChildrenFromSamePeer(ptx, nodeid)};

// These children should be sorted from newest to oldest. In the (probably uncommon) case
Expand All @@ -309,34 +311,6 @@ std::optional<PackageToValidate> TxDownloadManagerImpl::Find1P1CPackage(const CT
return PackageToValidate{ptx, child, nodeid, nodeid};
}
}

// If no suitable candidate from the same peer is found, also try children that were provided by
// a different peer. This is useful because sometimes multiple peers announce both transactions
// to us, and we happen to download them from different peers (we wouldn't have known that these
// 2 transactions are related). We still want to find 1p1c packages then.
//
// If we start tracking all announcers of orphans, we can restrict this logic to parent + child
// pairs in which both were provided by the same peer, i.e. delete this step.
const auto cpfp_candidates_different_peer{m_orphanage.GetChildrenFromDifferentPeer(ptx, nodeid)};

// Find the first 1p1c that hasn't already been rejected. We randomize the order to not
// create a bias that attackers can use to delay package acceptance.
//
// Create a random permutation of the indices.
std::vector<size_t> tx_indices(cpfp_candidates_different_peer.size());
std::iota(tx_indices.begin(), tx_indices.end(), 0);
std::shuffle(tx_indices.begin(), tx_indices.end(), m_opts.m_rng);

for (const auto index : tx_indices) {
// If we already tried a package and failed for any reason, the combined hash was
// cached in m_lazy_recent_rejects_reconsiderable.
const auto [child_tx, child_sender] = cpfp_candidates_different_peer.at(index);
Package maybe_cpfp_package{ptx, child_tx};
if (!RecentRejectsReconsiderableFilter().contains(GetPackageHash(maybe_cpfp_package)) &&
!RecentRejectsFilter().contains(child_tx->GetHash().ToUint256())) {
return PackageToValidate{ptx, child_tx, nodeid, child_sender};
}
}
return std::nullopt;
}

Expand Down
6 changes: 0 additions & 6 deletions src/test/fuzz/txorphan.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,6 @@ FUZZ_TARGET(txorphan, .init = initialize_orphanage)
return input.prevout.hash == ptx_potential_parent->GetHash();
}));
}
for (const auto& [child, peer] : orphanage.GetChildrenFromDifferentPeer(ptx_potential_parent, peer_id)) {
assert(std::any_of(child->vin.cbegin(), child->vin.cend(), [&](const auto& input) {
return input.prevout.hash == ptx_potential_parent->GetHash();
}));
assert(peer != peer_id);
}
}

// trigger orphanage functions
Expand Down
18 changes: 0 additions & 18 deletions src/test/orphanage_tests.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,15 +93,6 @@ static bool EqualTxns(const std::set<CTransactionRef>& set_txns, const std::vect
}
return true;
}
static bool EqualTxns(const std::set<CTransactionRef>& set_txns,
const std::vector<std::pair<CTransactionRef, NodeId>>& vec_txns)
{
if (vec_txns.size() != set_txns.size()) return false;
for (const auto& [tx, nodeid] : vec_txns) {
if (!set_txns.contains(tx)) return false;
}
return true;
}

BOOST_AUTO_TEST_CASE(DoS_mapOrphans)
{
Expand Down Expand Up @@ -310,18 +301,13 @@ BOOST_AUTO_TEST_CASE(get_children)
BOOST_CHECK(EqualTxns(expected_parent1_children, orphanage.GetChildrenFromSamePeer(parent1, node1)));
BOOST_CHECK(EqualTxns(expected_parent2_children, orphanage.GetChildrenFromSamePeer(parent2, node1)));

BOOST_CHECK(EqualTxns(expected_parent1_children, orphanage.GetChildrenFromDifferentPeer(parent1, node2)));
BOOST_CHECK(EqualTxns(expected_parent2_children, orphanage.GetChildrenFromDifferentPeer(parent2, node2)));

// The peer must match
BOOST_CHECK(orphanage.GetChildrenFromSamePeer(parent1, node2).empty());
BOOST_CHECK(orphanage.GetChildrenFromSamePeer(parent2, node2).empty());

// There shouldn't be any children of this tx in the orphanage
BOOST_CHECK(orphanage.GetChildrenFromSamePeer(child_p1n0_p2n0, node1).empty());
BOOST_CHECK(orphanage.GetChildrenFromSamePeer(child_p1n0_p2n0, node2).empty());
BOOST_CHECK(orphanage.GetChildrenFromDifferentPeer(child_p1n0_p2n0, node1).empty());
BOOST_CHECK(orphanage.GetChildrenFromDifferentPeer(child_p1n0_p2n0, node2).empty());
}

// Orphans provided by node1 and node2
Expand All @@ -344,31 +330,27 @@ BOOST_AUTO_TEST_CASE(get_children)
std::set<CTransactionRef> expected_parent1_node1{child_p1n0};

BOOST_CHECK(EqualTxns(expected_parent1_node1, orphanage.GetChildrenFromSamePeer(parent1, node1)));
BOOST_CHECK(EqualTxns(expected_parent1_node1, orphanage.GetChildrenFromDifferentPeer(parent1, node2)));
}

// Children of parent2 from node1:
{
std::set<CTransactionRef> expected_parent2_node1{child_p2n1};

BOOST_CHECK(EqualTxns(expected_parent2_node1, orphanage.GetChildrenFromSamePeer(parent2, node1)));
BOOST_CHECK(EqualTxns(expected_parent2_node1, orphanage.GetChildrenFromDifferentPeer(parent2, node2)));
}

// Children of parent1 from node2:
{
std::set<CTransactionRef> expected_parent1_node2{child_p1n0_p1n1, child_p1n0_p2n0};

BOOST_CHECK(EqualTxns(expected_parent1_node2, orphanage.GetChildrenFromSamePeer(parent1, node2)));
BOOST_CHECK(EqualTxns(expected_parent1_node2, orphanage.GetChildrenFromDifferentPeer(parent1, node1)));
}

// Children of parent2 from node2:
{
std::set<CTransactionRef> expected_parent2_node2{child_p1n0_p2n0};

BOOST_CHECK(EqualTxns(expected_parent2_node2, orphanage.GetChildrenFromSamePeer(parent2, node2)));
BOOST_CHECK(EqualTxns(expected_parent2_node2, orphanage.GetChildrenFromDifferentPeer(parent2, node1)));
}
}
}
Expand Down
32 changes: 0 additions & 32 deletions src/txorphanage.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -280,38 +280,6 @@ std::vector<CTransactionRef> TxOrphanage::GetChildrenFromSamePeer(const CTransac
return children_found;
}

std::vector<std::pair<CTransactionRef, NodeId>> TxOrphanage::GetChildrenFromDifferentPeer(const CTransactionRef& parent, NodeId nodeid) const
{
// First construct vector of iterators to ensure we do not return duplicates of the same tx.
std::vector<OrphanMap::iterator> iters;

// For each output, get all entries spending this prevout, filtering for ones not from the specified peer.
for (unsigned int i = 0; i < parent->vout.size(); i++) {
const auto it_by_prev = m_outpoint_to_orphan_it.find(COutPoint(parent->GetHash(), i));
if (it_by_prev != m_outpoint_to_orphan_it.end()) {
for (const auto& elem : it_by_prev->second) {
if (!elem->second.announcers.contains(nodeid)) {
iters.emplace_back(elem);
}
}
}
}

// Erase duplicates
std::sort(iters.begin(), iters.end(), IteratorComparator());
iters.erase(std::unique(iters.begin(), iters.end()), iters.end());

// Convert iterators to pair<CTransactionRef, NodeId>
std::vector<std::pair<CTransactionRef, NodeId>> children_found;
children_found.reserve(iters.size());
for (const auto& child_iter : iters) {
// Use first peer in announcers list
auto peer = *(child_iter->second.announcers.begin());
children_found.emplace_back(child_iter->second.tx, peer);
}
return children_found;
}

std::vector<TxOrphanage::OrphanTxBase> TxOrphanage::GetOrphanTransactions() const
{
std::vector<OrphanTxBase> ret;
Expand Down
4 changes: 0 additions & 4 deletions src/txorphanage.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -71,10 +71,6 @@ class TxOrphanage {
* recent to least recent. */
std::vector<CTransactionRef> GetChildrenFromSamePeer(const CTransactionRef& parent, NodeId nodeid) const;

/** Get all children that spend from this tx but were not received from nodeid. Also return
* which peer provided each tx. */
std::vector<std::pair<CTransactionRef, NodeId>> GetChildrenFromDifferentPeer(const CTransactionRef& parent, NodeId nodeid) const;

/** Get an orphan's parent_txids, or std::nullopt if the orphan is not present. */
std::optional<std::vector<Txid>> GetParentTxids(const Wtxid& wtxid);

Expand Down
21 changes: 10 additions & 11 deletions test/functional/p2p_opportunistic_1p1c.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@ def test_low_and_high_child(self, wallet):

@cleanup
def test_orphan_consensus_failure(self):
self.log.info("Check opportunistic 1p1c logic with consensus-invalid orphan causes disconnect of the correct peer")
self.log.info("Check opportunistic 1p1c logic requires parent and child to be from the same peer")
node = self.nodes[0]
low_fee_parent = self.create_tx_below_mempoolminfee(self.wallet)
coin = low_fee_parent["new_utxo"]
Expand All @@ -239,15 +239,17 @@ def test_orphan_consensus_failure(self):
parent_txid_int = int(low_fee_parent["txid"], 16)
bad_orphan_sender.wait_for_getdata([parent_txid_int])

# 3. A different peer relays the parent. Parent+Child are evaluated as a package and rejected.
parent_sender.send_message(msg_tx(low_fee_parent["tx"]))
# 3. A different peer relays the parent. Package is not evaluated because the transactions
# were not sent from the same peer.
parent_sender.send_and_ping(msg_tx(low_fee_parent["tx"]))

# 4. Transactions should not be in mempool.
node_mempool = node.getrawmempool()
assert low_fee_parent["txid"] not in node_mempool
assert tx_orphan_bad_wit.rehash() not in node_mempool

# 5. Peer that sent a consensus-invalid transaction should be disconnected.
# 5. Have the other peer send the tx too, so that tx_orphan_bad_wit package is attempted.
bad_orphan_sender.send_message(msg_tx(low_fee_parent["tx"]))
bad_orphan_sender.wait_for_disconnect()

# The peer that didn't provide the orphan should not be disconnected.
Expand Down Expand Up @@ -279,20 +281,17 @@ def test_parent_consensus_failure(self):
package_sender.wait_for_getdata([parent_txid_int])

# 3. A different node relays the parent. The parent is first evaluated by itself and
# rejected for being too low feerate. Then it is evaluated as a package and, after passing
# feerate checks, rejected for having a bad signature (consensus error).
fake_parent_sender.send_message(msg_tx(tx_parent_bad_wit))
# rejected for being too low feerate. It is not evaluated as a package because the child was
# sent from a different peer, so we don't find out that the child is consensus-invalid.
fake_parent_sender.send_and_ping(msg_tx(tx_parent_bad_wit))

# 4. Transactions should not be in mempool.
node_mempool = node.getrawmempool()
assert tx_parent_bad_wit.rehash() not in node_mempool
assert high_fee_child["txid"] not in node_mempool

# 5. Peer sent a consensus-invalid transaction.
fake_parent_sender.wait_for_disconnect()

self.log.info("Check that fake parent does not cause orphan to be deleted and real package can still be submitted")
# 6. Child-sending should not have been punished and the orphan should remain in orphanage.
# 5. Child-sending should not have been punished and the orphan should remain in orphanage.
# It can send the "real" parent transaction, and the package is accepted.
parent_wtxid_int = int(low_fee_parent["tx"].getwtxid(), 16)
package_sender.send_and_ping(msg_inv([CInv(t=MSG_WTX, h=parent_wtxid_int)]))
Expand Down

0 comments on commit f96df3d

Please sign in to comment.