Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/Makefile.am
Original file line number Diff line number Diff line change
Expand Up @@ -224,8 +224,9 @@ BITCOIN_CORE_H = \
timedata.h \
torcontrol.h \
txdb.h \
txrequest.h \
txmempool.h \
txorphanage.h \
txrequest.h \
undo.h \
util/asmap.h \
util/bip32.h \
Expand Down Expand Up @@ -347,8 +348,9 @@ libbitcoin_server_a_SOURCES = \
timedata.cpp \
torcontrol.cpp \
txdb.cpp \
txrequest.cpp \
txmempool.cpp \
txorphanage.cpp \
txrequest.cpp \
validation.cpp \
validationinterface.cpp \
versionbits.cpp \
Expand Down
1 change: 1 addition & 0 deletions src/init.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@
#include <torcontrol.h>
#include <txdb.h>
#include <txmempool.h>
#include <txorphanage.h>
#include <util/asmap.h>
#include <util/check.h>
#include <util/moneystr.h>
Expand Down
284 changes: 33 additions & 251 deletions src/net_processing.cpp

Large diffs are not rendered by default.

1 change: 0 additions & 1 deletion src/net_processing.h
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ class CTxMemPool;
class ChainstateManager;

extern RecursiveMutex cs_main;
extern RecursiveMutex g_cs_orphans;

/** Default for -maxorphantx, maximum number of orphan transactions kept in memory */
static const unsigned int DEFAULT_MAX_ORPHAN_TRANSACTIONS = 100;
Expand Down
69 changes: 34 additions & 35 deletions src/test/denialofservice_tests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
#include <script/signingprovider.h>
#include <script/standard.h>
#include <serialize.h>
#include <txorphanage.h>
#include <util/memory.h>
#include <util/string.h>
#include <util/system.h>
Expand Down Expand Up @@ -43,18 +44,6 @@ struct CConnmanTest : public CConnman {
}
};

// Tests these internal-to-net_processing.cpp methods:
extern bool AddOrphanTx(const CTransactionRef& tx, NodeId peer);
extern void EraseOrphansFor(NodeId peer);
extern unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans);

struct COrphanTx {
CTransactionRef tx;
NodeId fromPeer;
int64_t nTimeExpire;
};
extern std::map<uint256, COrphanTx> mapOrphanTransactions GUARDED_BY(g_cs_orphans);

static CService ip(uint32_t i)
{
struct in_addr s;
Expand Down Expand Up @@ -295,15 +284,23 @@ BOOST_AUTO_TEST_CASE(DoS_bantime)
peerLogic->FinalizeNode(dummyNode, dummy);
}

static CTransactionRef RandomOrphan()
class TxOrphanageTest : public TxOrphanage
{
std::map<uint256, COrphanTx>::iterator it;
LOCK2(cs_main, g_cs_orphans);
it = mapOrphanTransactions.lower_bound(InsecureRand256());
if (it == mapOrphanTransactions.end())
it = mapOrphanTransactions.begin();
return it->second.tx;
}
public:
inline size_t CountOrphans() const EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans)
{
return m_orphans.size();
}

CTransactionRef RandomOrphan() EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans)
{
std::map<uint256, OrphanTx>::iterator it;
it = m_orphans.lower_bound(InsecureRand256());
if (it == m_orphans.end())
it = m_orphans.begin();
return it->second.tx;
}
};

static void MakeNewKeyWithFastRandomContext(CKey& key)
{
Expand All @@ -323,11 +320,14 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans)
// signature's R and S values have leading zeros.
g_insecure_rand_ctx = FastRandomContext(ArithToUint256(arith_uint256(33)));

TxOrphanageTest orphanage;
CKey key;
MakeNewKeyWithFastRandomContext(key);
FillableSigningProvider keystore;
BOOST_CHECK(keystore.AddKey(key));

LOCK(g_cs_orphans);

// 50 orphan transactions:
for (int i = 0; i < 50; i++)
{
Expand All @@ -340,13 +340,13 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans)
tx.vout[0].nValue = 1*CENT;
tx.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key.GetPubKey()));

AddOrphanTx(MakeTransactionRef(tx), i);
orphanage.AddTx(MakeTransactionRef(tx), i);
}

