Skip to content

Commit b7a9c57

Browse files
kwvgkwvg
committed
refactor: remove need for mutual access to private members
1 parent 318de4f commit b7a9c57

File tree

4 files changed

+114
-74
lines changed

4 files changed

+114
-74
lines changed

‎src/instantsend/instantsend.cpp‎

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

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

7894
if (m_activeman) {
79-
sigman.RegisterRecoveredSigsListener(m_activeman.get());
95+
m_activeman->Start();
8096
}
8197
}
8298

8399
void CInstantSendManager::Stop()
84100
{
85101
if (m_activeman) {
86-
sigman.UnregisterRecoveredSigsListener(m_activeman.get());
102+
m_activeman->Stop();
87103
}
88104

89105
// make sure to call InterruptWorkerThread() first
@@ -346,9 +362,7 @@ void CInstantSendManager::ProcessInstantSendLock(NodeId from, PeerManager& peerm
346362
LogPrint(BCLog::INSTANTSEND, "CInstantSendManager::%s -- txid=%s, islock=%s: processing islock, peer=%d\n", __func__,
347363
islock->txid.ToString(), hash.ToString(), from);
348364
if (m_activeman) {
349-
LOCK(m_activeman->cs_creating);
350-
m_activeman->creatingInstantSendLocks.erase(islock->GetRequestId());
351-
m_activeman->txToCreatingInstantSendLocks.erase(islock->txid);
365+
m_activeman->ClearLockFromQueue(islock);
352366
}
353367
if (db.KnownInstantSendLock(hash)) {
354368
return;
@@ -594,23 +608,28 @@ void CInstantSendManager::RemoveNonLockedTx(const uint256& txid, bool retryChild
594608
void CInstantSendManager::RemoveConflictedTx(const CTransaction& tx)
595609
{
596610
RemoveNonLockedTx(tx.GetHash(), false);
597-
if (!m_activeman) return;
598-
599-
LOCK(m_activeman->cs_inputReqests);
600-
for (const auto& in : tx.vin) {
601-
auto inputRequestId = ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, in));
602-
m_activeman->inputRequestIds.erase(inputRequestId);
611+
if (m_activeman) {
612+
m_activeman->ClearInputsFromQueue(GetIdsFromLockable(tx.vin));
603613
}
604614
}
605615

606616
void CInstantSendManager::TruncateRecoveredSigsForInputs(const llmq::CInstantSendLock& islock)
607617
{
608-
if (!m_activeman) return;
618+
auto ids = GetIdsFromLockable(islock.inputs);
619+
if (m_activeman) {
620+
m_activeman->ClearInputsFromQueue(ids);
621+
}
622+
for (const auto& id : ids) {
623+
sigman.TruncateRecoveredSig(Params().GetConsensus().llmqTypeDIP0024InstantSend, id);
624+
}
625+
}
609626

610-
for (const auto& in : islock.inputs) {
611-
auto inputRequestId = ::SerializeHash(std::make_pair(INPUTLOCK_REQUESTID_PREFIX, in));
612-
WITH_LOCK(m_activeman->cs_inputReqests, m_activeman->inputRequestIds.erase(inputRequestId));
613-
sigman.TruncateRecoveredSig(Params().GetConsensus().llmqTypeDIP0024InstantSend, inputRequestId);
627+
void CInstantSendManager::TryEmplacePendingLock(const uint256& hash, const NodeId id, const CInstantSendLockPtr& islock)
628+
{
629+
if (db.KnownInstantSendLock(hash)) return;
630+
LOCK(cs_pendingLocks);
631+
if (!pendingInstantSendLocks.count(hash)) {
632+
pendingInstantSendLocks.emplace(hash, std::make_pair(id, islock));
614633
}
615634
}
616635

@@ -912,10 +931,30 @@ size_t CInstantSendManager::GetInstantSendLockCount() const
912931
void CInstantSendManager::WorkThreadMain(PeerManager& peerman)
913932
{
914933
while (!workInterrupt) {
915-
bool fMoreWork = ProcessPendingInstantSendLocks(peerman);
916-
if (m_activeman) {
917-
m_activeman->ProcessPendingRetryLockTxs();
918-
}
934+
bool fMoreWork{false};
935+
do {
936+
if (!IsInstantSendEnabled()) break;
937+
fMoreWork = ProcessPendingInstantSendLocks(peerman);
938+
if (!m_activeman) break;
939+
// Construct set of non-locked transactions that are pending to retry
940+
std::vector<CTransactionRef> txns{};
941+
{
942+
LOCK2(cs_nonLocked, cs_pendingRetry);
943+
if (pendingRetryTxs.empty()) break;
944+
txns.reserve(pendingRetryTxs.size());
945+
for (const auto& txid : pendingRetryTxs) {
946+
if (auto it = nonLockedTxs.find(txid); it != nonLockedTxs.end()) {
947+
const auto& [_, tx_info] = *it;
948+
if (tx_info.tx) {
949+
txns.push_back(tx_info.tx);
950+
}
951+
}
952+
}
953+
txns.shrink_to_fit();
954+
}
955+
// Retry processing them
956+
m_activeman->ProcessPendingRetryLockTxs(txns);
957+
} while (0);
919958

920959
if (!fMoreWork && !workInterrupt.sleep_for(std::chrono::milliseconds(100))) {
921960
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::CSigningManager;
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>
@@ -47,6 +47,31 @@ CSigningManager::CSigningManager(CChainState& chainstate, llmq::CChainLocksHandl
4747

4848
CSigningManager::~CSigningManager() = default;
4949

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

103-
void CSigningManager::ProcessPendingRetryLockTxs()
128+
void CSigningManager::ProcessPendingRetryLockTxs(const std::vector<CTransactionRef>& retryTxs)
104129
{
105-
const auto retryTxs = WITH_LOCK(m_isman.cs_pendingRetry, return m_isman.pendingRetryTxs);
106-
107-
if (retryTxs.empty()) {
108-
return;
109-
}
110-
111130
if (!m_isman.IsInstantSendEnabled()) {
112131
return;
113132
}
114133

115134
int retryCount = 0;
116-
for (const auto& txid : retryTxs) {
117-
CTransactionRef tx;
135+
for (const auto& tx : retryTxs) {
118136
{
119-
{
120-
LOCK(m_isman.cs_nonLocked);
121-
auto it = m_isman.nonLockedTxs.find(txid);
122-
if (it == m_isman.nonLockedTxs.end()) {
123-
continue;
124-
}
125-
tx = it->second.tx;
126-
}
127-
if (!tx) {
128-
continue;
129-
}
130-
131137
if (LOCK(cs_creating); txToCreatingInstantSendLocks.count(tx->GetHash())) {
132138
// we're already in the middle of locking this one
133139
continue;
@@ -156,8 +162,7 @@ void CSigningManager::ProcessPendingRetryLockTxs()
156162
}
157163

158164
if (retryCount != 0) {
159-
LogPrint(BCLog::INSTANTSEND, "instantsend::CSigningManager::%s -- retried %d TXs. nonLockedTxs.size=%d\n", __func__,
160-
retryCount, WITH_LOCK(m_isman.cs_nonLocked, return m_isman.nonLockedTxs.size()));
165+
LogPrint(BCLog::INSTANTSEND, "instantsend::CSigningManager::%s -- retried %d TXs.\n", __func__, retryCount);
161166
}
162167
}
163168

@@ -243,13 +248,7 @@ void CSigningManager::HandleNewInstantSendLockRecoveredSig(const llmq::CRecovere
243248
}
244249

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

255254
void CSigningManager::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
@@ -6,7 +6,6 @@
66
#define BITCOIN_INSTANTSEND_SIGNING_H
77

88
#include <instantsend/lock.h>
9-
#include <instantsend/instantsend.h>
109
#include <llmq/signing.h>
1110

1211
class CMasternodeSync;
@@ -64,33 +63,40 @@ class CSigningManager : public llmq::CRecoveredSigsListener
6463
CSporkManager& sporkman, CTxMemPool& mempool, const CMasternodeSync& mn_sync);
6564
~CSigningManager();
6665

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

7083
private:
71-
bool CheckCanLock(const CTransaction& tx, bool printDebug, const Consensus::Params& params) const;
72-
bool CheckCanLock(const COutPoint& outpoint, bool printDebug, const uint256& txHash,
73-
const Consensus::Params& params) const;
84+
[[nodiscard]] bool CheckCanLock(const CTransaction& tx, bool printDebug, const Consensus::Params& params) const;
85+
[[nodiscard]] bool CheckCanLock(const COutPoint& outpoint, bool printDebug, const uint256& txHash,
86+
const Consensus::Params& params) const;
7487

7588
void HandleNewInputLockRecoveredSig(const llmq::CRecoveredSig& recoveredSig, const uint256& txid)
7689
EXCLUSIVE_LOCKS_REQUIRED(!cs_creating);
7790
void HandleNewInstantSendLockRecoveredSig(const llmq::CRecoveredSig& recoveredSig)
78-
EXCLUSIVE_LOCKS_REQUIRED(!cs_creating, !m_isman.cs_pendingLocks);
91+
EXCLUSIVE_LOCKS_REQUIRED(!cs_creating);
7992

80-
bool IsInstantSendMempoolSigningEnabled() const;
93+
[[nodiscard]] bool IsInstantSendMempoolSigningEnabled() const;
8194

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

0 commit comments

Comments
 (0)