Merge #6351: backport: trivial 2024 10 23 pr8

0dbafcee46 Merge bitcoin/bitcoin#27289: Refactor: Remove unused FlatFilePos::SetNull (fanquake)
dbe2e04d62 Merge bitcoin/bitcoin#27212: test: Make the unlikely race in p2p_invalid_messages impossible (fanquake)
6f6b718f78 Merge bitcoin/bitcoin#27236: util: fix argsman dupe key error (fanquake)
74c6e38530 Merge bitcoin/bitcoin#27205: doc: Show how less noisy clang-tidy output can be achieved (fanquake)
9e552f0293 Merge bitcoin/bitcoin#27232: Use string interpolation for default value of -listen (fanquake)
2a39b93233 Merge bitcoin/bitcoin#27226: test: Use self.wait_until over wait_until_helper (fanquake)
be2e16f33a Merge bitcoin/bitcoin#27192: util: add missing include and fix function signature (fanquake)
176a4a60d2 Merge bitcoin/bitcoin#27173: valgrind: remove libsecp256k1 suppression (fanquake)
d2fc8be331 Merge bitcoin/bitcoin#27154: doc: mention sanitizer suppressions in developer docs (glozow)
f5b4cc7e32 Merge bitcoin/bitcoin#16195: util: Use void* throughout support/lockedpool.h (Andrew Chow)
c66c0fdbf8 Merge bitcoin/bitcoin#27137: test: Raise PRNG seed log to INFO (fanquake)
bba215031b Merge bitcoin/bitcoin#25950: test: fix test abort for high timeout values (and `--timeout-factor 0`) (fanquake)
6751add2ea Merge bitcoin/bitcoin#27107: doc: remove mention of "proper signing key" (merge-script)
34c895a542 Merge bitcoin/bitcoin#26997: psbt: s/transcation/transaction/ (fanquake)
befdbeddf9 Merge bitcoin/bitcoin#27097: descriptors: fix docstring (param [in] vs [out]) (fanquake)
c98dd824b6 Merge bitcoin/bitcoin#27080: Wallet: Zero out wallet master key upon locking so it doesn't persist in memory (Andrew Chow)

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:
  knst:
    utACK 0dbafcee46
  UdjinM6:
    utACK 0dbafcee46

Tree-SHA512: e93a1136e848aa6c6f3d9fb0567b3e284975d35e82bbc1d9a8cd908067df4bf1257c939882abcaca6820360a4982b991f505a1165d95e1a8b52c9b181b7026b7

contrib/valgrind.supp

@@ -146,14 +146,6 @@
    fun:_Znwm
    fun:_Z11LogInstancev
 }
