Skip to content

Commit

Permalink
Fix ODR violation leading to subtle build error on Fedora
Browse files Browse the repository at this point in the history 
Summary
---

See the description in issue Bitcoin-ABC#176. This closes Bitcoin-ABC#176.  The basic jist of
it is that we inherited some questionable code from Core in which
mapOrphanTransactions was declared inconsistently and this caused newer
GCC to emit bad code and/or complain.

The fix is to actually declare things only once in 1 place and not have
such footguns waiting to go off in the codebase.  See diff.

Test Plan
---

- `ninja check check-functional`
  • Loading branch information
@cculianu @ftrader
cculianu authored and ftrader committed Oct 19, 2020
1 parent b4cc9e2 commit a320927
Show file tree
Hide file tree
Showing 3 changed files with 67 additions and 68 deletions.
59 changes: 26 additions & 33 deletions src/net_processing.cpp
View file
Expand Up @@ -121,17 +121,10 @@ static const unsigned int MAX_GETDATA_SZ = 1000;
/// How many non standard orphan do we consider from a node before ignoring it.
static constexpr uint32_t MAX_NON_STANDARD_ORPHAN_PER_NODE = 5;

struct COrphanTx {
// When modifying, adapt the copy of this definition in tests/DoS_tests.
CTransactionRef tx;
NodeId fromPeer;
int64_t nTimeExpire;
};

namespace internal {
RecursiveMutex g_cs_orphans;
std::map<TxId, COrphanTx> mapOrphanTransactions GUARDED_BY(g_cs_orphans);

void EraseOrphansFor(NodeId peer);
}

