[Masternodes] Missing cs main locks in CalculateScore and GetMasternodeInputAge #1791

furszy · 2020-08-07T00:20:29Z

In CalculateScore, GetMasternodeInputAge and GetBlockHash we had chainActive accessed without cs_main being held. This PR fixes it.
No functional changes. Can be added to 4.2.1 back ports list as well.

…without cs_main fix.

random-zebra

Looking good.
Few non-blocking nits and redundant checks (unrelated, can be addressed in a followup PR).

Note: for future refactoring. In places like GetBlockHash we duplicate the implementation of the function GetChainTip() (defined and used in the miner).
We should move it to a more accessible file, and not duplicate those lines every time we need the tip index.

random-zebra · 2020-08-07T09:53:56Z

src/masternode.cpp


    if (mapCacheBlockHashes.count(nBlockHeight)) {
        hash = mapCacheBlockHashes[nBlockHeight];
        return true;
    }

-    const CBlockIndex* BlockLastSolved = chainActive.Tip();
-    const CBlockIndex* BlockReading = chainActive.Tip();
+    const CBlockIndex* BlockLastSolved = tipIndex;


Do we really need another variable for this?
Can't we directly define BlockLastSolved at the start (inside the cs_main block), instead of tipIndex?

yep, not needed at all, same as for the other variable BlockReading that can be removed.

I just didn't want to modify much code around this area for now (it needs its own focused PR). Just made the needed changes to synchronize chainActive access .

random-zebra · 2020-08-07T09:56:54Z

src/masternode.cpp


-    if (BlockLastSolved == NULL || BlockLastSolved->nHeight == 0 || chainActive.Tip()->nHeight + 1 < nBlockHeight) return false;
+    if (BlockLastSolved == nullptr || BlockLastSolved->nHeight == 0 || tipHeight + 1 < nBlockHeight) return false;


Here BlockLastSolved is just tipindex. If it is null, we've already returned (line 31).

yep. Not part of this PR focus. Can be tackled in another one.

random-zebra · 2020-08-07T10:04:46Z

src/masternode.cpp

