diff options
author | glozow <gloriajzhao@gmail.com> | 2022-11-28 10:51:15 +0000 |
---|---|---|
committer | glozow <gloriajzhao@gmail.com> | 2022-11-28 10:59:02 +0000 |
commit | a79b720092b6ce4134d386e95b3473a290f3c127 (patch) | |
tree | d5ea08b50521da91203407d8af86d510b80fa96a /src/test | |
parent | 9c2854cda4bee2a85d29f7303724aa7ea6fed64d (diff) | |
parent | 7082ce3e88d77456d60a5a66bd38807fbd66f311 (diff) | |
download | bitcoin-a79b720092b6ce4134d386e95b3473a290f3c127.tar.xz |
Merge bitcoin/bitcoin#26295: Replace global g_cs_orphans lock with local
7082ce3e88d77456d60a5a66bd38807fbd66f311 scripted-diff: rename and de-globalise g_cs_orphans (Anthony Towns)
733d85f79cde353d8c9b54370f296b1031fa33d9 Move all g_cs_orphans locking to txorphanage (Anthony Towns)
a936f41a5d5f7bb97425f82ec64dfae62e840a56 txorphanage: make m_peer_work_set private (Anthony Towns)
3614819864a84ac32f6d53c6ace79b5e71bc174a txorphange: move orphan workset to txorphanage (Anthony Towns)
6f8e442ba61378489a6e2e2ab5bcfbccca1a29ec net_processing: Localise orphan_work_set handling to ProcessOrphanTx (Anthony Towns)
0027174b396cacbaac5fd52f13be3dca9fcbbfb8 net_processing: move ProcessOrphanTx docs to declaration (Anthony Towns)
9910ed755c3dfd7669707b44d993a20030dd7477 net_processing: Pass a Peer& to ProcessOrphanTx (Anthony Towns)
89e2e0da0bcd0b0757d7b42907e2d2214da9f68b net_processing: move extra transactions to msgproc mutex (Anthony Towns)
ff8d44d1967d8ff9fb9b9ea0b87c0470d8cc2550 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 7082ce3e88d77456d60a5a66bd38807fbd66f311
glozow:
ACK 7082ce3e88d77456d60a5a66bd38807fbd66f311 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
Diffstat (limited to 'src/test')
-rw-r--r-- | src/test/fuzz/process_message.cpp | 1 | ||||
-rw-r--r-- | src/test/fuzz/process_messages.cpp | 1 | ||||
-rw-r--r-- | src/test/fuzz/txorphan.cpp | 24 | ||||
-rw-r--r-- | src/test/orphanage_tests.cpp | 8 |
4 files changed, 15 insertions, 19 deletions
diff --git a/src/test/fuzz/process_message.cpp b/src/test/fuzz/process_message.cpp index babd418b3e..f6000535b3 100644 --- a/src/test/fuzz/process_message.cpp +++ b/src/test/fuzz/process_message.cpp @@ -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> diff --git a/src/test/fuzz/process_messages.cpp b/src/test/fuzz/process_messages.cpp index 82e9f18d6d..41831fd176 100644 --- a/src/test/fuzz/process_messages.cpp +++ b/src/test/fuzz/process_messages.cpp @@ -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> diff --git a/src/test/fuzz/txorphan.cpp b/src/test/fuzz/txorphan.cpp index 55060f31cf..02743051e8 100644 --- a/src/test/fuzz/txorphan.cpp +++ b/src/test/fuzz/txorphan.cpp @@ -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); }); } diff --git a/src/test/orphanage_tests.cpp b/src/test/orphanage_tests.cpp index 842daa8bd4..a55b0bbcd0 100644 --- a/src/test/orphanage_tests.cpp +++ b/src/test/orphanage_tests.cpp @@ -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++) { |