// ... and 50 that depend on other orphans:
for (int i = 0; i < 50; i++)
{
CTransactionRef txPrev = RandomOrphan();
CTransactionRef txPrev = orphanage.RandomOrphan();

CMutableTransaction tx;
tx.vin.resize(1);
Expand All @@ -357,13 +357,13 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans)
tx.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key.GetPubKey()));
BOOST_CHECK(SignSignature(keystore, *txPrev, tx, 0, SIGHASH_ALL));

AddOrphanTx(MakeTransactionRef(tx), i);
orphanage.AddTx(MakeTransactionRef(tx), i);
}

// This really-big orphan should be ignored:
for (int i = 0; i < 10; i++)
{
CTransactionRef txPrev = RandomOrphan();
CTransactionRef txPrev = orphanage.RandomOrphan();

CMutableTransaction tx;
tx.vout.resize(1);
Expand All @@ -381,25 +381,24 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans)
for (unsigned int j = 1; j < tx.vin.size(); j++)
tx.vin[j].scriptSig = tx.vin[0].scriptSig;

BOOST_CHECK(!AddOrphanTx(MakeTransactionRef(tx), i));
BOOST_CHECK(!orphanage.AddTx(MakeTransactionRef(tx), i));
}

LOCK2(cs_main, g_cs_orphans);
// Test EraseOrphansFor:
for (NodeId i = 0; i < 3; i++)
{
size_t sizeBefore = mapOrphanTransactions.size();
EraseOrphansFor(i);
BOOST_CHECK(mapOrphanTransactions.size() < sizeBefore);
size_t sizeBefore = orphanage.CountOrphans();
orphanage.EraseForPeer(i);
BOOST_CHECK(orphanage.CountOrphans() < sizeBefore);
}

// Test LimitOrphanTxSize() function:
LimitOrphanTxSize(40);
BOOST_CHECK(mapOrphanTransactions.size() <= 40);
LimitOrphanTxSize(10);
BOOST_CHECK(mapOrphanTransactions.size() <= 10);
LimitOrphanTxSize(0);
BOOST_CHECK(mapOrphanTransactions.empty());
orphanage.LimitOrphans(40);
BOOST_CHECK(orphanage.CountOrphans() <= 40);
orphanage.LimitOrphans(10);
BOOST_CHECK(orphanage.CountOrphans() <= 10);
orphanage.LimitOrphans(0);
BOOST_CHECK(orphanage.CountOrphans() == 0);
}

BOOST_AUTO_TEST_SUITE_END()
1 change: 1 addition & 0 deletions src/test/fuzz/process_message.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <test/util/net.h>
#include <test/util/setup_common.h>
#include <test/util/validation.h>
#include <txorphanage.h>
#include <util/memory.h>
#include <validationinterface.h>
#include <version.h>
Expand Down
1 change: 1 addition & 0 deletions src/test/fuzz/process_messages.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
#include <test/util/net.h>
#include <test/util/setup_common.h>
#include <test/util/validation.h>
#include <txorphanage.h>
#include <util/memory.h>
#include <validation.h>
#include <validationinterface.h>
Expand Down
202 changes: 202 additions & 0 deletions src/txorphanage.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,202 @@
// Copyright (c) 2021 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <txorphanage.h>

#include <consensus/validation.h>
#include <logging.h>
#include <policy/policy.h>

#include <cassert>

/** Expiration time for orphan transactions in seconds */
static constexpr int64_t ORPHAN_TX_EXPIRE_TIME = 20 * 60;
/** Minimum time between orphan transactions expire time checks in seconds */
static constexpr int64_t ORPHAN_TX_EXPIRE_INTERVAL = 5 * 60;

RecursiveMutex g_cs_orphans;

bool TxOrphanage::AddTx(const CTransactionRef& tx, NodeId peer)
{
AssertLockHeld(g_cs_orphans);

const uint256& hash = tx->GetHash();
if (m_orphans.count(hash))
return false;

// Ignore big transactions, to avoid a
// send-big-orphans memory exhaustion attack. If a peer has a legitimate
// large transaction with a missing parent then we assume
// it will rebroadcast it later, after the parent transaction(s)
// have been mined or received.
// 100 orphans, each of which is at most 100,000 bytes big is
// at most 10 megabytes of orphans and somewhat more byprev index (in the worst case):
unsigned int sz = GetTransactionWeight(*tx);
if (sz > MAX_STANDARD_TX_WEIGHT)
{
LogPrint(BCLog::MEMPOOL, "ignoring large orphan tx (size: %u, hash: %s)\n", sz, hash.ToString());
return false;
}

auto ret = m_orphans.emplace(hash, OrphanTx{tx, peer, GetTime() + ORPHAN_TX_EXPIRE_TIME, m_orphan_list.size()});
assert(ret.second);
m_orphan_list.push_back(ret.first);
// Allow for lookups in the orphan pool by wtxid, as well as txid
m_wtxid_to_orphan_it.emplace(tx->GetWitnessHash(), ret.first);
for (const CTxIn& txin : tx->vin) {
m_outpoint_to_orphan_it[txin.prevout].insert(ret.first);
}

LogPrint(BCLog::MEMPOOL, "stored orphan tx %s (mapsz %u outsz %u)\n", hash.ToString(),
m_orphans.size(), m_outpoint_to_orphan_it.size());
return true;
}

