Merge #6353: backport: trivial 2024 10 23 pr10

4101fea Merge bitcoin#28304: doc: Remove confusing assert linter (fanquake) c59cb15 Merge bitcoin#26282: wallet: have prune error take precedence over assumedvalid (fanquake) e2e8598 Merge bitcoin#23997: wallet: avoid rescans under assumed-valid blocks (Andrew Chow) b66eebe Merge bitcoin#25599: build: Check for std::atomic::exchange rather than std::atomic_exchange (fanquake) 1204dc0 Merge bitcoin#25486: test: fix failing test `interface_usdt_utxocache.py` (MacroFake) de17997 Merge bitcoin#24062: refactor: replace RecursiveMutex `m_most_recent_block_mutex` with Mutex (MacroFake) c91f010 Merge bitcoin#25092: doc: various developer notes updates (MacroFake) f39fcd1 Merge bitcoin#24988: lint: Mention NONFATAL_UNREACHABLE in lint-assertions.py (fanquake) Pull request description: ## Issue being fixed or feature implemented Batch of trivial backports ## What was done? See commits ## How Has This Been Tested? built locally; large combined merge passed tests locally ## Breaking Changes Should be none ## Checklist: - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK 4101fea kwvg: utACK 4101fea Tree-SHA512: e948ff58b256f2ecb9611681f773570d233985f1470e3eaa6899f3b7e53701c06f56ed5b965d250e22764938b0afebc8d85f92879ba111a0e20127cd63e99809
dashpay · Oct 25, 2024 · bef82c4 · bef82c4
2 parents 3009b86 + 4101fea
commit bef82c4
Show file tree

Hide file tree

