Skip to content

Commit 8d78e7d

Browse files
kwvgkwvg
committed
refactor: remove need for mutual access to private members
1 parent f22fb22 commit 8d78e7d

File tree

4 files changed

+112
-74
lines changed

4 files changed

+112
-74
lines changed

‎src/instantsend/instantsend.cpp‎

Lines changed: 57 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,21 @@ using node::GetTransaction;
4343
namespace llmq {
4444
static const std::string_view INPUTLOCK_REQUESTID_PREFIX = "inlock";
4545

46+
namespace {
47+
template <typename T>
48+
requires std::same_as<T, CTxIn> || std::same_as<T, COutPoint>
49+
std::unordered_set<uint256, StaticSaltedHasher> GetIdsFromLockable(const std::vector<T>& vec)
50+
{
51+
std::unordered_set<uint256, StaticSaltedHasher> ret{};
52+
if (vec.empty()) return ret;
53+
ret.reserve(vec.size());
54+
for (const auto& in : vec) {
55+
ret.emplace(::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, in)));
56+
}
57+
return ret;
58+
}
59+
} // anonymous namespace
60+
4661
CInstantSendManager::CInstantSendManager(CChainLocksHandler& _clhandler, CChainState& chainstate, CQuorumManager& _qman,
4762
CSigningManager& _sigman, CSigSharesManager& _shareman,
4863
CSporkManager& sporkman, CTxMemPool& _mempool, const CMasternodeSync& mn_sync,
@@ -74,14 +89,14 @@ void CInstantSendManager::Start(PeerManager& peerman)
7489
workThread = std::thread(&util::TraceThread, "isman", [this, &peerman] { WorkThreadMain(peerman); });
7590

7691
if (m_signer) {
77-
sigman.RegisterRecoveredSigsListener(m_signer.get());
92+
m_signer->Start();
7893
}
7994
}
8095

8196
void CInstantSendManager::Stop()
8297
{
8398
if (m_signer) {
84-
sigman.UnregisterRecoveredSigsListener(m_signer.get());
99+
m_signer->Stop();
85100
}
86101

87102
// make sure to call InterruptWorkerThread() first
@@ -344,9 +359,7 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, PeerManager& peerm
344359
LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s: processing islock, peer=%d\n", __func__,
345360
islock->txid.ToString(), hash.ToString(), from);
346361
if (m_signer) {
347-
LOCK(m_signer->cs_creating);
348-
m_signer->creatingInstantSendLocks.erase(islock->GetRequestId());
349-
m_signer->txToCreatingInstantSendLocks.erase(islock->txid);
362+
m_signer->ClearLockFromQueue(islock);
350363
}
351364
if (db.KnownInstantSendLock(hash)) {
352365
return;
@@ -592,23 +605,28 @@ void CInstantSendManager::RemoveNonLockedTx(const uint256& txid, bool retryChild
592605
void CInstantSendManager::RemoveConflictedTx(const CTransaction& tx)
593606
{
594607
RemoveNonLockedTx(tx.GetHash(), false);
595-
if (!m_signer) return;
596-
597-
LOCK(m_signer->cs_inputReqests);
598-
for (const auto& in : tx.vin) {
599-
auto inputRequestId = ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, in));
600-
m_signer->inputRequestIds.erase(inputRequestId);
608+
if (m_signer) {
609+
m_signer->ClearInputsFromQueue(GetIdsFromLockable(tx.vin));
601610
}
602611
}
603612

604613
void CInstantSendManager::TruncateRecoveredSigsForInputs(const llmq::CInstantSendLock& islock)
605614
{
606-
if (!m_signer) return;
615+
auto ids = GetIdsFromLockable(islock.inputs);
616+
if (m_signer) {
617+
m_signer->ClearInputsFromQueue(ids);
618+
}
619+
for (const auto& id : ids) {
620+
sigman.TruncateRecoveredSig(Params().GetConsensus().llmqTypeDIP0024InstantSend, id);
621+
}
622+
}
607623

608-
for (const auto& in : islock.inputs) {
609-
auto inputRequestId = ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, in));
610-
WITH_LOCK(m_signer->cs_inputReqests, m_signer->inputRequestIds.erase(inputRequestId));
611-
sigman.TruncateRecoveredSig(Params().GetConsensus().llmqTypeDIP0024InstantSend, inputRequestId);
624+
void CInstantSendManager::TryEmplacePendingLock(const uint256& hash, const NodeId id, const CInstantSendLockPtr& islock)
625+
{
626+
if (db.KnownInstantSendLock(hash)) return;
627+
LOCK(cs_pendingLocks);
628+
if (!pendingInstantSendLocks.count(hash)) {
629+
pendingInstantSendLocks.emplace(hash, std::make_pair(id, islock));
612630
}
613631
}
614632