int TxOrphanage::EraseTx(const uint256& txid)
{
AssertLockHeld(g_cs_orphans);
std::map<uint256, OrphanTx>::iterator it = m_orphans.find(txid);
if (it == m_orphans.end())
return 0;
for (const CTxIn& txin : it->second.tx->vin)
{
auto itPrev = m_outpoint_to_orphan_it.find(txin.prevout);
if (itPrev == m_outpoint_to_orphan_it.end())
continue;
itPrev->second.erase(it);
if (itPrev->second.empty())
m_outpoint_to_orphan_it.erase(itPrev);
}

size_t old_pos = it->second.list_pos;
assert(m_orphan_list[old_pos] == it);
if (old_pos + 1 != m_orphan_list.size()) {
// Unless we're deleting the last entry in m_orphan_list, move the last
// entry to the position we're deleting.
auto it_last = m_orphan_list.back();
m_orphan_list[old_pos] = it_last;
it_last->second.list_pos = old_pos;
}
m_orphan_list.pop_back();
m_wtxid_to_orphan_it.erase(it->second.tx->GetWitnessHash());

m_orphans.erase(it);
return 1;
}

void TxOrphanage::EraseForPeer(NodeId peer)
{
AssertLockHeld(g_cs_orphans);

int nErased = 0;
std::map<uint256, OrphanTx>::iterator iter = m_orphans.begin();
while (iter != m_orphans.end())
{
std::map<uint256, OrphanTx>::iterator maybeErase = iter++; // increment to avoid iterator becoming invalid
if (maybeErase->second.fromPeer == peer)
{
nErased += EraseTx(maybeErase->second.tx->GetHash());
}
}
if (nErased > 0) LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx from peer=%d\n", nErased, peer);
}

unsigned int TxOrphanage::LimitOrphans(unsigned int max_orphans)
{
AssertLockHeld(g_cs_orphans);

unsigned int nEvicted = 0;
static int64_t nNextSweep;
int64_t nNow = GetTime();
if (nNextSweep <= nNow) {
// Sweep out expired orphan pool entries:
int nErased = 0;
int64_t nMinExpTime = nNow + ORPHAN_TX_EXPIRE_TIME - ORPHAN_TX_EXPIRE_INTERVAL;
std::map<uint256, OrphanTx>::iterator iter = m_orphans.begin();
while (iter != m_orphans.end())
{
std::map<uint256, OrphanTx>::iterator maybeErase = iter++;
if (maybeErase->second.nTimeExpire <= nNow) {
nErased += EraseTx(maybeErase->second.tx->GetHash());
} else {
nMinExpTime = std::min(maybeErase->second.nTimeExpire, nMinExpTime);
}
}
// Sweep again 5 minutes after the next entry that expires in order to batch the linear scan.
nNextSweep = nMinExpTime + ORPHAN_TX_EXPIRE_INTERVAL;
if (nErased > 0) LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx due to expiration\n", nErased);
}
FastRandomContext rng;
while (m_orphans.size() > max_orphans)
{
// Evict a random orphan:
size_t randompos = rng.randrange(m_orphan_list.size());
EraseTx(m_orphan_list[randompos]->first);
++nEvicted;
}
return nEvicted;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't the "orphanage overflow" log L3131 in net_processing be moved here instead of returning nEvicted ? And change the message to dissociate clearly expiration-from-eviction.

}

void TxOrphanage::AddChildrenToWorkSet(const CTransaction& tx, std::set<uint256>& orphan_work_set) const
{
AssertLockHeld(g_cs_orphans);
for (unsigned int i = 0; i < tx.vout.size(); i++) {
const auto it_by_prev = m_outpoint_to_orphan_it.find(COutPoint(tx.GetHash(), i));
if (it_by_prev != m_outpoint_to_orphan_it.end()) {
for (const auto& elem : it_by_prev->second) {
orphan_work_set.insert(elem->first);
}
}
}
}