-    if (chainActive.Tip() == NULL) return UINT256_ZERO;
+    {
+        LOCK(cs_main);
+        if (chainActive.Tip() == NULL) return UINT256_ZERO;


nit: use nullptr instead of NULL (or !chainActive.Tip()).

random-zebra · 2020-08-07T10:10:39Z

src/masternode.cpp

    int nBlocksAgo = 0;
-    if (nBlockHeight > 0) nBlocksAgo = (chainActive.Tip()->nHeight + 1) - nBlockHeight;
+    if (nBlockHeight > 0) nBlocksAgo = (tipHeight + 1) - nBlockHeight;
    assert(nBlocksAgo >= 0);


I know that this was already like that, and this PR is not changing the behaviour, but, since we are at it, why this assert?

nBlocksAgo < 0

iff

(tipHeight + 1) < nBlockHeight

in which case we've already returned (line 47)

random-zebra · 2020-08-07T10:14:32Z

src/masternode.cpp

+        CBlockIndex *pindex = chainActive.Tip();
+        if (!pindex) return false;
+        tipHeight = pindex->nHeight;
+        tipIndex = mapBlockIndex[pindex->GetBlockHash()];


nit: better to use mapBlockIndex.at(), which throws an exception if (for whatever reason) the key is not found.

random-zebra · 2020-08-07T10:37:51Z

src/masternode.cpp

+        LOCK(cs_main);
+        CBlockIndex *pindex = chainActive.Tip();
+        if (!pindex) return false;
+        tipHeight = pindex->nHeight;


nit: we can initialize this after the LOCK code-block, as

int tipHeight = tipIndex->nHeight

(...or just remove it, and use tipIndex->nHeight directly).

Fuzzbawls

utACK a14aba3

random-zebra · 2020-08-13T00:15:35Z

Merging...

Github-Pull: PIVX-Project#1791 Rebased-From: 224c8fd

…without cs_main fix. Github-Pull: PIVX-Project#1791 Rebased-From: a14aba3

dc0ed71 [BUG][GUI] Don't append cold-stake records twice (random-zebra) 91adcd7 masternode: CalculateScore and GetBlockHash accessing to chainActive without cs_main fix. (furszy) eeb112f GetMasternodeInputAge: Missing cs_main lock (furszy) 52ec12d refactor: decouple decompose coinstake from TransactionRecord::decomposeTransaction. (furszy) 51a8389 [BUG] Properly copy fCoinStake memeber between CTxInUndo and CCoins Github-Pull: #1796 Rebased-From: acf757b (random-zebra) 5c3caa4 lock cs_main for Misbehaving (furszy) 5c629d8 openssl.org dependency download link changed (CryptoDev-Project) 11aa63c Do not try to resend transactions if the node is importing or in IBD. (furszy) c59b62e [Core] Remove BIP30 check (random-zebra) 3f05eba include missing atomic to make CMake linux happy. (furszy) f0cfd88 Make the cs_sendProcessing a LOCK instead of a TRY_LOCK (Matt Corallo) d38e28c Split CNode::cs_vSend: message processing and message sending (Matt Corallo) 7561b18 Remove double brackets in addrman (Matt Corallo) 63c629b Fix AddrMan locking (Matt Corallo) f78cec4 Make fDisconnect an std::atomic (Matt Corallo) ec56cf5 net: fix a few cases where messages were sent rather than dropped upon disconnection (furszy) 7697085 Acquire cs_main lock before cs_wallet during wallet initialization (random-zebra) c796593 Remove bogus assert on number of oubound connections. (Matt Corallo) 7521065 wallet: Ignore MarkConflict if block hash is not known (random-zebra) 72042ac Move disconnectBlocks logging to use __func__ instead of hardcoded method name. (furszy) 0f66764 Simplify DisconnectBlock arguments/return value (furszy) cca4a8c Split logic to undo txin's off DisconnectBlock (furszy) 0f883e6 Cleaner disconnectBlock flow for coinbase and zerocoin txs. (furszy) a7cbc46 Move UndoWriteToDisk() and UndoReadFromDisk() to anon namespace (random-zebra) de5a405 Decouple CBlockUndo from CDiskBlockPos (jtimon) 671143c Decouple miner.o and txmempool.o from CTxUndo (random-zebra) 9419228 Decouple CCoins from CTxInUndo (jtimon) Pull request description: Backports the following PRs to the `4.2` branch: #1746 #1735 #1767 #1769 #1781 #1780 #1775 #1783 #1750 #1785 #1796 #1776 #1791 ACKs for top commit: furszy: Added #1805. utACK dc0ed71. random-zebra: utACK dc0ed71 Tree-SHA512: 0e59c9e751597b1b6f9a419e117946cec468dffdb921c882a44e5770ecb1a36b7d3d25f8c7f97d48bb3d59f136492842c08969901512d957a17bfaec6aece449

def064f [Refactor] Cleanup usage of chainActive.{Tip(),Height()} in a few places (random-zebra) 026dcc9 [Refactor] Move GetChainTip from miner to main (random-zebra) Pull request description: Refactor a few places in masternode/rpc code where `chainActive.Tip()` or `chainActive.Height()` is used. Make use of the function `GetChainTip()`, moved from miner to main, which returns a more reliable pointer (from mapBlockIndex, instead of chainActive). Remove redundant checks in masternode's `GetBlockHash` (e.g. see comments in #1791). ACKs for top commit: furszy: re-re-ACK def064f. Tree-SHA512: 03a4e50b3846ee3f91fd529ebe7d629400c553c64ff033fe40901f96bc106dba237bf344e7c4ac0092cfd206f7c7ea3de003a91e8f82e27a6bd416923d006f90

furszy added 2 commits August 6, 2020 21:06

GetMasternodeInputAge: Missing cs_main lock

224c8fd

masternode: CalculateScore and GetBlockHash accessing to chainActive …

a14aba3

…without cs_main fix.

furszy self-assigned this Aug 7, 2020

random-zebra approved these changes Aug 7, 2020

View reviewed changes

Fuzzbawls added this to the 5.0.0 milestone Aug 12, 2020

Fuzzbawls approved these changes Aug 12, 2020

View reviewed changes

random-zebra merged commit 6162df9 into PIVX-Project:master Aug 13, 2020

Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request Aug 13, 2020

GetMasternodeInputAge: Missing cs_main lock

eeb112f

Github-Pull: PIVX-Project#1791 Rebased-From: 224c8fd

Fuzzbawls pushed a commit to Fuzzbawls/PIVX that referenced this pull request Aug 13, 2020

masternode: CalculateScore and GetBlockHash accessing to chainActive …

91adcd7

…without cs_main fix. Github-Pull: PIVX-Project#1791 Rebased-From: a14aba3

random-zebra mentioned this pull request Aug 13, 2020

[Refactor] Cleanup access to chainActive in a few places #1806

Merged

Fuzzbawls mentioned this pull request Aug 13, 2020

[Backport] 4.2.1 backports #1807

Merged

random-zebra modified the milestones: 5.0.0, 4.3.0 Sep 10, 2020

furszy deleted the 2020_some_missing_cs_main_locks branch November 29, 2022 14:27

lyricidal mentioned this pull request Dec 19, 2022

[Upstream] [Masternodes] Missing cs main locks in CalculateScore and GetMasternodeInputAge PRCYCoin/PRCYCoin#313

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Masternodes] Missing cs main locks in CalculateScore and GetMasternodeInputAge #1791

[Masternodes] Missing cs main locks in CalculateScore and GetMasternodeInputAge #1791

furszy commented Aug 7, 2020

random-zebra left a comment

random-zebra Aug 7, 2020

furszy Aug 7, 2020

random-zebra Aug 7, 2020

furszy Aug 7, 2020

random-zebra Aug 7, 2020

random-zebra Aug 7, 2020

random-zebra Aug 7, 2020

random-zebra Aug 7, 2020

Fuzzbawls left a comment

random-zebra commented Aug 13, 2020


		if (BlockLastSolved == NULL \|\| BlockLastSolved->nHeight == 0 \|\| chainActive.Tip()->nHeight + 1 < nBlockHeight) return false;
		if (BlockLastSolved == nullptr \|\| BlockLastSolved->nHeight == 0 \|\| tipHeight + 1 < nBlockHeight) return false;

[Masternodes] Missing cs main locks in CalculateScore and GetMasternodeInputAge #1791

[Masternodes] Missing cs main locks in CalculateScore and GetMasternodeInputAge #1791

Conversation

furszy commented Aug 7, 2020

random-zebra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fuzzbawls left a comment

Choose a reason for hiding this comment

random-zebra commented Aug 13, 2020