-{
-   Suppress secp256k1_context_create still reachable memory warning
-   Memcheck:Leak
-   match-leak-kinds: reachable
-   fun:malloc
-   ...
-   fun:secp256k1_context_create
-}
 {
    Suppress BCLog::Logger::StartLogging() still reachable memory warning
    Memcheck:Leak

contrib/verifybinaries/README.md

@@ -1,16 +1,5 @@
 ### Verify Binaries
 
-#### Preparation:
-
-Make sure you obtain the proper release signing key and verify the fingerprint with several independent sources.
-
-```sh
-$ gpg --fingerprint "Bitcoin Core binary release signing key"
-pub   4096R/36C2E964 2015-06-24 [expires: YYYY-MM-DD]
-      Key fingerprint = 01EA 5486 DE18 A882 D4C2  6845 90C8 019E 36C2 E964
-uid                  Wladimir J. van der Laan (Bitcoin Core binary release signing key) <laanwj@gmail.com>
-```
-
 #### Usage:
 
 This script attempts to download the signature file `SHA256SUMS.asc` from https://bitcoin.org.

doc/developer-notes.md

@@ -188,11 +188,14 @@ apt install clang-tidy bear clang
 Then, pass clang as compiler to configure, and use bear to produce the `compile_commands.json`:
 
 ```sh
-./autogen.sh && ./configure CC=clang CXX=clang++
-make clean && bear make -j $(nproc)     # For bear 2.x
-make clean && bear -- make -j $(nproc)  # For bear 3.x
+./autogen.sh && ./configure CC=clang CXX=clang++ --enable-suppress-external-warnings
+make clean && bear --config src/.bear-tidy-config -- make -j $(nproc)
 ```
 
+The output is denoised of errors from external dependencies and includes with
+`--enable-suppress-external-warnings` and `--config src/.bear-tidy-config`. Both
+options may be omitted to view the full list of errors.
+
 To run clang-tidy on all source files:
 
 ```sh
@@ -513,8 +516,19 @@ address sanitizer, libtsan for the thread sanitizer, and libubsan for the
 undefined sanitizer. If you are missing required libraries, the configure script
 will fail with a linker error when testing the sanitizer flags.
 
-The test suite should pass cleanly with the `thread` and `undefined` sanitizers,
-but there are a number of known problems when using the `address` sanitizer. The
+The test suite should pass cleanly with the `thread` and `undefined` sanitizers. You
+may need to use a suppressions file, see `test/sanitizer_suppressions`. They may be
+used as follows:
+```bash
+export LSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/lsan"
+export TSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/tsan:halt_on_error=1:second_deadlock_stack=1"
+export UBSAN_OPTIONS="suppressions=$(pwd)/test/sanitizer_suppressions/ubsan:print_stacktrace=1:halt_on_error=1:report_error_type=1"
+```
+
+See the CI config for more examples, and upstream documentation for more information
+about any additional options.
+
+There are a number of known problems when using the `address` sanitizer. The
 address sanitizer is known to fail in
 [sha256_sse4::Transform](/src/crypto/sha256_sse4.cpp) which makes it unusable
 unless you also use `--disable-asm` when running configure. We would like to fix

src/flatfile.h

@@ -13,15 +13,15 @@
 
 struct FlatFilePos
 {
-    int nFile;
-    unsigned int nPos;
+    int nFile{-1};
+    unsigned int nPos{0};
 
     SERIALIZE_METHODS(FlatFilePos, obj)
     {
         READWRITE(VARINT_MODE(obj.nFile, VarIntMode::NONNEGATIVE_SIGNED), VARINT(obj.nPos));
     }
 
-    FlatFilePos() : nFile(-1), nPos(0) {}
+    FlatFilePos() {}
 
     FlatFilePos(int nFileIn, unsigned int nPosIn) :
         nFile(nFileIn),
@@ -36,7 +36,6 @@ struct FlatFilePos
         return !(a == b);
     }
 
-    void SetNull() { nFile = -1; nPos = 0; }
     bool IsNull() const { return (nFile == -1); }
 
     std::string ToString() const;

src/index/disktxpos.h

@@ -10,7 +10,7 @@
 
 struct CDiskTxPos : public FlatFilePos
 {
-    unsigned int nTxOffset; // after header
+    unsigned int nTxOffset{0}; // after header
 
     SERIALIZE_METHODS(CDiskTxPos, obj)
     {
@@ -21,15 +21,7 @@ struct CDiskTxPos : public FlatFilePos
     CDiskTxPos(const FlatFilePos &blockIn, unsigned int nTxOffsetIn) : FlatFilePos(blockIn.nFile, blockIn.nPos), nTxOffset(nTxOffsetIn) {
     }
 
-    CDiskTxPos() {
-        SetNull();
-    }
-
-    void SetNull() {
-        FlatFilePos::SetNull();
-        nTxOffset = 0;
-    }
+    CDiskTxPos() {}
 };
 
-
 #endif // BITCOIN_INDEX_DISKTXPOS_H

src/init.cpp

@@ -564,7 +564,7 @@ void SetupServerArgs(ArgsManager& argsman)
     argsman.AddArg("-externalip=<ip>", "Specify your own public address", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-fixedseeds", strprintf("Allow fixed seeds if DNS seeds don't provide peers (default: %u)", DEFAULT_FIXEDSEEDS), ArgsManager::ALLOW_BOOL, OptionsCategory::CONNECTION);
     argsman.AddArg("-forcednsseed", strprintf("Always query for peer addresses via DNS lookup (default: %u)", DEFAULT_FORCEDNSSEED), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
-    argsman.AddArg("-listen", "Accept connections from outside (default: 1 if no -proxy or -connect)", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
+    argsman.AddArg("-listen", strprintf("Accept connections from outside (default: %u if no -proxy or -connect)", DEFAULT_LISTEN), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-listenonion", strprintf("Automatically create Tor onion service (default: %d)", DEFAULT_LISTEN_ONION), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-maxconnections=<n>", strprintf("Maintain at most <n> connections to peers (temporary service connections excluded) (default: %u). This limit does not apply to connections manually added via -addnode or the addnode RPC, which have a separate limit of %u.", DEFAULT_MAX_PEER_CONNECTIONS, MAX_ADDNODE_CONNECTIONS), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
     argsman.AddArg("-maxreceivebuffer=<n>", strprintf("Maximum per-connection receive buffer, <n>*1000 bytes (default: %u)", DEFAULT_MAXRECEIVEBUFFER), ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);

src/psbt.h

@@ -442,7 +442,7 @@ struct PartiallySignedTransaction
 
         // Make sure that we got an unsigned tx
         if (!tx) {
-            throw std::ios_base::failure("No unsigned transcation was provided");
+            throw std::ios_base::failure("No unsigned transaction was provided");
         }
 
         // Read input data

src/script/descriptor.h

@@ -35,7 +35,7 @@ public:
     /** Retrieve a cached parent xpub
      *
      * @param[in] key_exp_pos Position of the key expression within the descriptor
-     * @param[in] xpub The CExtPubKey to get from cache
+     * @param[out] xpub The CExtPubKey to get from cache
      */
     bool GetCachedParentExtPubKey(uint32_t key_exp_pos, CExtPubKey& xpub) const;
     /** Cache an xpub derived at an index
@@ -49,7 +49,7 @@ public:
      *
      * @param[in] key_exp_pos Position of the key expression within the descriptor
      * @param[in] der_index Derivation index of the xpub
-     * @param[in] xpub The CExtPubKey to get from cache
+     * @param[out] xpub The CExtPubKey to get from cache
      */
     bool GetCachedDerivedExtPubKey(uint32_t key_exp_pos, uint32_t der_index, CExtPubKey& xpub) const;
     /** Cache a last hardened xpub
@@ -61,7 +61,7 @@ public:
     /** Retrieve a cached last hardened xpub
      *
      * @param[in] key_exp_pos Position of the key expression within the descriptor
-     * @param[in] xpub The CExtPubKey to get from cache
+     * @param[out] xpub The CExtPubKey to get from cache
      */
     bool GetCachedLastHardenedExtPubKey(uint32_t key_exp_pos, CExtPubKey& xpub) const;
 

src/support/lockedpool.cpp

@@ -27,6 +27,7 @@
 #include <iomanip>
 #include <iostream>
 #endif
+#include <utility>
 
 LockedPoolManager* LockedPoolManager::_instance = nullptr;
 
@@ -43,12 +44,12 @@ static inline size_t align_up(size_t x, size_t align)
 // Implementation: Arena
 
 Arena::Arena(void *base_in, size_t size_in, size_t alignment_in):
-    base(static_cast<char*>(base_in)), end(static_cast<char*>(base_in) + size_in), alignment(alignment_in)
+    base(base_in), end(static_cast<char*>(base_in) + size_in), alignment(alignment_in)
 {
     // Start with one free chunk that covers the entire arena
     auto it = size_to_free_chunk.emplace(size_in, base);
     chunks_free.emplace(base, it);
-    chunks_free_end.emplace(base + size_in, it);
+    chunks_free_end.emplace(static_cast<char*>(base) + size_in, it);
 }
 
 Arena::~Arena()
@@ -74,8 +75,9 @@ void* Arena::alloc(size_t size)
 
     // Create the used-chunk, taking its space from the end of the free-chunk
     const size_t size_remaining = size_ptr_it->first - size;
-    auto allocated = chunks_used.emplace(size_ptr_it->second + size_remaining, size).first;
-    chunks_free_end.erase(size_ptr_it->second + size_ptr_it->first);
+    char* const free_chunk = static_cast<char*>(size_ptr_it->second);
+    auto allocated = chunks_used.emplace(free_chunk + size_remaining, size).first;
+    chunks_free_end.erase(free_chunk + size_ptr_it->first);
     if (size_ptr_it->first == size) {
         // whole chunk is used up
         chunks_free.erase(size_ptr_it->second);
@@ -83,11 +85,11 @@ void* Arena::alloc(size_t size)
         // still some memory left in the chunk
         auto it_remaining = size_to_free_chunk.emplace(size_remaining, size_ptr_it->second);
         chunks_free[size_ptr_it->second] = it_remaining;
-        chunks_free_end.emplace(size_ptr_it->second + size_remaining, it_remaining);
+        chunks_free_end.emplace(free_chunk + size_remaining, it_remaining);
     }
     size_to_free_chunk.erase(size_ptr_it);
 
-    return reinterpret_cast<void*>(allocated->first);
+    return allocated->first;
 }
 
 void Arena::free(void *ptr)
@@ -98,11 +100,11 @@ void Arena::free(void *ptr)
     }
 
     // Remove chunk from used map
-    auto i = chunks_used.find(static_cast<char*>(ptr));
+    auto i = chunks_used.find(ptr);
     if (i == chunks_used.end()) {
         throw std::runtime_error("Arena: invalid or double free");
     }
-    std::pair<char*, size_t> freed = *i;
+    auto freed = std::make_pair(static_cast<char*>(i->first), i->second);
     chunks_used.erase(i);
 
     // coalesce freed with previous chunk

src/support/lockedpool.h

@@ -89,23 +89,23 @@ public:
      */
     bool addressInArena(void *ptr) const { return ptr >= base && ptr < end; }
 private:
-    typedef std::multimap<size_t, char*> SizeToChunkSortedMap;
+    typedef std::multimap<size_t, void*> SizeToChunkSortedMap;
     /** Map to enable O(log(n)) best-fit allocation, as it's sorted by size */
     SizeToChunkSortedMap size_to_free_chunk;
 
-    typedef std::unordered_map<char*, SizeToChunkSortedMap::const_iterator> ChunkToSizeMap;
+    typedef std::unordered_map<void*, SizeToChunkSortedMap::const_iterator> ChunkToSizeMap;
     /** Map from begin of free chunk to its node in size_to_free_chunk */
     ChunkToSizeMap chunks_free;
     /** Map from end of free chunk to its node in size_to_free_chunk */
     ChunkToSizeMap chunks_free_end;
 
     /** Map from begin of used chunk to its size */
-    std::unordered_map<char*, size_t> chunks_used;
+    std::unordered_map<void*, size_t> chunks_used;
 
     /** Base address of arena */
-    char* base;
+    void* base;
     /** End address of arena */
-    char* end;
+    void* end;
     /** Minimum chunk alignment */
     size_t alignment;
 };

src/test/flatfile_tests.cpp

@@ -23,6 +23,9 @@ BOOST_AUTO_TEST_CASE(flatfile_filename)
 
     FlatFileSeq seq2(data_dir / "a", "b", 16 * 1024);
     BOOST_CHECK_EQUAL(seq2.FileName(pos), data_dir / "a" / "b00456.dat");
+
+    // Check default constructor IsNull
+    assert(FlatFilePos{}.IsNull());
 }
 
 BOOST_AUTO_TEST_CASE(flatfile_open)

src/test/fuzz/flatfile.cpp

@@ -25,6 +25,4 @@ FUZZ_TARGET(flatfile)
         assert((*flat_file_pos == *another_flat_file_pos) != (*flat_file_pos != *another_flat_file_pos));
     }
     (void)flat_file_pos->ToString();
-    flat_file_pos->SetNull();
-    assert(flat_file_pos->IsNull());
 }

src/test/settings_tests.cpp

@@ -80,7 +80,7 @@ BOOST_AUTO_TEST_CASE(ReadWrite)
     BOOST_CHECK(values.empty());
     BOOST_CHECK(errors.empty());
 
-    // Check duplicate keys not allowed
+    // Check duplicate keys not allowed and that values returns empty if a duplicate is found.
     WriteText(path, R"({
         "dupe": "string",
         "dupe": "dupe"
@@ -88,6 +88,7 @@ BOOST_AUTO_TEST_CASE(ReadWrite)
     BOOST_CHECK(!util::ReadSettings(path, values, errors));
     std::vector<std::string> dup_keys = {strprintf("Found duplicate key dupe in settings file %s", fs::PathToString(path))};
     BOOST_CHECK_EQUAL_COLLECTIONS(errors.begin(), errors.end(), dup_keys.begin(), dup_keys.end());
+    BOOST_CHECK(values.empty());
 
     // Check non-kv json files not allowed
     WriteText(path, R"("non-kv")");

src/util/readwritefile.cpp

@@ -3,6 +3,8 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
 
+#include <util/readwritefile.h>
+
 #include <fs.h>
 
 #include <limits>
@@ -10,7 +12,7 @@
 #include <string>
 #include <utility>
 
-std::pair<bool,std::string> ReadBinaryFile(const fs::path &filename, size_t maxsize=std::numeric_limits<size_t>::max())
+std::pair<bool,std::string> ReadBinaryFile(const fs::path &filename, size_t maxsize)
 {
     FILE *f = fsbridge::fopen(filename, "rb");
     if (f == nullptr)

src/util/settings.cpp

@@ -111,6 +111,8 @@ bool ReadSettings(const fs::path& path, std::map<std::string, SettingsValue>& va
         auto inserted = values.emplace(in_keys[i], in_values[i]);
         if (!inserted.second) {
             errors.emplace_back(strprintf("Found duplicate key %s in settings file %s", in_keys[i], fs::PathToString(path)));
+            values.clear();
+            break;
         }
     }
     return errors.empty();

src/wallet/wallet.cpp

@@ -25,6 +25,7 @@
 #include <script/script.h>
 #include <script/sign.h>
 #include <script/signingprovider.h>
+#include <support/cleanse.h>
 #include <txmempool.h>
 #include <util/bip32.h>
 #include <util/check.h>
@@ -5588,7 +5589,10 @@ bool CWallet::Lock(bool fAllowMixing)
 
     if(!fAllowMixing) {
         LOCK(cs_wallet);
-        vMasterKey.clear();
+        if (!vMasterKey.empty()) {
+            memory_cleanse(vMasterKey.data(), vMasterKey.size() * sizeof(decltype(vMasterKey)::value_type));
+            vMasterKey.clear();
+        }
     }
 
     fOnlyMixingAllowed = fAllowMixing;

test/functional/p2p_invalid_messages.py

@@ -66,6 +66,11 @@ class InvalidMessagesTest(BitcoinTestFramework):
     def test_buffer(self):
         self.log.info("Test message with header split across two buffers is received")
         conn = self.nodes[0].add_p2p_connection(P2PDataStore())
+        # After add_p2p_connection both sides have the verack processed.
+        # However the pong from conn in reply to the ping from the node has not
+        # been processed and recorded in totalbytesrecv.
+        # Flush the pong from conn by sending a ping from conn.
+        conn.sync_with_ping(timeout=1)
         # Create valid message
         msg = conn.build_message(msg_ping(nonce=12345))
         cut_pos = 12  # Chosen at an arbitrary position within the header
@@ -75,8 +80,6 @@ class InvalidMessagesTest(BitcoinTestFramework):
         # Wait until node has processed the first half of the message
         self.wait_until(lambda: self.nodes[0].getnettotals()['totalbytesrecv'] != before)
         middle = self.nodes[0].getnettotals()['totalbytesrecv']
-        # If this assert fails, we've hit an unlikely race
-        # where the test framework sent a message in between the two halves
         assert_equal(middle, before + cut_pos)
         conn.send_raw_message(msg[cut_pos:])
         conn.sync_with_ping(timeout=1)

test/functional/test_framework/authproxy.py

@@ -78,7 +78,10 @@ class AuthServiceProxy():
         passwd = None if self.__url.password is None else self.__url.password.encode('utf8')
         authpair = user + b':' + passwd
         self.__auth_header = b'Basic ' + base64.b64encode(authpair)
-        self.timeout = timeout
+        # clamp the socket timeout, since larger values can cause an
+        # "Invalid argument" exception in Python's HTTP(S) client
+        # library on some operating systems (e.g. OpenBSD, FreeBSD)
+        self.timeout = min(timeout, 2147483)
         self._set_conn(connection)
 
     def __getattr__(self, name):

test/functional/test_framework/p2p.py

@@ -620,7 +620,7 @@ class P2PInterface(P2PConnection):
 
     def wait_for_connect(self, timeout=60):
         test_function = lambda: self.is_connected
-        wait_until_helper(test_function, timeout=timeout, lock=p2p_lock)
+        self.wait_until(test_function, timeout=timeout, check_connected=False)
 
     def wait_for_disconnect(self, timeout=60):
         test_function = lambda: not self.is_connected

test/functional/test_framework/test_framework.py

@@ -305,10 +305,10 @@ class BitcoinTestFramework(metaclass=BitcoinTestMetaClass):
         if seed is None:
             seed = random.randrange(sys.maxsize)
         else:
-            self.log.debug("User supplied random seed {}".format(seed))
+            self.log.info("User supplied random seed {}".format(seed))
 
         random.seed(seed)
-        self.log.debug("PRNG seed is: {}".format(seed))
+        self.log.info("PRNG seed is: {}".format(seed))
 
         self.log.debug('Setting up network thread')
         self.network_thread = NetworkThread()