bool TxOrphanage::HaveTx(const GenTxid& gtxid) const
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's say an attacker sends an invalid orphan Z to Alice. Such orphan will be stored
in m_orphans until expiration time, random eviction or parent reception. Another honest
peer Bob announce Z's parent and Z through txid-relay to Alice. Announcement for Z from Bob
won't be scheduled for request by AddTxAnnouncement() as it'll be bounced off before
due to a positive AlreadyHaveTx().

If Bob sends a valid Z's parent, it should be accepted but Alice's version of Z will be
rejected as invalid. Assuming Bob is initial broadcaster of Z and same trick is repeated
against all his tx-relay peers, Z won't propagate until next Bob rebroadcast.

Is the following description correct ? If yes, I don't think that's concerning, assuming Bob
rebroadcast frequency is pretty high (which is the case for time-sensitive Bitcoin
applications).

Though not introduced by this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think what you're saying is: Bob has tx P (parent) and a child of that tx C. Mallory knows both and malleates the witness of C so that it creates a new invalid tx X that has the same txid as C but obviously a different wtxid. Mallory relays X to Alice before Alice hears about P or C. Alice adds X to the orphan pool and requests P from Mallory by wtxid.

I think the expected behaviour is then: Carol announces both P and C to Alice; if wtxid relay is active, both are announced via wtxid so don't conflict with X, and both are requested. If the request for P from Mallory is still active, C will be requested first, and when we receive it, we'll see there's already an entry for that txid in the orphan pool and ignore it. I'm not sure if we'll re-request from other honest peers at that point in normal usage; though I think this is a case where #21061 would eventually save the day.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if we'll re-request from other honest peers at that point in normal usage

Once the transaction is rejected from orphanage in reason of an already present orphan, we forget its txid/wtxid from tx-requester. So effectively, we shouldn't re-request it from other onest peers at that stage. Ultimately, parent P withhold by Mallory should expire, Alice will fetch a valid parent P from another peer. When Bob rebroadcasts C, Mallory isn't able to interfere anymore as malleated X will be rejected directly, without being logged in orphan pool.

Yes I agree #21061 should make things better.

{
LOCK(g_cs_orphans);
if (gtxid.IsWtxid()) {
return m_wtxid_to_orphan_it.count(gtxid.GetHash());
} else {
return m_orphans.count(gtxid.GetHash());
}
}

std::pair<CTransactionRef, NodeId> TxOrphanage::GetTx(const uint256& txid) const
{
AssertLockHeld(g_cs_orphans);

const auto it = m_orphans.find(txid);
if (it == m_orphans.end()) return {nullptr, -1};
return {it->second.tx, it->second.fromPeer};
}

void TxOrphanage::EraseForBlock(const CBlock& block)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we had a std::map<uint256, OrphanMap::iterator> m_parentid_to_orphan_it index we could update each peer's orphan_work_set in function of the parent received through this block.

Otherwise, I think you might have stalling valid orphans never re-processed. Any ulterior parent announcement should be bounce off by m_recent_confirmed_transactions. Though rare enough to not be worthy of the complexity...

{
LOCK(g_cs_orphans);

std::vector<uint256> vOrphanErase;

for (const CTransactionRef& ptx : block.vtx) {
const CTransaction& tx = *ptx;

// Which orphan pool entries must we evict?
for (const auto& txin : tx.vin) {
auto itByPrev = m_outpoint_to_orphan_it.find(txin.prevout);
if (itByPrev == m_outpoint_to_orphan_it.end()) continue;
for (auto mi = itByPrev->second.begin(); mi != itByPrev->second.end(); ++mi) {
const CTransaction& orphanTx = *(*mi)->second.tx;
const uint256& orphanHash = orphanTx.GetHash();
vOrphanErase.push_back(orphanHash);
}
}
}

// Erase orphan transactions included or precluded by this block
if (vOrphanErase.size()) {
int nErased = 0;
for (const uint256& orphanHash : vOrphanErase) {
nErased += EraseTx(orphanHash);
}
LogPrint(BCLog::MEMPOOL, "Erased %d orphan tx included or conflicted by block\n", nErased);
}
}
Loading