@@ -910,10 +928,29 @@ size_t CInstantSendManager::GetInstantSendLockCount() const
910928
void CInstantSendManager::WorkThreadMain(PeerManager& peerman)
911929
{
912930
while (!workInterrupt) {
913-
bool fMoreWork = ProcessPendingInstantSendLocks(peerman);
914-
if (m_signer) {
915-
m_signer->ProcessPendingRetryLockTxs();
916-
}
931+
bool fMoreWork = [&]() -> bool {
932+
if (!IsInstantSendEnabled()) return false;
933+
const bool more_work{ProcessPendingInstantSendLocks(peerman)};
934+
if (!m_signer) return more_work;
935+
// Construct set of non-locked transactions that are pending to retry
936+
std::vector<CTransactionRef> txns{};
937+
{
938+
LOCK2(cs_nonLocked, cs_pendingRetry);
939+
if (pendingRetryTxs.empty()) return more_work;
940+
txns.reserve(pendingRetryTxs.size());
941+
for (const auto& txid : pendingRetryTxs) {
942+
if (auto it = nonLockedTxs.find(txid); it != nonLockedTxs.end()) {
943+
const auto& [_, tx_info] = *it;
944+
if (tx_info.tx) {
945+
txns.push_back(tx_info.tx);
946+
}
947+
}
948+
}
949+
}
950+
// Retry processing them
951+
m_signer->ProcessPendingRetryLockTxs(txns);
952+
return more_work;
953+
}();
917954

918955
if (!fMoreWork && !workInterrupt.sleep_for(std::chrono::milliseconds(100))) {
919956
return;

‎src/instantsend/instantsend.h‎

Lines changed: 3 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -120,11 +120,7 @@ class CInstantSendManager
120120
EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry);
121121

122122
void WorkThreadMain(PeerManager& peerman)
123-
NO_THREAD_SAFETY_ANALYSIS;
124-
// Needed as compiler cannot differentiate between negative capability against member and against
125-
// member accessed through reference.
126-
// TODO: Remove this, it's terrible.
127-
// EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry);
123+
EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingLocks, !cs_pendingRetry);
128124

129125
void HandleFullyConfirmedBlock(const CBlockIndex* pindex)
130126
EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingRetry);
@@ -154,6 +150,8 @@ class CInstantSendManager
154150
EXCLUSIVE_LOCKS_REQUIRED(!cs_nonLocked, !cs_pendingRetry);
155151

156152
void RemoveConflictingLock(const uint256& islockHash, const CInstantSendLock& islock);
153+
void TryEmplacePendingLock(const uint256& hash, const NodeId id, const CInstantSendLockPtr& islock)
154+
EXCLUSIVE_LOCKS_REQUIRED(!cs_pendingLocks);
157155

158156
size_t GetInstantSendLockCount() const;
159157

@@ -164,8 +162,6 @@ class CInstantSendManager
164162
* allows ChainLocks to continue even while this spork is disabled.
165163
*/
166164
bool RejectConflictingBlocks() const;
167-
168-
friend class ::instantsend::InstantSendSigner;
169165
};
170166
} // namespace llmq
171167

‎src/instantsend/signing.cpp‎

Lines changed: 30 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
#include <util/irange.h>
1212
#include <validation.h>
1313

