wallet: Set m_last_block_processed to nullptr in SetNull() #11749

Prior to this commit the non-static class member m_last_block_processed was not initialized in the CWallet constructor nor in any functions that the constructor calls.

m_last_block_processed was introduced in 5ee31726360cbe343f5a1a50a5e440db736da5b7 (part of PR #10286) which was merged into master a couple of days ago.

Friendly ping @TheBlueMatt :-)

Is the lock really necessary?

@promag m_last_block_processed is guarded by cs_main according to this comment in wallet.h:

https://github.com/bitcoin/bitcoin/blob/3c098a8aa0780009c11b66b1a5d488a928629ebf/src/wallet/wallet.h#L732-L734

IMO it should be guarded by cs_wallet, it's a wallet thing. m_last_block_processed is changed in: https://github.com/bitcoin/bitcoin/blob/3c098a8aa0780009c11b66b1a5d488a928629ebf/src/wallet/wallet.cpp#L1236-L1237

And as you can see both cs_main and cs_wallet are held.

Then, here cs_wallet should be locked too: https://github.com/bitcoin/bitcoin/blob/3c098a8aa0780009c11b66b1a5d488a928629ebf/src/wallet/wallet.cpp#L1272-L1284

cs_main is needed because of chainActive. @TheBlueMatt wdyt?

@promag Sounds reasonable! I'll update once we get @TheBlueMatt's clarification :-)

@promag Would replacing the current commit with the following be in line with your suggestion? :-)

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 29eabd1..7aa7293 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -1272,10 +1272,7 @@ void CWallet::BlockUntilSyncedToCurrentChain() {
     {
         // Skip the queue-draining stuff if we know we're caught up with
         // chainActive.Tip()...
-        // We could also take cs_wallet here, and call m_last_block_processed
-        // protected by cs_wallet instead of cs_main, but as long as we need
-        // cs_main here anyway, its easier to just call it cs_main-protected.
-        LOCK(cs_main);
+        LOCK2(cs_main, cs_wallet);
         const CBlockIndex* initialChainTip = chainActive.Tip();

         if (m_last_block_processed->GetAncestor(initialChainTip->nHeight) == initialChainTip) {
@@ -3970,7 +3970,10 @@ CWallet* CWallet::CreateWalletFromFile(const std::string walletFile) EXCLUSIVE_L
             pindexRescan = FindForkInGlobalIndex(chainActive, locator);
     }

-    walletInstance->m_last_block_processed = chainActive.Tip();
+    {
+        LOCK(walletInstance->cs_wallet);
+        walletInstance->m_last_block_processed = chainActive.Tip();
+    }
     RegisterValidationInterface(walletInstance);

     if (chainActive.Tip() && chainActive.Tip() != pindexRescan)
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index d6fbf7d..2ac3b71 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -731,9 +731,9 @@ private:
      * to have seen all transactions in the chain, but is only used to track
      * live BlockConnected callbacks.
      *
-     * Protected by cs_main (see BlockUntilSyncedToCurrentChain)
+     * Protected by cs_wallet
      */
@@ -798,6 +798,7 @@ public:
             nOrderPosNext = 0;
             nTimeFirstKey = 0;
             nRelockTime = 0;
+            m_last_block_processed = nullptr;
         }
         nMasterKeyMaxID = 0;
         pwalletdbEncryption = nullptr;

Looks good? :-)

Ugh, this is set fine in the place CWallets are created, I agree it should be initialized, but it's not a bug and going back and forth on stuff like this is just a waste of time. Most importantly, this obviously doesn't need a cs_main, is there really some static analyser that wants that there?

On November 22, 2017 7:40:24 AM PST, practicalswift notifications@github.com wrote:

@promag Sounds reasonable! I'll update once we get @TheBlueMatt's clarification :-)

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/11749#issuecomment-346387695

@TheBlueMatt

I agree that fixing static analyzer warnings in mature old code is not worth it, but I think the case could be made that we can be more liberal in accepting fixes for static analyzer warnings introduced by brand new code (such as this - merged only days ago). Sounds reasonable?

I don't have any strong feelings about the initialization fix (beyond the warm fuzzy feeling of seeing the number of static analyzer warnings decrease rather than increase over time), but I feel very strongly about getting the locking requirements annotated correctly :-)

That takes us to the important part discussed in this PR:

The comment …

https://github.com/bitcoin/bitcoin/blob/3c098a8aa0780009c11b66b1a5d488a928629ebf/src/wallet/wallet.h#L732-L734

… suggested that m_last_block_processed is guarded by cs_main, so a corresponding Clang thread analysis annotation was added (in the annotations PR) …

const CBlockIndex* m_last_block_processed GUARDED_BY(cs_main);

… and hence the LOCK(cs_main) in SetNull() (as enforced by Clang's thread safety analysis).

What is the correct locking annotation for m_last_block_processed – is it GUARDED_BY(cs_main) rather than GUARDED_BY(cs_wallet)?

My point was not that we shouldn't accept simple static analysis fixes, but more that we should keep them simple and avoid review burden by making them as minimal as possible and not going back and forth on them. Static analysis shouldn't complain about variables in a constructor, and adding an extern cs_main in wallet.h just sucks - either move SetNull to the .cpp or move the setting to the constructor (does anything even use SetNull?).

On November 22, 2017 12:27:33 PM PST, practicalswift notifications@github.com wrote:

@TheBlueMatt

I agree that fixing static analyzer warnings in mature old code is not worth it, but I think the case could be made that we can be more liberal in accepting fixes for static analyzer warnings introduced by brand new code (such as this - merged only days ago). Sounds reasonable?

I don't have any strong feelings about the initialization fix (beyond the warm fuzzy feeling of seeing the number of static analyzer warnings decrease rather than increase over time), but I feel very strongly about stating all locking requirements explicitly :-)

That takes us to the important part:

The comment …

https://github.com/bitcoin/bitcoin/blob/3c098a8aa0780009c11b66b1a5d488a928629ebf/src/wallet/wallet.h#L732-L734

… suggested that m_last_block_processed is guarded by cs_main, so a corresponding Clang thread analysis annotation was added …

const CBlockIndex* m_last_block_processed GUARDED_BY(cs_main);

… and hence the LOCK(cs_main) in SetNull() (as enforced by Clang's thread safety analysis).

What is the correct locking annotation for m_last_block_processed – is it GUARDED_BY(cs_main) rather than GUARDED_BY(cs_wallet)?

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/11749#issuecomment-346464511

@TheBlueMatt I agree that adding an extern cs_main in wallet.h sucks – the locking annotation GUARDED_BY(cs_main) is obviously incorrect :-)

Is …

const CBlockIndex* m_last_block_processed GUARDED_BY(cs_wallet);

… the correct locking annotation for m_last_block_processed?

I believe that is (currently) correct, though we may be able to change it. I think it's time to think harder about wallet cs_main locking, especially with @ryanosky's work, I believe there's an opportunity to rethink a bunch of it when adding annotations.

Either way, can we avoid ugly patches that result in a bunch of time wasted going back and forth just to fix (incorrect) static analysis warnings?

On November 22, 2017 12:35:24 PM PST, practicalswift notifications@github.com wrote:

@TheBlueMatt I agree that adding an extern cs_main in wallet.h sucks – the locking annotation is obviously incorrect :-)

Is …

const CBlockIndex* m_last_block_processed GUARDED_BY(cs_main);

… the correct locking annotation for m_last_block_processed?

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bitcoin/bitcoin/pull/11749#issuecomment-346466211