Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: double lock of deterministicMNManager->cs #5637

Merged
merged 3 commits into from
Oct 23, 2023
Merged

fix: double lock of deterministicMNManager->cs #5637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
75cc716
fix: double lock of deterministicMNManager->cs
knst Oct 21, 2023
4c87168
fixup
knst Oct 22, 2023
8c81985
fix: missing mutex for caches in CheckCbTxMerkleRoots
knst Oct 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions src/evo/cbtx.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,12 +111,10 @@ bool CheckCbTxMerkleRoots(const CBlock& block, const CBlockIndex* pindex, const

bool CalcCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindexPrev, uint256& merkleRootRet, BlockValidationState& state, const CCoinsViewCache& view)
{
LOCK(deterministicMNManager->cs);

try {
static int64_t nTimeDMN = 0;
static int64_t nTimeSMNL = 0;
static int64_t nTimeMerkle = 0;
static std::atomic<int64_t> nTimeDMN = 0;
static std::atomic<int64_t> nTimeSMNL = 0;
static std::atomic<int64_t> nTimeMerkle = 0;

int64_t nTime1 = GetTimeMicros();

Expand All @@ -134,10 +132,12 @@ bool CalcCbTxMerkleRootMNList(const CBlock& block, const CBlockIndex* pindexPrev
int64_t nTime3 = GetTimeMicros(); nTimeSMNL += nTime3 - nTime2;
LogPrint(BCLog::BENCHMARK, " - CSimplifiedMNList: %.2fms [%.2fs]\n", 0.001 * (nTime3 - nTime2), nTimeSMNL * 0.000001);

static CSimplifiedMNList smlCached;
static uint256 merkleRootCached;
static bool mutatedCached{false};
static Mutex cached_mutex;
static CSimplifiedMNList smlCached GUARDED_BY(cached_mutex);
static uint256 merkleRootCached GUARDED_BY(cached_mutex);
static bool mutatedCached GUARDED_BY(cached_mutex) {false};

LOCK(cached_mutex);
if (sml == smlCached) {
merkleRootRet = merkleRootCached;
if (mutatedCached) {
Expand Down
8 changes: 3 additions & 5 deletions src/evo/deterministicmns.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -613,8 +613,6 @@ bool CDeterministicMNManager::ProcessBlock(const CBlock& block, const CBlockInde
int nHeight = pindex->nHeight;

try {
LOCK(cs);

if (!BuildNewListFromBlock(block, pindex->pprev, state, view, newList, true)) {
// pass the state returned by the function above
return false;
Expand All @@ -626,6 +624,8 @@ bool CDeterministicMNManager::ProcessBlock(const CBlock& block, const CBlockInde

newList.SetBlockHash(pindex->GetBlockHash());

LOCK(cs);

oldList = GetListForBlockInternal(pindex->pprev);
diff = oldList.BuildDiff(newList);

Expand Down Expand Up @@ -708,11 +708,9 @@ void CDeterministicMNManager::UpdatedBlockTip(const CBlockIndex* pindex)

bool CDeterministicMNManager::BuildNewListFromBlock(const CBlock& block, const CBlockIndex* pindexPrev, BlockValidationState& state, const CCoinsViewCache& view, CDeterministicMNList& mnListRet, bool debugLogs)
{
AssertLockHeld(cs);

int nHeight = pindexPrev->nHeight + 1;

CDeterministicMNList oldList = GetListForBlockInternal(pindexPrev);
CDeterministicMNList oldList = GetListForBlock(pindexPrev);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related. Internal doesn't do CS, non -internal does, so, it must be changed

CDeterministicMNList newList = oldList;
newList.SetBlockHash(uint256()); // we can't know the final block hash, so better not return a (invalid) block hash
newList.SetHeight(nHeight);
Expand Down
6 changes: 2 additions & 4 deletions src/evo/deterministicmns.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -574,10 +574,8 @@ class CDeterministicMNManager
static constexpr int DISK_SNAPSHOTS = llmq_max_blocks() / DISK_SNAPSHOT_PERIOD + 1;
static constexpr int LIST_DIFFS_CACHE_SIZE = DISK_SNAPSHOT_PERIOD * DISK_SNAPSHOTS;

public:
Mutex cs;

private:
Mutex cs;
knst marked this conversation as resolved.
Show resolved Hide resolved
Mutex cs_cleanup;
// We have performed CleanupCache() on this height.
int did_cleanup GUARDED_BY(cs_cleanup) {0};
Expand Down Expand Up @@ -607,7 +605,7 @@ class CDeterministicMNManager

// the returned list will not contain the correct block hash (we can't know it yet as the coinbase TX is not updated yet)
bool BuildNewListFromBlock(const CBlock& block, const CBlockIndex* pindexPrev, BlockValidationState& state, const CCoinsViewCache& view,
CDeterministicMNList& mnListRet, bool debugLogs) EXCLUSIVE_LOCKS_REQUIRED(cs);
CDeterministicMNList& mnListRet, bool debugLogs) LOCKS_EXCLUDED(cs);
static void HandleQuorumCommitment(const llmq::CFinalCommitment& qc, const CBlockIndex* pQuorumBaseBlockIndex, CDeterministicMNList& mnList, bool debugLogs);

CDeterministicMNList GetListForBlock(const CBlockIndex* pindex) LOCKS_EXCLUDED(cs) {
Expand Down
Loading