/**
* Average delay between local address broadcasts in seconds.
Expand Down Expand Up @@ -246,12 +239,12 @@ struct IteratorComparator {
}
};
std::map<COutPoint,
std::set<std::map<TxId, COrphanTx>::iterator, IteratorComparator>>
mapOrphanTransactionsByPrev GUARDED_BY(g_cs_orphans);
std::set<std::map<TxId, internal::COrphanTx>::iterator, IteratorComparator>>
mapOrphanTransactionsByPrev GUARDED_BY(internal::g_cs_orphans);

static size_t vExtraTxnForCompactIt GUARDED_BY(g_cs_orphans) = 0;
static size_t vExtraTxnForCompactIt GUARDED_BY(internal::g_cs_orphans) = 0;
static std::vector<std::pair<TxHash, CTransactionRef>>
vExtraTxnForCompact GUARDED_BY(g_cs_orphans);
vExtraTxnForCompact GUARDED_BY(internal::g_cs_orphans);
} // namespace

namespace {
Expand Down Expand Up @@ -902,7 +895,7 @@ void RequestTx(CNodeState *state, const TxId &txid, int64_t nNow)

// This function is used for testing the stale tip eviction logic, see
// denialofservice_tests.cpp
void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) {
void internal::UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds) {
LOCK(cs_main);
CNodeState *state = State(node);
if (state) {
Expand Down Expand Up @@ -951,7 +944,7 @@ void PeerLogicValidation::FinalizeNode(const Config &config, NodeId nodeid,
for (const QueuedBlock &entry : state->vBlocksInFlight) {
mapBlocksInFlight.erase(entry.hash);
}
EraseOrphansFor(nodeid);
internal::EraseOrphansFor(nodeid);
nPreferredDownload -= state->fPreferredDownload;
nPeersWithValidatedDownloads -= (state->nBlocksInFlightValidHeaders != 0);
assert(nPeersWithValidatedDownloads >= 0);
Expand Down Expand Up @@ -997,7 +990,7 @@ bool GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats) {
//

static void AddToCompactExtraTransactions(const CTransactionRef &tx)
EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans) {
EXCLUSIVE_LOCKS_REQUIRED(internal::g_cs_orphans) {
size_t max_extra_txn = gArgs.GetArg("-blockreconstructionextratxn",
DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN);
if (max_extra_txn <= 0) {
Expand All @@ -1013,7 +1006,7 @@ static void AddToCompactExtraTransactions(const CTransactionRef &tx)
vExtraTxnForCompactIt = (vExtraTxnForCompactIt + 1) % max_extra_txn;
}

bool AddOrphanTx(const CTransactionRef &tx, NodeId peer)
bool internal::AddOrphanTx(const CTransactionRef &tx, NodeId peer)
EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans) {
const TxId &txid = tx->GetId();
if (mapOrphanTransactions.count(txid)) {
Expand Down Expand Up @@ -1044,21 +1037,21 @@ bool AddOrphanTx(const CTransactionRef &tx, NodeId peer)
AddToCompactExtraTransactions(tx);

LogPrint(BCLog::MEMPOOL, "stored orphan tx %s (mapsz %u outsz %u)\n",
txid.ToString(), mapOrphanTransactions.size(),
txid.ToString(), internal::mapOrphanTransactions.size(),
mapOrphanTransactionsByPrev.size());
return true;
}

static int EraseOrphanTx(const TxId &id) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans) {
const auto it = mapOrphanTransactions.find(id);
if (it == mapOrphanTransactions.end()) {
static int EraseOrphanTx(const TxId &id) EXCLUSIVE_LOCKS_REQUIRED(internal::g_cs_orphans) {
const auto it = internal::mapOrphanTransactions.find(id);
if (it == internal::mapOrphanTransactions.end()) {
return 0;
}
// Note: parameter `id` may not be used beyond this point since it may point
// to data we will erase, potentially. So we wrap the work we do here in the
// lambda below, to ensure no future programmer inadvertently accesses `id`
// while looping below.
return [&it]() EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans) {
return [&it]() EXCLUSIVE_LOCKS_REQUIRED(internal::g_cs_orphans) {
for (const CTxIn &txin : it->second.tx->vin) {
const auto itPrev = mapOrphanTransactionsByPrev.find(txin.prevout);
if (itPrev == mapOrphanTransactionsByPrev.end()) {
Expand All @@ -1069,12 +1062,12 @@ static int EraseOrphanTx(const TxId &id) EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans)
mapOrphanTransactionsByPrev.erase(itPrev);
}
}
mapOrphanTransactions.erase(it);
internal::mapOrphanTransactions.erase(it);
return 1;
}();
}

void EraseOrphansFor(NodeId peer) {
void internal::EraseOrphansFor(NodeId peer) {
LOCK(g_cs_orphans);
int nErased = 0;
auto iter = mapOrphanTransactions.begin();
Expand All @@ -1091,7 +1084,7 @@ void EraseOrphansFor(NodeId peer) {
}
}

unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans) {
unsigned int internal::LimitOrphanTxSize(unsigned int nMaxOrphans) {
LOCK(g_cs_orphans);

unsigned int nEvicted = 0;
Expand Down Expand Up @@ -1229,7 +1222,7 @@ PeerLogicValidation::PeerLogicValidation(CConnman *connmanIn, BanMan *banman,
void PeerLogicValidation::BlockConnected(
const std::shared_ptr<const CBlock> &pblock, const CBlockIndex *pindex,
const std::vector<CTransactionRef> &vtxConflicted) {
LOCK(g_cs_orphans);
LOCK(internal::g_cs_orphans);

std::vector<TxId> vOrphanErase;

Expand Down Expand Up @@ -1436,8 +1429,8 @@ static bool AlreadyHave(const CInv &inv) EXCLUSIVE_LOCKS_REQUIRED(cs_main) {

const TxId txid(inv.hash);
{
LOCK(g_cs_orphans);
if (mapOrphanTransactions.count(txid)) {
LOCK(internal::g_cs_orphans);
if (internal::mapOrphanTransactions.count(txid)) {
return true;
}
}
Expand Down Expand Up @@ -2837,7 +2830,7 @@ static bool ProcessMessage(const Config &config, CNode *pfrom,
CInv inv(MSG_TX, txid);
pfrom->AddInventoryKnown(inv);

LOCK2(cs_main, g_cs_orphans);
LOCK2(cs_main, internal::g_cs_orphans);

bool fMissingInputs = false;
CValidationState state;
Expand Down Expand Up @@ -2962,14 +2955,14 @@ static bool ProcessMessage(const Config &config, CNode *pfrom,
RequestTx(State(pfrom->GetId()), _txid, nNow);
}
}
AddOrphanTx(ptx, pfrom->GetId());
internal::AddOrphanTx(ptx, pfrom->GetId());

// DoS prevention: do not allow mapOrphanTransactions to grow
// unbounded
unsigned int nMaxOrphanTx = (unsigned int)std::max(
int64_t(0), gArgs.GetArg("-maxorphantx",
DEFAULT_MAX_ORPHAN_TRANSACTIONS));
unsigned int nEvicted = LimitOrphanTxSize(nMaxOrphanTx);
unsigned int nEvicted = internal::LimitOrphanTxSize(nMaxOrphanTx);
if (nEvicted > 0) {
LogPrint(BCLog::MEMPOOL,
"mapOrphan overflow, removed %u tx\n", nEvicted);
Expand Down Expand Up @@ -3131,7 +3124,7 @@ static bool ProcessMessage(const Config &config, CNode *pfrom,
bool fBlockReconstructed = false;

{
LOCK2(cs_main, g_cs_orphans);
LOCK2(cs_main, internal::g_cs_orphans);
// If AcceptBlockHeader returned true, it set pindex
assert(pindex);
UpdateBlockAvailability(pfrom->GetId(), pindex->GetBlockHash());
Expand Down Expand Up @@ -4878,7 +4871,7 @@ class CNetProcessingCleanup {
CNetProcessingCleanup() {}
~CNetProcessingCleanup() {
// orphan transactions
mapOrphanTransactions.clear();
internal::mapOrphanTransactions.clear();
mapOrphanTransactionsByPrev.clear();
}
} instance_of_cnetprocessingcleanup;
23 changes: 23 additions & 0 deletions src/net_processing.h
View file
Expand Up @@ -11,6 +11,8 @@
#include <sync.h>
#include <validationinterface.h>

#include <map>

extern RecursiveMutex cs_main;

class Config;
Expand Down Expand Up @@ -132,4 +134,25 @@ bool GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats);
/** Increase a node's misbehavior score. */
void Misbehaving(NodeId nodeid, int howmuch, const std::string &reason = "");