14-
#include <instantsend/db.h>
14+
#include <instantsend/instantsend.h>
1515
#include <llmq/chainlocks.h>
1616
#include <llmq/quorums.h>
1717
#include <masternode/sync.h>
@@ -48,6 +48,31 @@ InstantSendSigner::InstantSendSigner(CChainState& chainstate, llmq::CChainLocksH
4848

4949
InstantSendSigner::~InstantSendSigner() = default;
5050

51+
void InstantSendSigner::Start()
52+
{
53+
m_sigman.RegisterRecoveredSigsListener(this);
54+
}
55+
56+
void InstantSendSigner::Stop()
57+
{
58+
m_sigman.UnregisterRecoveredSigsListener(this);
59+
}
60+
61+
void InstantSendSigner::ClearInputsFromQueue(const std::unordered_set<uint256, StaticSaltedHasher>& ids)
62+
{
63+
LOCK(cs_inputReqests);
64+
for (const auto& id : ids) {
65+
inputRequestIds.erase(id);
66+
}
67+
}
68+
69+
void InstantSendSigner::ClearLockFromQueue(const llmq::CInstantSendLockPtr& islock)
70+
{
71+
LOCK(cs_creating);
72+
creatingInstantSendLocks.erase(islock->GetRequestId());
73+
txToCreatingInstantSendLocks.erase(islock->txid);
74+
}
75+
5176
MessageProcessingResult InstantSendSigner::HandleNewRecoveredSig(const llmq::CRecoveredSig& recoveredSig)
5277
{
5378
if (!m_isman.IsInstantSendEnabled()) {
@@ -101,34 +126,15 @@ void InstantSendSigner::HandleNewInputLockRecoveredSig(const llmq::CRecoveredSig
101126
TrySignInstantSendLock(*tx);
102127
}
103128

104-
void InstantSendSigner::ProcessPendingRetryLockTxs()
129+
void InstantSendSigner::ProcessPendingRetryLockTxs(const std::vector<CTransactionRef>& retryTxs)
105130
{
106-
const auto retryTxs = WITH_LOCK(m_isman.cs_pendingRetry, return m_isman.pendingRetryTxs);
107-
108-
if (retryTxs.empty()) {
109-
return;
110-
}
111-
112131
if (!m_isman.IsInstantSendEnabled()) {
113132
return;
114133
}
115134

116135
int retryCount = 0;
117-
for (const auto& txid : retryTxs) {
118-
CTransactionRef tx;
136+
for (const auto& tx : retryTxs) {
119137
{
120-
{
121-
LOCK(m_isman.cs_nonLocked);
122-
auto it = m_isman.nonLockedTxs.find(txid);
123-
if (it == m_isman.nonLockedTxs.end()) {
124-
continue;
125-
}
126-
tx = it->second.tx;
127-
}
128-
if (!tx) {
129-
continue;
130-
}
131-
132138
if (LOCK(cs_creating); txToCreatingInstantSendLocks.count(tx->GetHash())) {
133139
// we're already in the middle of locking this one
134140
continue;
@@ -157,8 +163,7 @@ void InstantSendSigner::ProcessPendingRetryLockTxs()
157163
}
158164

159165
if (retryCount != 0) {
160-
LogPrint(BCLog::INSTANTSEND, "%s -- retried %d TXs. nonLockedTxs.size=%d\n", __func__,
161-
retryCount, WITH_LOCK(m_isman.cs_nonLocked, return m_isman.nonLockedTxs.size()));
166+
LogPrint(BCLog::INSTANTSEND, "%s -- retried %d TXs.\n", __func__, retryCount);
162167
}
163168
}
164169

@@ -244,13 +249,7 @@ void InstantSendSigner::HandleNewInstantSendLockRecoveredSig(const llmq::CRecove
244249
}
245250

246251
islock->sig = recoveredSig.sig;
247-
auto hash = ::SerializeHash(*islock);
248-
249-
if (WITH_LOCK(m_isman.cs_pendingLocks, return m_isman.pendingInstantSendLocks.count(hash)) || m_isman.db.KnownInstantSendLock(hash)) {
250-
return;
251-
}
252-
LOCK(m_isman.cs_pendingLocks);
253-
m_isman.pendingInstantSendLocks.emplace(hash, std::make_pair(-1, islock));
252+
m_isman.TryEmplacePendingLock(/*hash=*/::SerializeHash(*islock), /*id=*/-1, islock);
254253
}
255254

256255
void InstantSendSigner::ProcessTx(const CTransaction& tx, bool fRetroactive, const Consensus::Params& params)

‎src/instantsend/signing.h‎

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,6 @@
55
#ifndef BITCOIN_INSTANTSEND_SIGNING_H
66
#define BITCOIN_INSTANTSEND_SIGNING_H
77

8-
#include <instantsend/instantsend.h>
98
#include <instantsend/lock.h>
109
#include <llmq/signing.h>
1110

@@ -65,33 +64,40 @@ class InstantSendSigner : public llmq::CRecoveredSigsListener
6564
CTxMemPool& mempool, const CMasternodeSync& mn_sync);
6665
~InstantSendSigner();
6766

67+
void Start();
68+
void Stop();
69+
70+
void ClearInputsFromQueue(const std::unordered_set<uint256, StaticSaltedHasher>& ids)
71+
EXCLUSIVE_LOCKS_REQUIRED(!cs_inputReqests);
72+
73+
void ClearLockFromQueue(const llmq::CInstantSendLockPtr& islock)
74+
EXCLUSIVE_LOCKS_REQUIRED(!cs_creating);
75+
6876
[[nodiscard]] MessageProcessingResult HandleNewRecoveredSig(const llmq::CRecoveredSig& recoveredSig) override
69-
EXCLUSIVE_LOCKS_REQUIRED(!cs_creating, !cs_inputReqests, !m_isman.cs_pendingLocks);
77+
EXCLUSIVE_LOCKS_REQUIRED(!cs_creating, !cs_inputReqests);
78+
79+
void ProcessPendingRetryLockTxs(const std::vector<CTransactionRef>& retryTxs)
80+
EXCLUSIVE_LOCKS_REQUIRED(!cs_creating, !cs_inputReqests);
81+
void ProcessTx(const CTransaction& tx, bool fRetroactive, const Consensus::Params& params)
82+
EXCLUSIVE_LOCKS_REQUIRED(!cs_creating, !cs_inputReqests);
7083

7184
private:
72-
bool CheckCanLock(const CTransaction& tx, bool printDebug, const Consensus::Params& params) const;
73-
bool CheckCanLock(const COutPoint& outpoint, bool printDebug, const uint256& txHash,
74-
const Consensus::Params& params) const;
85+
[[nodiscard]] bool CheckCanLock(const CTransaction& tx, bool printDebug, const Consensus::Params& params) const;
86+
[[nodiscard]] bool CheckCanLock(const COutPoint& outpoint, bool printDebug, const uint256& txHash,
87+
const Consensus::Params& params) const;
7588

7689
void HandleNewInputLockRecoveredSig(const llmq::CRecoveredSig& recoveredSig, const uint256& txid)
7790
EXCLUSIVE_LOCKS_REQUIRED(!cs_creating);
7891
void HandleNewInstantSendLockRecoveredSig(const llmq::CRecoveredSig& recoveredSig)
79-
EXCLUSIVE_LOCKS_REQUIRED(!cs_creating, !m_isman.cs_pendingLocks);
92+
EXCLUSIVE_LOCKS_REQUIRED(!cs_creating);
8093

81-
bool IsInstantSendMempoolSigningEnabled() const;
94+
[[nodiscard]] bool IsInstantSendMempoolSigningEnabled() const;
8295

83-
void ProcessPendingRetryLockTxs()
84-
EXCLUSIVE_LOCKS_REQUIRED(!cs_creating, !cs_inputReqests, !m_isman.cs_nonLocked, !m_isman.cs_pendingRetry);
85-
void ProcessTx(const CTransaction& tx, bool fRetroactive, const Consensus::Params& params)
86-
EXCLUSIVE_LOCKS_REQUIRED(!cs_creating, !cs_inputReqests);
87-
88-
bool TrySignInputLocks(const CTransaction& tx, bool allowResigning, Consensus::LLMQType llmqType,
89-
const Consensus::Params& params)
96+
[[nodiscard]] bool TrySignInputLocks(const CTransaction& tx, bool allowResigning, Consensus::LLMQType llmqType,
97+
const Consensus::Params& params)
9098
EXCLUSIVE_LOCKS_REQUIRED(!cs_inputReqests);
9199
void TrySignInstantSendLock(const CTransaction& tx)
92100
EXCLUSIVE_LOCKS_REQUIRED(!cs_creating);
93-
94-
friend class ::llmq::CInstantSendManager;
95101
};
96102
} // namespace instantsend
97103

0 commit comments

Comments
 (0)