Showing 10 changed files with 91 additions and 61 deletions.
diff --git a/build-aux/m4/l_atomic.m4 b/build-aux/m4/l_atomic.m4
@@ -20,7 +20,7 @@ m4_define([_CHECK_ATOMIC_testbody], [[
 
   int main() {
     std::atomic<bool> lock{true};
-    std::atomic_exchange(&lock, false);
+    lock.exchange(false);
 
     std::atomic<std::chrono::seconds> t{0s};
     t.store(2s);
@@ -38,6 +38,8 @@ m4_define([_CHECK_ATOMIC_testbody], [[
 AC_DEFUN([CHECK_ATOMIC], [
 
   AC_LANG_PUSH(C++)
+  TEMP_CXXFLAGS="$CXXFLAGS"
+  CXXFLAGS="$CXXFLAGS $PTHREAD_CFLAGS"
 
   AC_MSG_CHECKING([whether std::atomic can be used without link library])
 
@@ -55,5 +57,6 @@ AC_DEFUN([CHECK_ATOMIC], [
         ])
     ])
 
+  CXXFLAGS="$TEMP_CXXFLAGS"
   AC_LANG_POP
 ])
diff --git a/configure.ac b/configure.ac
@@ -79,9 +79,6 @@ else
   AX_CXX_COMPILE_STDCXX([20], [noext], [mandatory])
 fi
 
-dnl Check if -latomic is required for <std::atomic>
-CHECK_ATOMIC
-
 dnl check if additional link flags are required for std::filesystem
 CHECK_FILESYSTEM
 
@@ -914,6 +911,9 @@ AC_C_BIGENDIAN
 dnl Check for pthread compile/link requirements
 AX_PTHREAD
 
+dnl Check if -latomic is required for <std::atomic>
+CHECK_ATOMIC
+
 dnl The following macro will add the necessary defines to bitcoin-config.h, but
 dnl they also need to be passed down to any subprojects. Pull the results out of
 dnl the cache and add them to CPPFLAGS.

diff --git a/doc/developer-notes.md b/doc/developer-notes.md
@@ -31,6 +31,7 @@ Developer Notes
     - [C++ data structures](#c-data-structures)
     - [Strings and formatting](#strings-and-formatting)
     - [Shadowing](#shadowing)
+    - [Lifetimebound](#lifetimebound)
     - [Threads and synchronization](#threads-and-synchronization)
     - [Scripts](#scripts)
         - [Shebang](#shebang)
@@ -95,7 +96,10 @@ code.
     Guidelines](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Renum-caps),
     which recommend using `snake_case`.  Please use what seems appropriate.
   - Class names, function names, and method names are UpperCamelCase
-    (PascalCase). Do not prefix class names with `C`.
+    (PascalCase). Do not prefix class names with `C`. See [Internal interface
+    naming style](#internal-interface-naming-style) for an exception to this
+    convention.
+
   - Test suite naming convention: The Boost test suite in file
     `src/test/foo_tests.cpp` should be named `foo_tests`. Test suite names
     must be unique.
@@ -160,6 +164,27 @@ public:
 } // namespace foo
 ```
 
+Coding Style (C++ functions and methods)
+--------------------
+
+- When ordering function parameters, place input parameters first, then any
+  in-out parameters, followed by any output parameters.
+
+- *Rationale*: API consistency.
+
+- Prefer returning values directly to using in-out or output parameters. Use
+  `std::optional` where helpful for returning values.
+
+- *Rationale*: Less error-prone (no need for assumptions about what the output
+  is initialized to on failure), easier to read, and often the same or better
+  performance.
+
+- Generally, use `std::optional` to represent optional by-value inputs (and
+  instead of a magic default value, if there is no real default). Non-optional
+  input parameters should usually be values or const references, while
+  non-optional in-out and output parameters should usually be references, as
+  they cannot be null.
+
 Coding Style (C++ named arguments)
 ------------------------------
 
@@ -695,10 +720,6 @@ Wallet
 
 - Make sure that no crashes happen with run-time option `-disablewallet`.
 
-- Include `db_cxx.h` (BerkeleyDB header) only when `ENABLE_WALLET` is set.
-
-  - *Rationale*: Otherwise compilation of the disable-wallet build will fail in environments without BerkeleyDB.
-
 General C++
 -------------
 
@@ -710,12 +731,6 @@ Common misconceptions are clarified in those sections:
 - Passing (non-)fundamental types in the [C++ Core
   Guideline](https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#Rf-conventional).
 
-- Assertions should not have side-effects.
-
-  - *Rationale*: Even though the source code is set to refuse to compile
-    with assertions disabled, having side-effects in assertions is unexpected and
-    makes the code harder to understand.
-
 - If you use the `.h`, you must link the `.cpp`.
 
   - *Rationale*: Include files define the interface for the code in implementation files. Including one but
@@ -908,12 +923,22 @@ from using a different variable with the same name),
 please name variables so that their names do not shadow variables defined in the source code.
 
 When using nested cycles, do not name the inner cycle variable the same as in
-the upper cycle, etc.
+the outer cycle, etc.
+
+Lifetimebound
+--------------
+
+The [Clang `lifetimebound`
+attribute](https://clang.llvm.org/docs/AttributeReference.html#lifetimebound)
+can be used to tell the compiler that a lifetime is bound to an object and
+potentially see a compile-time warning if the object has a shorter lifetime from
+the invalid use of a temporary. You can use the attribute by adding a `LIFETIMEBOUND`
+annotation defined in `src/attributes.h`; please grep the codebase for examples.
 
 Threads and synchronization
 ----------------------------
 
-- Prefer `Mutex` type to `RecursiveMutex` one
+- Prefer `Mutex` type to `RecursiveMutex` one.
 
 - Consistently use [Clang Thread Safety Analysis](https://clang.llvm.org/docs/ThreadSafetyAnalysis.html) annotations to
   get compile-time warnings about potential race conditions in code. Combine annotations in function declarations with
@@ -986,6 +1011,8 @@ TRY_LOCK(cs_vNodes, lockNodes);
 Scripts
 --------------------------
 
+Write scripts in Python rather than bash, when possible.
+
 ### Shebang
 
 - Use `#!/usr/bin/env bash` instead of obsolete `#!/bin/bash`.
@@ -1438,22 +1465,9 @@ communication:
   virtual boost::signals2::scoped_connection connectTipChanged(TipChangedFn fn) = 0;
   ```
 
-- For consistency and friendliness to code generation tools, interface method
-  input and inout parameters should be ordered first and output parameters
-  should come last.
-
-  Example:
-
-  ```c++
-  // Good: error output param is last
-  virtual bool broadcastTransaction(const CTransactionRef& tx, CAmount max_fee, std::string& error) = 0;
-
-  // Bad: error output param is between input params
-  virtual bool broadcastTransaction(const CTransactionRef& tx, std::string& error, CAmount max_fee) = 0;
-  ```
+- Interface methods should not be overloaded.
 
-- For friendliness to code generation tools, interface methods should not be
-  overloaded:
+  *Rationale*: consistency and friendliness to code generation tools.
 
   Example:
 
@@ -1467,10 +1481,13 @@ communication:
   virtual bool disconnect(NodeId id) = 0;
   ```
 
-- For consistency and friendliness to code generation tools, interface method
-  names should be `lowerCamelCase` and standalone function names should be
+### Internal interface naming style
+
+- Interface method names should be `lowerCamelCase` and standalone function names should be
   `UpperCamelCase`.
 
+  *Rationale*: consistency and friendliness to code generation tools.
+
   Examples:
 
   ```c++

diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
@@ -307,6 +307,9 @@ class Chain
     //! to be prepared to handle this by ignoring notifications about unknown
     //! removed transactions and already added new transactions.
     virtual void requestMempoolTransactions(Notifications& notifications) = 0;
+
+    //! Return true if an assumed-valid chain is in use.
+    virtual bool hasAssumedValidChain() = 0;
 };
 
 //! Interface to let node manage chain clients (wallets, or maybe tools for

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
@@ -906,7 +906,7 @@ class PeerManagerImpl final : public PeerManager
 
 
     // All of the following cache a recent block, and are protected by m_most_recent_block_mutex
-    RecursiveMutex m_most_recent_block_mutex;
+    Mutex m_most_recent_block_mutex;
     std::shared_ptr<const CBlock> m_most_recent_block GUARDED_BY(m_most_recent_block_mutex);
     std::shared_ptr<const CBlockHeaderAndShortTxIDs> m_most_recent_compact_block GUARDED_BY(m_most_recent_block_mutex);
     uint256 m_most_recent_block_hash GUARDED_BY(m_most_recent_block_mutex);
@@ -5676,15 +5676,16 @@ bool PeerManagerImpl::SendMessages(CNode* pto)
                     LogPrint(BCLog::NET, "%s sending header-and-ids %s to peer=%d\n", __func__,
                             vHeaders.front().GetHash().ToString(), pto->GetId());
 
-                    bool fGotBlockFromCache = false;
+                    std::optional<CSerializedNetMsg> cached_cmpctblock_msg;
                     {
                         LOCK(m_most_recent_block_mutex);
                         if (m_most_recent_block_hash == pBestIndex->GetBlockHash()) {
-                            m_connman.PushMessage(pto, msgMaker.Make(NetMsgType::CMPCTBLOCK, *m_most_recent_compact_block));
-                            fGotBlockFromCache = true;
+                            cached_cmpctblock_msg = msgMaker.Make(NetMsgType::CMPCTBLOCK, *m_most_recent_compact_block);
                         }
                     }
-                    if (!fGotBlockFromCache) {
+                    if (cached_cmpctblock_msg.has_value()) {
+                        m_connman.PushMessage(pto, std::move(cached_cmpctblock_msg.value()));
+                    } else {
                         CBlock block;
                         bool ret = ReadBlockFromDisk(block, pBestIndex, consensusParams);
                         assert(ret);

diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
@@ -1008,6 +1008,11 @@ class ChainImpl : public Chain
             notifications.transactionAddedToMempool(entry.GetSharedTx(), /* nAcceptTime = */ 0, /* mempool_sequence = */ 0);
         }
     }
+    bool hasAssumedValidChain() override
+    {
+        return Assert(m_node.chainman)->IsSnapshotActive();
+    }
+
     NodeContext& m_node;
 };
 } // namespace

diff --git a/src/util/check.h b/src/util/check.h
@@ -30,7 +30,6 @@ T&& inline_check_non_fatal(T&& val, const char* file, int line, const char* func
         throw NonFatalCheckError(
             format_internal_error(assertion, file, line, func, PACKAGE_BUGREPORT));
     }
-
     return std::forward<T>(val);
 }
 

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
@@ -5034,20 +5034,33 @@ bool CWallet::AttachChain(const std::shared_ptr<CWallet>& walletInstance, interf
 
     if (tip_height && *tip_height != rescan_height)
     {
-        if (chain.havePruned()) {
+        // Technically we could execute the code below in any case, but performing the
+        // `while` loop below can make startup very slow, so only check blocks on disk
+        // if necessary.
+        if (chain.havePruned() || chain.hasAssumedValidChain()) {
             int block_height = *tip_height;
             while (block_height > 0 && chain.haveBlockOnDisk(block_height - 1) && rescan_height != block_height) {
                 --block_height;
             }
 
             if (rescan_height != block_height) {
-                // We can't rescan beyond non-pruned blocks, stop and throw an error.
+                // We can't rescan beyond blocks we don't have data for, stop and throw an error.
                 // This might happen if a user uses an old wallet within a pruned node
                 // or if they ran -disablewallet for a longer time, then decided to re-enable
                 // Exit early and print an error.
+                // It also may happen if an assumed-valid chain is in use and therefore not
+                // all block data is available.
                 // If a block is pruned after this check, we will load the wallet,
                 // but fail the rescan with a generic error.
-                error = _("Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of pruned node)");
+
+                error = chain.havePruned() ?
+                     _("Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of pruned node)") :
+                     strprintf(_(
+                        "Error loading wallet. Wallet requires blocks to be downloaded, "
+                        "and software does not currently support loading wallets while "
+                        "blocks are being downloaded out of order when using assumeutxo "
+                        "snapshots. Wallet should be able to load successfully after "
+                        "node sync reaches height %s"), block_height);
                 return false;
             }
         }

diff --git a/test/functional/interface_usdt_utxocache.py b/test/functional/interface_usdt_utxocache.py
@@ -170,8 +170,7 @@ def test_uncache(self):
 
         # Create a transaction and invalidate it by changing the txid of the previous
         # output to the coinbase txid of the block at height 1.
-        invalid_tx = self.wallet.create_self_transfer(
-            from_node=self.nodes[0])["tx"]
+        invalid_tx = self.wallet.create_self_transfer()["tx"]
         invalid_tx.vin[0].prevout.hash = int(block_1_coinbase_txid, 16)
 
         self.log.info("hooking into the utxocache:uncache tracepoint")

diff --git a/test/lint/lint-assertions.py b/test/lint/lint-assertions.py
@@ -23,27 +23,17 @@ def git_grep(params: [], error_msg: ""):
 
 
 def main():
-    # PRE31-C (SEI CERT C Coding Standard):
-    # "Assertions should not contain assignments, increment, or decrement operators."
-    exit_code = git_grep([
-        "-E",
-        r"[^_]assert\(.*(\+\+|\-\-|[^=!<>]=[^=!<>]).*\);",
-        "--",
-        "*.cpp",
-        "*.h"
-    ], "Assertions should not have side effects:")
-
-    # Macro CHECK_NONFATAL(condition) should be used instead of assert for RPC code, where it
-    # is undesirable to crash the whole program. See: src/util/check.h
+    # Aborting the whole process is undesirable for RPC code. So nonfatal
+    # checks should be used over assert. See: src/util/check.h
     # src/rpc/server.cpp is excluded from this check since it's mostly meta-code.
-    exit_code |= git_grep([
+    exit_code = git_grep([
         "-nE",
-        r"\<(A|a)ssert *\(.*\);",
+        r"\<(A|a)ss(ume|ert) *\(.*\);",
         "--",
         "src/rpc/",
         "src/wallet/rpc*",
-        ":(exclude)src/rpc/server.cpp"
-    ], "CHECK_NONFATAL(condition) should be used instead of assert for RPC code.")
+        ":(exclude)src/rpc/server.cpp",
+    ], "CHECK_NONFATAL(condition) or NONFATAL_UNREACHABLE should be used instead of assert for RPC code.")
 
     sys.exit(exit_code)
-Original file line number
+Diff line change
@@ Expand Up @@
             throw NonFatalCheckError(
                 format_internal_error(assertion, file, line, func, PACKAGE_BUGREPORT));
         }
         return std::forward<T>(val);
     }
@@ Expand Down @@