highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-06-02 01:04:43 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#26295: Replace global g_cs_orphans lock with local

Browse Source 
7082ce3e88 scripted-diff: rename and de-globalise g_cs_orphans (Anthony Towns)
733d85f79c Move all g_cs_orphans locking to txorphanage (Anthony Towns)
a936f41a5d txorphanage: make m_peer_work_set private (Anthony Towns)
3614819864 txorphange: move orphan workset to txorphanage (Anthony Towns)
6f8e442ba6 net_processing: Localise orphan_work_set handling to ProcessOrphanTx (Anthony Towns)
0027174b39 net_processing: move ProcessOrphanTx docs to declaration (Anthony Towns)
9910ed755c net_processing: Pass a Peer& to ProcessOrphanTx (Anthony Towns)
89e2e0da0b net_processing: move extra transactions to msgproc mutex (Anthony Towns)
ff8d44d196 Remove unnecessary includes of txorphange.h (Anthony Towns)

Pull request description:

  Moves extra transactions to be under the `m_msgproc_mutex` lock rather than `g_cs_orphans` and refactors orphan handling so that the lock can be internal to the `TxOrphange` class.

ACKs for top commit:
  dergoegge:
    Code review ACK 7082ce3e88
  glozow:
    ACK 7082ce3e88 via code review and some [basic testing](https://github.com/glozow/bitcoin/blob/review-26295/src/test/orphanage_tests.cpp#L150). I think putting txorphanage in charge of handling peer work sets is the right direction.

Tree-SHA512: 1ec454c3a69ebd45ff652770d6a55c6b183db71aba4d12639ed70f525f0035e069a81d06e9b65b66e87929c607080a1c5e5dcd2ca91eaa2cf202dc6c02aa6818
This commit is contained in:
glozow
2022-11-28 10:51:15 +00:00
parent 9c2854cda4 7082ce3e88
commit a79b720092
8 changed files with 124 additions and 97 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
src/test/fuzz/process_message.cpp
View File

@@ -19,7 +19,6 @@
#include <test/util/net.h>
#include <test/util/setup_common.h>
#include <test/util/validation.h>
#include <txorphanage.h>
#include <validationinterface.h>
#include <version.h>

1
src/test/fuzz/process_messages.cpp
View File

@@ -14,7 +14,6 @@
#include <test/util/net.h>
#include <test/util/setup_common.h>
#include <test/util/validation.h>
#include <txorphanage.h>
#include <validation.h>
#include <validationinterface.h>

24
src/test/fuzz/txorphan.cpp
View File

@@ -36,7 +36,6 @@ FUZZ_TARGET_INIT(txorphan, initialize_orphanage)
SetMockTime(ConsumeTime(fuzzed_data_provider));
TxOrphanage orphanage;
std::set<uint256> orphan_work_set;
std::vector<COutPoint> outpoints;
// initial outpoints used to construct transactions later
for (uint8_t i = 0; i < 4; i++) {
@@ -86,15 +85,19 @@ FUZZ_TARGET_INIT(txorphan, initialize_orphanage)
CallOneOf(
fuzzed_data_provider,
[&] {
LOCK(g_cs_orphans);
orphanage.AddChildrenToWorkSet(*tx, orphan_work_set);
orphanage.AddChildrenToWorkSet(*tx, peer_id);
},
[&] {
bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetHash()));
{
LOCK(g_cs_orphans);
bool get_tx = orphanage.GetTx(tx->GetHash()).first != nullptr;
Assert(have_tx == get_tx);
NodeId originator;
bool more = true;
CTransactionRef ref = orphanage.GetTxToReconsider(peer_id, originator, more);
if (!ref) {
Assert(!more);
} else {
bool have_tx = orphanage.HaveTx(GenTxid::Txid(ref->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(ref->GetHash()));
Assert(have_tx);
}
}
},
[&] {
@@ -102,14 +105,12 @@ FUZZ_TARGET_INIT(txorphan, initialize_orphanage)
// AddTx should return false if tx is too big or already have it
// tx weight is unknown, we only check when tx is already in orphanage
{
LOCK(g_cs_orphans);
bool add_tx = orphanage.AddTx(tx, peer_id);
// have_tx == true -> add_tx == false
Assert(!have_tx || !add_tx);
}
have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetHash()));
{
LOCK(g_cs_orphans);
bool add_tx = orphanage.AddTx(tx, peer_id);
// if have_tx is still false, it must be too big
Assert(!have_tx == (GetTransactionWeight(*tx) > MAX_STANDARD_TX_WEIGHT));
@@ -120,25 +121,22 @@ FUZZ_TARGET_INIT(txorphan, initialize_orphanage)
bool have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetHash()));
// EraseTx should return 0 if m_orphans doesn't have the tx
{
LOCK(g_cs_orphans);
Assert(have_tx == orphanage.EraseTx(tx->GetHash()));
}
have_tx = orphanage.HaveTx(GenTxid::Txid(tx->GetHash())) || orphanage.HaveTx(GenTxid::Wtxid(tx->GetHash()));
// have_tx should be false and EraseTx should fail
{
LOCK(g_cs_orphans);
Assert(!have_tx && !orphanage.EraseTx(tx->GetHash()));
}
},
[&] {
LOCK(g_cs_orphans);
orphanage.EraseForPeer(peer_id);
},
[&] {
// test mocktime and expiry
SetMockTime(ConsumeTime(fuzzed_data_provider));
auto limit = fuzzed_data_provider.ConsumeIntegral<unsigned int>();
WITH_LOCK(g_cs_orphans, orphanage.LimitOrphans(limit));
orphanage.LimitOrphans(limit);
Assert(orphanage.Size() <= limit);
});
}

8
src/test/orphanage_tests.cpp
View File

@@ -20,13 +20,15 @@ BOOST_FIXTURE_TEST_SUITE(orphanage_tests, TestingSetup)
class TxOrphanageTest : public TxOrphanage
{
public:
inline size_t CountOrphans() const EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans)
inline size_t CountOrphans() const EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
{
LOCK(m_mutex);
return m_orphans.size();
}
CTransactionRef RandomOrphan() EXCLUSIVE_LOCKS_REQUIRED(g_cs_orphans)
CTransactionRef RandomOrphan() EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)
{
LOCK(m_mutex);
std::map<uint256, OrphanTx>::iterator it;
it = m_orphans.lower_bound(InsecureRand256());
if (it == m_orphans.end())
@@ -59,8 +61,6 @@ BOOST_AUTO_TEST_CASE(DoS_mapOrphans)
FillableSigningProvider keystore;
BOOST_CHECK(keystore.AddKey(key));
LOCK(g_cs_orphans);
// 50 orphan transactions:
for (int i = 0; i < 50; i++)
{