// `internal` namespace exposed *FOR TESTS ONLY*
// This namespace is for exposed internals not intended for public usage.
// We would ideally have made these private to the net_processing.cpp
// translation unit only, but since some tests need to see these functions
// (see denialofservice_tests.cpp), we do this instead.
namespace internal {
struct COrphanTx {
CTransactionRef tx;
NodeId fromPeer;
int64_t nTimeExpire;
};

extern RecursiveMutex g_cs_orphans;
extern std::map<TxId, COrphanTx> mapOrphanTransactions GUARDED_BY(g_cs_orphans);

bool AddOrphanTx(const CTransactionRef &tx, NodeId peer);
void EraseOrphansFor(NodeId peer);
unsigned int LimitOrphanTxSize(unsigned int nMaxOrphans);
void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds);
} // namespace internal

#endif // BITCOIN_NET_PROCESSING_H
53 changes: 18 additions & 35 deletions src/test/denialofservice_tests.cpp
View file
Expand Up @@ -40,20 +40,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 RecursiveMutex g_cs_orphans;
extern std::map<uint256, COrphanTx>
mapOrphanTransactions GUARDED_BY(g_cs_orphans);

static CService ip(uint32_t i) {
struct in_addr s;
s.s_addr = i;
Expand All @@ -62,8 +48,6 @@ static CService ip(uint32_t i) {

static NodeId id = 0;

void UpdateLastBlockAnnounceTime(NodeId node, int64_t time_in_seconds);

BOOST_FIXTURE_TEST_SUITE(denialofservice_tests, TestingSetup)

// Test eviction of an outbound peer whose chain never advances
Expand Down Expand Up @@ -213,7 +197,7 @@ BOOST_AUTO_TEST_CASE(stale_tip_peer_management) {

// Update the last announced block time for the last
// peer, and check that the next newest node gets evicted.
UpdateLastBlockAnnounceTime(vNodes.back()->GetId(), GetTime());
internal::UpdateLastBlockAnnounceTime(vNodes.back()->GetId(), GetTime());

peerLogic->CheckForStaleTipAndEvictPeers(consensusParams);
for (int i = 0; i < nMaxOutbound - 1; ++i) {
Expand Down Expand Up @@ -465,11 +449,10 @@ BOOST_AUTO_TEST_CASE(DoS_DiscourageRolling) {
}

static CTransactionRef RandomOrphan() {
std::map<uint256, COrphanTx>::iterator it;
LOCK2(cs_main, g_cs_orphans);
it = mapOrphanTransactions.lower_bound(InsecureRand256());
if (it == mapOrphanTransactions.end()) {
it = mapOrphanTransactions.begin();
LOCK2(cs_main, internal::g_cs_orphans);
auto it = internal::mapOrphanTransactions.lower_bound(TxId{InsecureRand256()});
if (it == internal::mapOrphanTransactions.end()) {
it = internal::mapOrphanTransactions.begin();
}
return it->second.tx;
}
Expand All @@ -491,7 +474,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) {
tx.vout[0].scriptPubKey =
GetScriptForDestination(key.GetPubKey().GetID());

AddOrphanTx(MakeTransactionRef(tx), i);
internal::AddOrphanTx(MakeTransactionRef(tx), i);
}

// ... and 50 that depend on other orphans:
Expand All @@ -507,7 +490,7 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) {
GetScriptForDestination(key.GetPubKey().GetID());
SignSignature(keystore, *txPrev, tx, 0, SigHashType());

AddOrphanTx(MakeTransactionRef(tx), i);
internal::AddOrphanTx(MakeTransactionRef(tx), i);
}

// This really-big orphan should be ignored:
Expand All @@ -530,24 +513,24 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans) {
tx.vin[j].scriptSig = tx.vin[0].scriptSig;
}

BOOST_CHECK(!AddOrphanTx(MakeTransactionRef(tx), i));
BOOST_CHECK(!internal::AddOrphanTx(MakeTransactionRef(tx), i));
}

LOCK2(cs_main, g_cs_orphans);
LOCK2(cs_main, internal::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 = internal::mapOrphanTransactions.size();
internal::EraseOrphansFor(i);
BOOST_CHECK(internal::mapOrphanTransactions.size() < sizeBefore);
}

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

BOOST_AUTO_TEST_SUITE_END()

0 comments on commit a320927

Please sign in to comment.