From f39fcd1402a60c249635a812a0b1ebbfaa7647b6 Mon Sep 17 00:00:00 2001 From: fanquake Date: Thu, 28 Apr 2022 12:40:22 +0100 Subject: [PATCH 1/8] Merge bitcoin/bitcoin#24988: lint: Mention NONFATAL_UNREACHABLE in lint-assertions.py fa82a1ed833fd749849fa19267207b63e338d84d lint: Mention NONFATAL_UNREACHABLE in lint-assertions.py (MacroFake) Pull request description: Follow up to commit b1c5991eebb916755be188f355ad36fe01a3f529. Also remove empty newline added in that commit. ACKs for top commit: fanquake: ACK fa82a1ed833fd749849fa19267207b63e338d84d Tree-SHA512: cf398eceb135672137183bfa19ee57a82553a3dbcbce74db954c6fcd79f9606092cc0d8217610fe6cd67b7ef2d4f01d90329f0f568516d9b14aa2cd0f0715478 --- src/util/check.h | 1 - test/lint/lint-assertions.py | 12 ++++++------ 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/src/util/check.h b/src/util/check.h index 247d89bdb6e3f..da257f07231c4 100644 --- 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(val); } diff --git a/test/lint/lint-assertions.py b/test/lint/lint-assertions.py index a8d2b3927cfb4..195ff33d1127d 100755 --- a/test/lint/lint-assertions.py +++ b/test/lint/lint-assertions.py @@ -30,20 +30,20 @@ def main(): r"[^_]assert\(.*(\+\+|\-\-|[^=!<>]=[^=!<>]).*\);", "--", "*.cpp", - "*.h" + "*.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([ "-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) From c91f010e0edbeae254d85041981d5f4de23ee2b0 Mon Sep 17 00:00:00 2001 From: MacroFake Date: Mon, 16 May 2022 10:38:25 +0200 Subject: [PATCH 2/8] Merge bitcoin/bitcoin#25092: doc: various developer notes updates 654284209f30fd309c326a527cda1d2d7385cee8 Add clang lifetimebound section to developer notes (Jon Atack) e66b321fd1ddfffd9bfc59d407ad8f03490b873c Add C++ functions and methods section to developer notes (Jon Atack) 5fca70f5b16fee4a732a1d7fd3fb1c7e775decdf Link in developer notes style to internal interface exception (Jon Atack) fc4cb857ccfa622e76f0f8e7aa164ca4d8bd599a Prefer Python for scripts in developer notes (Jon Atack) 370120ec2ff4b5e7d5cd6678a7be7cfe651be509 Remove obsolete BDB ENABLE_WALLET section in developer notes (Jon Atack) Pull request description: A few updates noticed while working on a lifetimebound section. - Remove obsolete BDB ENABLE_WALLET section (only one file, src/wallet/bdb.h, still has a `db_cxx.h` BDB header) - Prefer Python for scripts in developer notes (and a few miscellaneous touch-ups) - In the code style section, add a link to the internal interface exception so that people are aware of it - Add a "C++ functions and methods" section - Add a Clang `lifetimebound` attribute section ACKs for top commit: laanwj: ACK 654284209f30fd309c326a527cda1d2d7385cee8 jarolrod: code review ACK https://github.com/bitcoin/bitcoin/commit/654284209f30fd309c326a527cda1d2d7385cee8 Tree-SHA512: d2ae335cac899451d5c7bdc8e94fd82053a5f44ac1ba444bdde71abaaa24a519713c1078f3a8f07ec8466b181788a613fd3c68061e54b3fdc8cd6f3e3f9791ec --- doc/developer-notes.md | 71 ++++++++++++++++++++++++++++-------------- 1 file changed, 47 insertions(+), 24 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 162547f1dd971..5f5bcc129ecfe 100644 --- 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) ------------------------------ @@ -681,10 +706,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++ ------------- @@ -894,12 +915,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 @@ -972,6 +1003,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`. @@ -1421,22 +1454,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. +- Interface methods should not be overloaded. - 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; - ``` - -- For friendliness to code generation tools, interface methods should not be - overloaded: + *Rationale*: consistency and friendliness to code generation tools. Example: @@ -1450,10 +1470,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++ From de17997621cce2784af63dd95b2027884d58fcf1 Mon Sep 17 00:00:00 2001 From: MacroFake Date: Tue, 17 May 2022 08:44:06 +0200 Subject: [PATCH 3/8] Merge bitcoin/bitcoin#24062: refactor: replace RecursiveMutex `m_most_recent_block_mutex` with Mutex 83003ffe049a432f6fa4127e054f073127e70b90 refactor: replace RecursiveMutex `m_most_recent_block_mutex` with Mutex (Sebastian Falbesoner) 8edd0d31ac683378135a9839e5d4172b82f8f5b8 refactor: reduce scope of lock `m_most_recent_block_mutex` (Sebastian Falbesoner) Pull request description: This PR is related to #19303 and gets rid of the RecursiveMutex `m_most_recent_block_mutex`. All of the critical sections (5 in total) only directly access the guarded elements, i.e. it is not possible that within one section another one is called, and we can use a regular Mutex: https://github.com/bitcoin/bitcoin/blob/b019cdc036343a437fd7ced85467bd95f48d84c4/src/net_processing.cpp#L1650-L1655 https://github.com/bitcoin/bitcoin/blob/b019cdc036343a437fd7ced85467bd95f48d84c4/src/net_processing.cpp#L1861-L1865 https://github.com/bitcoin/bitcoin/blob/b019cdc036343a437fd7ced85467bd95f48d84c4/src/net_processing.cpp#L3149-L3152 https://github.com/bitcoin/bitcoin/blob/b019cdc036343a437fd7ced85467bd95f48d84c4/src/net_processing.cpp#L3201-L3206 https://github.com/bitcoin/bitcoin/blob/b019cdc036343a437fd7ced85467bd95f48d84c4/src/net_processing.cpp#L4763-L4769 The scope of the last critical section is reduced in the first commit, in order to avoid calling the non-trivial method `CConnman::PushMessage` while the lock is held. ACKs for top commit: furszy: Code ACK 83003ffe with a small comment. hebasto: ACK 83003ffe049a432f6fa4127e054f073127e70b90 w0xlt: ACK https://github.com/bitcoin/bitcoin/pull/24062/commits/83003ffe049a432f6fa4127e054f073127e70b90 Tree-SHA512: 3df290cafd2f6c4d40afb9f14e822a77d9c1828e66f5e2233f3ac1deccc2b0a8290bc5fb8eb992f49d39e887b50bc0e9aad63e05db2d870791a8d409fb95695f --- src/net_processing.cpp | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/net_processing.cpp b/src/net_processing.cpp index e0af64376bdbe..f47d6b65f421e 100644 --- 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 m_most_recent_block GUARDED_BY(m_most_recent_block_mutex); std::shared_ptr 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 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); From 1204dc0f833b04fc6e17281750218b8d62a6d290 Mon Sep 17 00:00:00 2001 From: MacroFake Date: Tue, 28 Jun 2022 08:31:16 +0200 Subject: [PATCH 4/8] Merge bitcoin/bitcoin#25486: test: fix failing test `interface_usdt_utxocache.py` f665c6ecda854adaaa629e961b1912847b42fd34 test: fix failing test interface_usdt_utxocache.py (Sebastian Falbesoner) Pull request description: The `from_node` argument doesn't exist anymore for `MiniWallet.create_self_transfer` since PR #25435 (commit fa8421bc5bcd483a9501257073db17ff2e76eb46), leading to an error on master: ``` $ sudo ./test/functional/interface_usdt_utxocache.py 2022-06-27T17:45:35.585000Z TestFramework (INFO): Initializing test directory /tmp/bitcoin_func_test_7s1djjo1 2022-06-27T17:45:36.515000Z TestFramework (INFO): testing the utxocache:uncache tracepoint API 2022-06-27T17:45:36.517000Z TestFramework (ERROR): Unexpected exception caught during testing Traceback (most recent call last): File "/home/honeybadger/bitcoin/test/functional/test_framework/test_framework.py", line 133, in main self.run_test() File "/home/honeybadger/bitcoin/./test/functional/interface_usdt_utxocache.py", line 149, in run_test self.test_uncache() File "/home/honeybadger/bitcoin/./test/functional/interface_usdt_utxocache.py", line 172, in test_uncache invalid_tx = self.wallet.create_self_transfer( TypeError: create_self_transfer() got an unexpected keyword argument 'from_node' 2022-06-27T17:45:36.568000Z TestFramework (INFO): Stopping nodes [...] ``` Fix this by removing the argument. (Unfortunately, the USDT tests don't seem to run on any CI target, I guess that's due to missing permissions to hook into the kernel.) ACKs for top commit: MarcoFalke: cr ACK f665c6ecda854adaaa629e961b1912847b42fd34 Tree-SHA512: 74f8e398739a25ab5518ff71b998d03d4e529a786ba5b424509de81a511ad3e2e1cd38a5b7bb9f1f5a21340391d6807f4951ff39fa3a2ad65a3b11b989eebea6 --- test/functional/interface_usdt_utxocache.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/test/functional/interface_usdt_utxocache.py b/test/functional/interface_usdt_utxocache.py index 9ae1cba1e5868..13e8d165c3ddc 100755 --- 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") From b66eebe64d20eab262f715748c8df11a8786f72c Mon Sep 17 00:00:00 2001 From: fanquake Date: Mon, 18 Jul 2022 16:31:44 +0100 Subject: [PATCH 5/8] Merge bitcoin/bitcoin#25599: build: Check for std::atomic::exchange rather than std::atomic_exchange 4de4221ab4b645ff77503c777c9b195a962e9fa1 build: Check for std::atomic::exchange rather than std::atomic_exchange (Andrew Chow) Pull request description: Our usage of std::atomic is with it's own exchange function, not std::atomic_exchange. So we should be looking specifically for that function. This removes the need for -latomic for riscv builds, which resolves a guix cross architecture reproducibility issue. ACKs for top commit: hebasto: ACK 4de4221ab4b645ff77503c777c9b195a962e9fa1 fanquake: ACK 4de4221ab4b645ff77503c777c9b195a962e9fa1 Tree-SHA512: dd8225fc9c6a335601f611700003d0249b9ef941efa502db39306129677929d013048e9221be1d6d7f0ea2d90313d4b87de239f441be21b25bea40a6c19a031e --- build-aux/m4/l_atomic.m4 | 5 ++++- configure.ac | 6 +++--- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/build-aux/m4/l_atomic.m4 b/build-aux/m4/l_atomic.m4 index 40639dfe618e5..602b57fe43263 100644 --- a/build-aux/m4/l_atomic.m4 +++ b/build-aux/m4/l_atomic.m4 @@ -18,7 +18,7 @@ m4_define([_CHECK_ATOMIC_testbody], [[ int main() { std::atomic lock{true}; - std::atomic_exchange(&lock, false); + lock.exchange(false); std::atomic t{0s}; t.store(2s); @@ -34,6 +34,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]) @@ -51,5 +53,6 @@ AC_DEFUN([CHECK_ATOMIC], [ ]) ]) + CXXFLAGS="$TEMP_CXXFLAGS" AC_LANG_POP ]) diff --git a/configure.ac b/configure.ac index 929d264229696..ca3bf95798e59 100644 --- 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 -CHECK_ATOMIC - dnl check if additional link flags are required for std::filesystem CHECK_FILESYSTEM @@ -896,6 +893,9 @@ AC_C_BIGENDIAN dnl Check for pthread compile/link requirements AX_PTHREAD +dnl Check if -latomic is required for +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. From e2e8598c5a8b92e2ca9a21e39fd0a7c017010937 Mon Sep 17 00:00:00 2001 From: Andrew Chow Date: Mon, 18 Jul 2022 14:27:49 -0400 Subject: [PATCH 6/8] Merge bitcoin/bitcoin#23997: wallet: avoid rescans under assumed-valid blocks 817326a828d6148dc63d9ef08f641b9c0c522411 wallet: avoid rescans if under the snapshot (James O'Beirne) Pull request description: This is part of the [assumeutxo project](https://github.com/bitcoin/bitcoin/projects/11) (parent PR: #15606) --- Refuse to load a wallet if it requires a rescan lower than the height of assumed-valid blocks. Of course in live code right now, `BLOCK_ASSUMED_VALID` block index entries don't exist since they're a unique flag introduced by the use of UTXO snapshots, so this is prophylactic code exercised only by unittests. ACKs for top commit: achow101: ACK 817326a828d6148dc63d9ef08f641b9c0c522411 ryanofsky: Code review ACK 817326a828d6148dc63d9ef08f641b9c0c522411. This seems like the simplest change we can make to avoid wallet problems when an assumeutxo snapshot is loaded. Tree-SHA512: cfa44b2eb33d1818d30df45210d0dde1e9b78cc9b7c88cb985054dc28427bba9e0905debe4196065d1d3a5ce7bca7e605e629d5ce5f0225b25395746e6d3d596 --- src/interfaces/chain.h | 3 +++ src/node/interfaces.cpp | 5 +++++ src/wallet/wallet.cpp | 17 ++++++++++++++--- 3 files changed, 22 insertions(+), 3 deletions(-) diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index ea5fe712abf0d..cbea0fa54df96 100644 --- 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/node/interfaces.cpp b/src/node/interfaces.cpp index d680ab8488952..a8556c35d9e65 100644 --- 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/wallet/wallet.cpp b/src/wallet/wallet.cpp index a5db987bbb70b..6c0d275ce635d 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -5028,20 +5028,31 @@ bool CWallet::AttachChain(const std::shared_ptr& 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.hasAssumedValidChain() ? + _( + "Assumed-valid: last wallet synchronisation goes beyond " + "available block data. You need to wait for the background " + "validation chain to download more blocks.") : + _("Prune: last wallet synchronisation goes beyond pruned data. You need to -reindex (download the whole blockchain again in case of pruned node)"); return false; } } From c59cb158e53153d89786adc7f1f4e1954d1971ae Mon Sep 17 00:00:00 2001 From: fanquake Date: Mon, 10 Oct 2022 16:52:06 +0800 Subject: [PATCH 7/8] Merge bitcoin/bitcoin#26282: wallet: have prune error take precedence over assumedvalid 1c36bafc5f7db268546dcc86c793071a7e9d35e0 wallet: have prune error take precedence over assumedvalid (James O'Beirne) Pull request description: Fixes https://github.com/bitcoin/bitcoin/pull/23997#discussion_r891412739. From Russ Yanofsky: > Agree with all of Marco's points here and think this should be updated > > If havePrune and hasAssumedValidChain are both true, better to show havePrune error message. Assumed-valid error message is vague and not very actionable. Would suggest "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 though assumeutxo snapshots. Wallet should be able to load successfully after node sync reaches height {block_height}" ACKs for top commit: MarcoFalke: ACK 1c36bafc5f7db268546dcc86c793071a7e9d35e0 aureleoules: ACK 1c36bafc5f7db268546dcc86c793071a7e9d35e0 Tree-SHA512: bfb0024bb962525cbbd392ade3c0331a8b0525e7f2f2ab52b2dbb9b6dd6311070d85ecb762a7689db84a30991971865698ab6fec187206e6a92133790c5a91dc --- src/wallet/wallet.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 6c0d275ce635d..fd9ceeaf587be 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -5047,12 +5047,14 @@ bool CWallet::AttachChain(const std::shared_ptr& walletInstance, interf // If a block is pruned after this check, we will load the wallet, // but fail the rescan with a generic error. - error = chain.hasAssumedValidChain() ? - _( - "Assumed-valid: last wallet synchronisation goes beyond " - "available block data. You need to wait for the background " - "validation chain to download more blocks.") : - _("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; } } From 4101fea62094cecf7f4e0572fe3423de9e9be4c7 Mon Sep 17 00:00:00 2001 From: fanquake Date: Tue, 3 Oct 2023 10:43:41 +0100 Subject: [PATCH 8/8] Merge bitcoin/bitcoin#28304: doc: Remove confusing assert linter fa6e6a3f03a38f8b431bf694268ed344d1815b3b doc: Remove confusing assert linter (MarcoFalke) Pull request description: The `assert()` documentation and linter are redundant and confusing: * The source code already refuses to compile with `assert()` disabled. * They violate the assumptions about `Assert()`, which *requires* side effects. * The existing linter doesn't enforce the guideline, only checking for `++` and `--` side effects. Fix all issues by removing the docs and the linter. See also https://github.com/bitcoin/bitcoin/pull/26684#discussion_r1287370102 Going forward everyone is free to use whatever code in this regard they think is the easiest to read. Also, everyone is still free to share style-nits, if they think it is a good use of their time and of the pull request author. Finally, the author is still free to dismiss or ignore this style-nit, or any other style-nit. ACKs for top commit: hebasto: ACK fa6e6a3f03a38f8b431bf694268ed344d1815b3b, I have reviewed the code and it looks OK. theStack: ACK fa6e6a3f03a38f8b431bf694268ed344d1815b3b Tree-SHA512: 686738d71e1316cc95e5d3f71869b55a02bfb137c795cc0875057f4410e564bc8eff03c985a2087b007fb08fc84551c7da1e8b30c7a9c3f2b14e5e44a5970236 --- doc/developer-notes.md | 6 ------ test/lint/lint-assertions.py | 12 +----------- 2 files changed, 1 insertion(+), 17 deletions(-) diff --git a/doc/developer-notes.md b/doc/developer-notes.md index 5f5bcc129ecfe..a6b19e2e7a5ec 100644 --- a/doc/developer-notes.md +++ b/doc/developer-notes.md @@ -717,12 +717,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 diff --git a/test/lint/lint-assertions.py b/test/lint/lint-assertions.py index 195ff33d1127d..7731dc7901da4 100755 --- a/test/lint/lint-assertions.py +++ b/test/lint/lint-assertions.py @@ -23,20 +23,10 @@ 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:") - # 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)ss(ume|ert) *\(.*\);", "--",