Merge pull request #5297 from PastaPastaPasta/develop-trivial-2023-04-06

backport: trivial backports April 6

configure.ac

@@ -435,17 +435,18 @@ if test "x$enable_werror" = "xyes"; then
   AX_CHECK_COMPILE_FLAG([-Werror=suggest-override],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=suggest-override"],,[[$CXXFLAG_WERROR]],
                         [AC_LANG_SOURCE([[struct A { virtual void f(); }; struct B : A { void f() final; };]])])
   AX_CHECK_COMPILE_FLAG([-Werror=unreachable-code-loop-increment],[ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=unreachable-code-loop-increment"],,[[$CXXFLAG_WERROR]])
+  AX_CHECK_COMPILE_FLAG([-Werror=mismatched-tags], [ERROR_CXXFLAGS="$ERROR_CXXFLAGS -Werror=mismatched-tags"], [], [$CXXFLAG_WERROR])
 fi
 
 if test "x$CXXFLAGS_overridden" = "xno"; then
   AX_CHECK_COMPILE_FLAG([-Wall],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wall"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wextra],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wextra"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wgnu],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wgnu"],,[[$CXXFLAG_WERROR]])
-  AX_CHECK_COMPILE_FLAG([-Wformat],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat"],,[[$CXXFLAG_WERROR]])
+  dnl some compilers will ignore -Wformat-security without -Wformat, so just combine the two here.
+  AX_CHECK_COMPILE_FLAG([-Wformat -Wformat-security],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat -Wformat-security"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wvla],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wvla"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wshadow-field],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wshadow-field"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wswitch],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wswitch"],,[[$CXXFLAG_WERROR]])
-  AX_CHECK_COMPILE_FLAG([-Wformat-security],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wformat-security"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wthread-safety],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wthread-safety"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wrange-loop-analysis],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wrange-loop-analysis"],,[[$CXXFLAG_WERROR]])
   AX_CHECK_COMPILE_FLAG([-Wredundant-decls],[WARN_CXXFLAGS="$WARN_CXXFLAGS -Wredundant-decls"],,[[$CXXFLAG_WERROR]])

depends/README.md

@@ -41,7 +41,7 @@ The paths are automatically configured and no other options are needed unless ta
 
 #### For macOS cross compilation
 
-    sudo apt-get install curl librsvg2-bin libtiff-tools bsdmainutils cmake imagemagick libcap-dev libz-dev libbz2-dev python3-setuptools libtinfo5
+    sudo apt-get install curl librsvg2-bin libtiff-tools bsdmainutils cmake imagemagick libcap-dev libz-dev libbz2-dev python3-setuptools libtinfo5 xorriso
 
 Note: You must obtain the macOS SDK before proceeding with a cross-compile.
 Under the depends directory, create a subdirectory named `SDKs`.

doc/files.md

@@ -8,6 +8,10 @@
 
 - [Multi-wallet environment](#multi-wallet-environment)
 
+  - [Berkeley DB database based wallets](#berkeley-db-database-based-wallets)
+
+  - [SQLite database based wallets](#sqlite-database-based-wallets)
+
 - [GUI settings](#gui-settings)
 
 - [Legacy subdirectories and files](#legacy-subdirectories-and-files)
@@ -69,26 +73,36 @@ Subdirectory       | File(s)               | Description
 
 ## Multi-wallet environment
 
-Wallets are Berkeley DB (BDB) databases:
+Wallets are Berkeley DB (BDB) or SQLite databases.
 
-Subdirectory | File(s)           | Description
--------------|-------------------|------------
-`database/`  | BDB logging files | Part of BDB environment; created at start and deleted on shutdown; a user *must keep it as safe* as personal wallet `wallet.dat`
-`./`         | `db.log`          | BDB error file
-`./`         | `wallet.dat`      | Personal wallet with keys and transactions. May be either a Berkeley DB or SQLite database file.
-`./`         | `.walletlock`     | Wallet lock file
-`./`         | `wallet.dat-journal` | SQLite Rollback Journal file for `wallet.dat`. Usually created at start and deleted on shutdown. A user *must keep it as safe* as the `wallet.dat` file.
-
-1. Each user-defined wallet named "wallet_name" resides in `wallets/wallet_name/` subdirectory.
+1. Each user-defined wallet named "wallet_name" resides in the `wallets/wallet_name/` subdirectory.
 
 2. The default (unnamed) wallet resides in `wallets/` subdirectory; if the latter does not exist, the wallet resides in the data directory.
 
-3. A wallet database path can be specified by `-wallet` option.
+3. A wallet database path can be specified with the `-wallet` option.
 
 4. `wallet.dat` files must not be shared across different node instances, as that can result in key-reuse and double-spends due the lack of synchronization between instances.
 
 5. Any copy or backup of the wallet should be done through a `backupwallet` call in order to update and lock the wallet, preventing any file corruption caused by updates during the copy.
 
+
+### Berkeley DB database based wallets
+
+Subdirectory | File(s)           | Description
+-------------|-------------------|-------------
+`database/`  | BDB logging files | Part of BDB environment; created at start and deleted on shutdown; a user *must keep it as safe* as personal wallet `wallet.dat`
+`./`         | `db.log`          | BDB error file
+`./`         | `wallet.dat`      | Personal wallet (a BDB database) with keys and transactions
+`./`         | `.walletlock`     | BDB wallet lock file
+
+### SQLite database based wallets
+
+Subdirectory | File                 | Description
+-------------|----------------------|-------------
+`./`         | `wallet.dat`         | Personal wallet (a SQLite database) with keys and transactions
+`./`         | `wallet.dat-journal` | SQLite Rollback Journal file for `wallet.dat`. Usually created at start and deleted on shutdown. A user *must keep it as safe* as the `wallet.dat` file.
+
+
 ## GUI settings
 
 `dash-qt` uses [`QSettings`](https://doc.qt.io/qt-5/qsettings.html) class; this implies platform-specific [locations where application settings are stored](https://doc.qt.io/qt-5/qsettings.html#locations-where-application-settings-are-stored).

doc/release-notes-18466.md

@@ -0,0 +1,10 @@
+Low-level RPC changes
+---------------------
+
+- Error codes have been updated to be more accurate for the following error cases (#18466):
+  - `signmessage` now returns RPC_INVALID_ADDRESS_OR_KEY (-5) if the
+    passed address is invalid. Previously returned RPC_TYPE_ERROR (-3).
+  - `verifymessage` now returns RPC_INVALID_ADDRESS_OR_KEY (-5) if the
+    passed address is invalid. Previously returned RPC_TYPE_ERROR (-3).
+  - `verifymessage` now returns RPC_TYPE_ERROR (-3) if the passed signature
+    is malformed. Previously returned RPC_INVALID_ADDRESS_OR_KEY (-5).

src/net_processing.cpp

@@ -2889,8 +2889,7 @@ void PeerLogicValidation::ProcessMessage(
     }
 
     if (!pfrom.fSuccessfullyConnected) {
-        // Must have a verack message before anything else
-        Misbehaving(pfrom.GetId(), 1, "non-verack message before version handshake");
+        LogPrint(BCLog::NET, "Unsupported message \"%s\" prior to verack from peer=%d\n", SanitizeString(msg_type), pfrom.GetId());
         return;
     }
 

src/policy/fees.cpp

@@ -6,6 +6,8 @@
 #include <policy/fees.h>
 
 #include <clientversion.h>
+#include <fs.h>
+#include <logging.h>
 #include <streams.h>
 #include <txmempool.h>
 #include <util/system.h>
@@ -14,17 +16,14 @@ static const char* FEE_ESTIMATES_FILENAME="fee_estimates.dat";
 
 static constexpr double INF_FEERATE = 1e99;
 
-std::string StringForFeeEstimateHorizon(FeeEstimateHorizon horizon) {
-    static const std::map<FeeEstimateHorizon, std::string> horizon_strings = {
-        {FeeEstimateHorizon::SHORT_HALFLIFE, "short"},
-        {FeeEstimateHorizon::MED_HALFLIFE, "medium"},
-        {FeeEstimateHorizon::LONG_HALFLIFE, "long"},
-    };
-    auto horizon_string = horizon_strings.find(horizon);
-
-    if (horizon_string == horizon_strings.end()) return "unknown";
-
-    return horizon_string->second;
+std::string StringForFeeEstimateHorizon(FeeEstimateHorizon horizon)
+{
+    switch (horizon) {
+    case FeeEstimateHorizon::SHORT_HALFLIFE: return "short";
+    case FeeEstimateHorizon::MED_HALFLIFE: return "medium";
+    case FeeEstimateHorizon::LONG_HALFLIFE: return "long";
+    } // no default case, so the compiler can warn about missing cases
+    assert(false);
 }
 /**
  * We will instantiate an instance of this class to track transactions that were
@@ -640,7 +639,7 @@ CFeeRate CBlockPolicyEstimator::estimateFee(int confTarget) const
 
 CFeeRate CBlockPolicyEstimator::estimateRawFee(int confTarget, double successThreshold, FeeEstimateHorizon horizon, EstimationResult* result) const
 {
-    TxConfirmStats* stats;
+    TxConfirmStats* stats = nullptr;
     double sufficientTxs = SUFFICIENT_FEETXS;
     switch (horizon) {
     case FeeEstimateHorizon::SHORT_HALFLIFE: {
@@ -656,10 +655,8 @@ CFeeRate CBlockPolicyEstimator::estimateRawFee(int confTarget, double successThr
         stats = longStats.get();
         break;
     }
-    default: {
-        throw std::out_of_range("CBlockPolicyEstimator::estimateRawFee unknown FeeEstimateHorizon");
-    }
-    }
+    } // no default case, so the compiler can warn about missing cases
+    assert(stats);
 
     LOCK(m_cs_fee_estimator);
     // Return failure if trying to analyze a target we're not tracking
@@ -689,10 +686,8 @@ unsigned int CBlockPolicyEstimator::HighestTargetTracked(FeeEstimateHorizon hori
     case FeeEstimateHorizon::LONG_HALFLIFE: {
         return longStats->GetMaxConfirms();
     }
-    default: {
-        throw std::out_of_range("CBlockPolicyEstimator::HighestTargetTracked unknown FeeEstimateHorizon");
-    }
-    }
+    } // no default case, so the compiler can warn about missing cases
+    assert(false);
 }
 
 unsigned int CBlockPolicyEstimator::BlockSpan() const
@@ -870,7 +865,7 @@ void CBlockPolicyEstimator::Flush() {
     fs::path est_filepath = GetDataDir() / FEE_ESTIMATES_FILENAME;
     CAutoFile est_file(fsbridge::fopen(est_filepath, "wb"), SER_DISK, CLIENT_VERSION);
     if (est_file.IsNull() || !Write(est_file)) {
-        LogPrintf("Failed to write fee estimates to %s\n", est_filepath.string());
+        LogPrintf("Failed to write fee estimates to %s. Continue anyway.\n", est_filepath.string());
     }
 }
 
@@ -906,8 +901,9 @@ bool CBlockPolicyEstimator::Read(CAutoFile& filein)
         int nVersionRequired, nVersionThatWrote;
         unsigned int nFileBestSeenHeight;
         filein >> nVersionRequired >> nVersionThatWrote;
-        if (nVersionRequired > CLIENT_VERSION)
-            return error("CBlockPolicyEstimator::Read(): up-version (%d) fee estimate file", nVersionRequired);
+        if (nVersionRequired > CLIENT_VERSION) {
+            throw std::runtime_error(strprintf("up-version (%d) fee estimate file", nVersionRequired));
+        }
 
         // Read fee estimates file into temporary variables so existing data
         // structures aren't corrupted if there is an exception.
@@ -924,8 +920,9 @@ bool CBlockPolicyEstimator::Read(CAutoFile& filein)
             std::vector<double> fileBuckets;
             filein >> fileBuckets;
             size_t numBuckets = fileBuckets.size();
-            if (numBuckets <= 1 || numBuckets > 1000)
+            if (numBuckets <= 1 || numBuckets > 1000) {
                 throw std::runtime_error("Corrupt estimates file. Must have between 2 and 1000 feerate buckets");
+            }
 
             auto fileFeeStats{std::make_unique<TxConfirmStats>(buckets, bucketMap, MED_BLOCK_PERIODS, MED_DECAY, MED_SCALE)};
             auto fileShortStats{std::make_unique<TxConfirmStats>(buckets, bucketMap, SHORT_BLOCK_PERIODS, SHORT_DECAY, SHORT_SCALE)};

src/rpc/misc.cpp

@@ -447,11 +447,11 @@ static UniValue verifymessage(const JSONRPCRequest& request)
 
     switch (MessageVerify(strAddress, strSign, strMessage)) {
     case MessageVerificationResult::ERR_INVALID_ADDRESS:
-        throw JSONRPCError(RPC_TYPE_ERROR, "Invalid address");
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
     case MessageVerificationResult::ERR_ADDRESS_NO_KEY:
         throw JSONRPCError(RPC_TYPE_ERROR, "Address does not refer to key");
     case MessageVerificationResult::ERR_MALFORMED_SIGNATURE:
-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Malformed base64 encoding");
+        throw JSONRPCError(RPC_TYPE_ERROR, "Malformed base64 encoding");
     case MessageVerificationResult::ERR_PUBKEY_NOT_RECOVERED:
     case MessageVerificationResult::ERR_NOT_SIGNED:
         return false;

src/test/fuzz/process_message.cpp

@@ -73,6 +73,10 @@ void fuzz_target(const std::vector<uint8_t>& buffer, const std::string& LIMIT_TO
         g_setup->m_node.peer_logic->ProcessMessage(p2p_node, random_message_type, random_bytes_data_stream, GetTimeMillis(), Params(), std::atomic<bool>{false});
     } catch (const std::ios_base::failure& e) {
     }
+    {
+        LOCK(p2p_node.cs_sendProcessing);
+        g_setup->m_node.peer_logic->SendMessages(&p2p_node);
+    }
     SyncWithValidationInterfaceQueue();
 }
 

src/test/fuzz/process_messages.cpp

@@ -72,6 +72,10 @@ FUZZ_TARGET_INIT(process_messages, initialize_process_messages)
             connman.ProcessMessagesOnce(random_node);
         } catch (const std::ios_base::failure&) {
         }
+        {
+            LOCK(random_node.cs_sendProcessing);
+            g_setup->m_node.peer_logic->SendMessages(&random_node);
+        }
     }
     connman.ClearTestNodes();
     SyncWithValidationInterfaceQueue();

src/test/fuzz/util.h

@@ -255,8 +255,8 @@ inline CNetAddr ConsumeNetAddr(FuzzedDataProvider& fuzzed_data_provider) noexcep
     const Network network = fuzzed_data_provider.PickValueInArray({Network::NET_IPV4, Network::NET_IPV6, Network::NET_INTERNAL, Network::NET_ONION});
     CNetAddr net_addr;
     if (network == Network::NET_IPV4) {
-        const in_addr v4_addr = {
-            .s_addr = fuzzed_data_provider.ConsumeIntegral<uint32_t>()};
+        in_addr v4_addr = {};
+        v4_addr.s_addr = fuzzed_data_provider.ConsumeIntegral<uint32_t>();
         net_addr = CNetAddr{v4_addr};
     } else if (network == Network::NET_IPV6) {
         if (fuzzed_data_provider.remaining_bytes() >= 16) {

src/test/sock_tests.cpp

@@ -95,7 +95,7 @@ static void CreateSocketPair(int s[2])
 static void SendAndRecvMessage(const Sock& sender, const Sock& receiver)
 {
     const char* msg = "abcd";
-    constexpr size_t msg_len = 4;
+    constexpr ssize_t msg_len = 4;
     char recv_buf[10];
 
     BOOST_CHECK_EQUAL(sender.Send(msg, msg_len, 0), msg_len);

src/validation.h

@@ -47,7 +47,7 @@ class CBlockIndex;
 class CBlockTreeDB;
 class CBlockUndo;
 class CChainParams;
-class CCheckpointData;
+struct CCheckpointData;
 class CInv;
 class CConnman;
 class CScriptCheck;

src/wallet/bdb.cpp

@@ -723,6 +723,23 @@ bool BerkeleyBatch::TxnAbort()
     return (ret == 0);
 }
 
+bool BerkeleyDatabaseSanityCheck()
+{
+    int major, minor;
+    DbEnv::version(&major, &minor, nullptr);
+
+    /* If the major version differs, or the minor version of library is *older*
+     * than the header that was compiled against, flag an error.
+     */
+    if (major != DB_VERSION_MAJOR || minor < DB_VERSION_MINOR) {
+        LogPrintf("BerkeleyDB database version conflict: header version is %d.%d, library version is %d.%d\n",
+            DB_VERSION_MAJOR, DB_VERSION_MINOR, major, minor);
+        return false;
+    }
+
+    return true;
+}
+
 std::string BerkeleyDatabaseVersion()
 {
     return DbEnv::version(nullptr, nullptr, nullptr);

src/wallet/bdb.h

@@ -222,6 +222,10 @@ public:
 
 std::string BerkeleyDatabaseVersion();
 
+/** Perform sanity check of runtime BDB version versus linked BDB version.
+ */
+bool BerkeleyDatabaseSanityCheck();
+
 //! Return object giving access to Berkeley database at specified path.
 std::unique_ptr<BerkeleyDatabase> MakeBerkeleyDatabase(const fs::path& path, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);
 

src/wallet/init.cpp

@@ -118,6 +118,11 @@ void WalletInit::AddWalletOptions(ArgsManager& argsman) const
 
 bool WalletInit::ParameterInteraction() const
 {
+#ifdef USE_BDB
+     if (!BerkeleyDatabaseSanityCheck()) {
+         return InitError(Untranslated("A version conflict was detected between the run-time BerkeleyDB library and the one used during compilation."));
+     }
+#endif
     if (gArgs.GetBoolArg("-disablewallet", DEFAULT_DISABLE_WALLET)) {
         for (const std::string& wallet : gArgs.GetArgs("-wallet")) {
             LogPrintf("%s: parameter interaction: -disablewallet -> ignoring -wallet=%s\n", __func__, wallet);

src/wallet/rpcwallet.cpp

@@ -589,7 +589,7 @@ static UniValue signmessage(const JSONRPCRequest& request)
 
     CTxDestination dest = DecodeDestination(strAddress);
     if (!IsValidDestination(dest)) {
-        throw JSONRPCError(RPC_TYPE_ERROR, "Invalid address");
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "Invalid address");
     }
 
     const PKHash *pkhash = std::get_if<PKHash>(&dest);

src/wallet/wallet.cpp

@@ -2638,8 +2638,8 @@ std::map<CTxDestination, std::vector<COutput>> CWallet::ListCoins() const
 
     std::vector<COutPoint> lockedCoins;
     ListLockedCoins(lockedCoins);
-    // Include watch-only for wallets without private keys
-    const bool include_watch_only = IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
+    // Include watch-only for LegacyScriptPubKeyMan wallets without private keys
+    const bool include_watch_only = GetLegacyScriptPubKeyMan() && IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
     const isminetype is_mine_filter = include_watch_only ? ISMINE_WATCH_ONLY : ISMINE_SPENDABLE;
     for (const COutPoint& output : lockedCoins) {
         auto it = mapWallet.find(output.hash);

test/functional/feature_notifications.py

@@ -16,7 +16,7 @@ from test_framework.util import (
 # Windows disallow control characters (0-31) and /\?%:|"<>
 FILE_CHAR_START = 32 if os.name == 'nt' else 1
 FILE_CHAR_END = 128
-FILE_CHAR_BLOCKLIST = '/\\?%*:|"<>' if os.name == 'nt' else '/'
+FILE_CHARS_DISALLOWED = '/\\?%*:|"<>' if os.name == 'nt' else '/'
 
 
 def notify_outputname(walletname, txid):
@@ -29,7 +29,7 @@ class NotificationsTest(BitcoinTestFramework):
         self.setup_clean_chain = True
 
     def setup_network(self):
-        self.wallet = ''.join(chr(i) for i in range(FILE_CHAR_START, FILE_CHAR_END) if chr(i) not in FILE_CHAR_BLOCKLIST)
+        self.wallet = ''.join(chr(i) for i in range(FILE_CHAR_START, FILE_CHAR_END) if chr(i) not in FILE_CHARS_DISALLOWED)
         self.alertnotify_dir = os.path.join(self.options.tmpdir, "alertnotify")
         self.blocknotify_dir = os.path.join(self.options.tmpdir, "blocknotify")
         self.walletnotify_dir = os.path.join(self.options.tmpdir, "walletnotify")

test/functional/p2p_compactblocks.py

@@ -183,28 +183,21 @@ class CompactBlocksTest(BitcoinTestFramework):
         test_node.request_headers_and_sync(locator=[tip])
 
         # Now try a SENDCMPCT message with too-high version
-        sendcmpct = msg_sendcmpct()
-        sendcmpct.version = preferred_version + 1
-        sendcmpct.announce = True
-        test_node.send_and_ping(sendcmpct)
+        test_node.send_and_ping(msg_sendcmpct(announce=True, version=preferred_version+1))
         check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" not in p.last_message)
 
         # Headers sync before next test.
         test_node.request_headers_and_sync(locator=[tip])
 
         # Now try a SENDCMPCT message with valid version, but announce=False
-        sendcmpct.version = preferred_version
-        sendcmpct.announce = False
-        test_node.send_and_ping(sendcmpct)
+        test_node.send_and_ping(msg_sendcmpct(announce=False, version=preferred_version))
         check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" not in p.last_message)
 
         # Headers sync before next test.
         test_node.request_headers_and_sync(locator=[tip])
 
         # Finally, try a SENDCMPCT message with announce=True
-        sendcmpct.version = preferred_version
-        sendcmpct.announce = True
-        test_node.send_and_ping(sendcmpct)
+        test_node.send_and_ping(msg_sendcmpct(announce=True, version=preferred_version))
         check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" in p.last_message)
 
         # Try one more time (no headers sync should be needed!)
@@ -215,15 +208,11 @@ class CompactBlocksTest(BitcoinTestFramework):
         check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" in p.last_message)
 
         # Try one more time, after sending a version-1, announce=false message.
-        sendcmpct.version = preferred_version - 1
-        sendcmpct.announce = False
-        test_node.send_and_ping(sendcmpct)
+        test_node.send_and_ping(msg_sendcmpct(announce=False, version=preferred_version-1))
         check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" in p.last_message)
 
         # Now turn off announcements
-        sendcmpct.version = preferred_version
-        sendcmpct.announce = False
-        test_node.send_and_ping(sendcmpct)
+        test_node.send_and_ping(msg_sendcmpct(announce=False, version=preferred_version))
         check_announcement_of_new_block(node, test_node, lambda p: "cmpctblock" not in p.last_message and "headers" in p.last_message)
 
         # This code should be enabled after increasing cmctblk version
@@ -231,9 +220,7 @@ class CompactBlocksTest(BitcoinTestFramework):
         if to_validate and old_node is not None:
             # Verify that a peer using an older protocol version can receive
             # announcements from this node.
-            sendcmpct.version = preferred_version - 1
-            sendcmpct.announce = True
-            old_node.send_and_ping(sendcmpct)
+            old_node.send_and_ping(msg_sendcmpct(announce=True, version=preferred_version-1))
             # Header sync
             old_node.request_headers_and_sync(locator=[tip])
             check_announcement_of_new_block(node, old_node, lambda p: "cmpctblock" in p.last_message)
@@ -681,11 +668,7 @@ class CompactBlocksTest(BitcoinTestFramework):
         node = self.nodes[0]
         tip = node.getbestblockhash()
         peer.get_headers(locator=[int(tip, 16)], hashstop=0)
-
-        msg = msg_sendcmpct()
-        msg.version = peer.cmpct_version
-        msg.announce = True
-        peer.send_and_ping(msg)
+        peer.send_and_ping(msg_sendcmpct(announce=True, version=peer.cmpct_version))
 
     def test_compactblock_reconstruction_multiple_peers(self, stalling_peer, delivery_peer):
         node = self.nodes[0]

test/functional/p2p_filter.py

@@ -12,10 +12,7 @@ from test_framework.messages import (
     msg_getdata,
     msg_filterload,
 )
-from test_framework.mininode import (
-    P2PInterface,
-    mininode_lock,
-)
+from test_framework.mininode import P2PInterface
 from test_framework.test_framework import BitcoinTestFramework
 
 
@@ -68,18 +65,15 @@ class FilterTest(BitcoinTestFramework):
         filter_address = self.nodes[0].decodescript(filter_node.watch_script_pubkey)['addresses'][0]
 
         self.log.info('Check that we receive merkleblock and tx if the filter matches a tx in a block')
-        filter_node.merkleblock_received = False
         block_hash = self.nodes[0].generatetoaddress(1, filter_address)[0]
         txid = self.nodes[0].getblock(block_hash)['tx'][0]
+        filter_node.wait_for_merkleblock(block_hash)
         filter_node.wait_for_tx(txid)
-        assert filter_node.merkleblock_received
 
         self.log.info('Check that we only receive a merkleblock if the filter does not match a tx in a block')
-        with mininode_lock:
-            filter_node.last_message.pop("merkleblock", None)
         filter_node.tx_received = False
-        self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())
-        filter_node.wait_for_merkleblock()
+        block_hash = self.nodes[0].generatetoaddress(1, self.nodes[0].getnewaddress())[0]
+        filter_node.wait_for_merkleblock(block_hash)
         assert not filter_node.tx_received
 
         self.log.info('Check that we not receive a tx if the filter does not match a mempool tx')

test/functional/p2p_invalid_locator.py

@@ -34,7 +34,7 @@ class InvalidLocatorTest(BitcoinTestFramework):
             msg.locator.vHave = [int(node.getblockhash(i - 1), 16) for i in range(block_count, block_count - (MAX_LOCATOR_SZ), -1)]
             node.p2p.send_message(msg)
             if type(msg) == msg_getheaders:
-                node.p2p.wait_for_header(int(node.getbestblockhash(), 16))
+                node.p2p.wait_for_header(node.getbestblockhash())
             else:
                 node.p2p.wait_for_block(int(node.getbestblockhash(), 16))
 

test/functional/rpc_signmessage.py

@@ -5,7 +5,10 @@
 """Test RPC commands for signing and verifying messages."""
 
 from test_framework.test_framework import BitcoinTestFramework
-from test_framework.util import assert_equal
+from test_framework.util import (
+    assert_equal,
+    assert_raises_rpc_error,
+)
 
 class SignMessagesTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -37,5 +40,22 @@ class SignMessagesTest(BitcoinTestFramework):
         assert not self.nodes[0].verifymessage(other_address, signature, message)
         assert not self.nodes[0].verifymessage(address, other_signature, message)
 
+        self.log.info('test parameter validity and error codes')
+        # signmessage(withprivkey) have two required parameters
+        for num_params in [0, 1, 3, 4, 5]:
+            param_list = ["dummy"]*num_params
+            assert_raises_rpc_error(-1, "signmessagewithprivkey", self.nodes[0].signmessagewithprivkey, *param_list)
+            assert_raises_rpc_error(-1, "signmessage", self.nodes[0].signmessage, *param_list)
+        # verifymessage has three required parameters
+        for num_params in [0, 1, 2, 4, 5]:
+            param_list = ["dummy"]*num_params
+            assert_raises_rpc_error(-1, "verifymessage", self.nodes[0].verifymessage, *param_list)
+        # invalid key or address provided
+        assert_raises_rpc_error(-5, "Invalid private key", self.nodes[0].signmessagewithprivkey, "invalid_key", message)
+        assert_raises_rpc_error(-5, "Invalid address", self.nodes[0].signmessage, "invalid_addr", message)
+        assert_raises_rpc_error(-5, "Invalid address", self.nodes[0].verifymessage, "invalid_addr", signature, message)
+        # malformed signature provided
+        assert_raises_rpc_error(-3, "Malformed base64 encoding", self.nodes[0].verifymessage, self.nodes[0].getnewaddress(), "invalid_sig", message)
+
 if __name__ == '__main__':
     SignMessagesTest().main()

test/functional/test_framework/messages.py

@@ -1819,10 +1819,23 @@ class msg_headers2:
         return "msg_headers2(headers=%s)" % repr(self.headers)
 
 class msg_merkleblock:
+    __slots__ = ("merkleblock",)
     command = b"merkleblock"
 
+    def __init__(self, merkleblock=None):
+        if merkleblock is None:
+            self.merkleblock = CMerkleBlock()
+        else:
+            self.merkleblock = merkleblock
+
     def deserialize(self, f):
-        pass  # Placeholder for now
+        self.merkleblock.deserialize(f)
+
+    def serialize(self):
+        return self.merkleblock.serialize()
+
+    def __repr__(self):
+        return "msg_merkleblock(merkleblock=%s)" % (repr(self.merkleblock))
 
 
 class msg_filterload:
@@ -1858,9 +1871,9 @@ class msg_sendcmpct:
     __slots__ = ("announce", "version")
     command = b"sendcmpct"
 
-    def __init__(self):
-        self.announce = False
-        self.version = 1
+    def __init__(self, announce=False, version=1):
+        self.announce = announce
+        self.version = version
 
     def deserialize(self, f):
         self.announce = struct.unpack("<?", f.read(1))[0]

test/functional/test_framework/mininode.py

@@ -473,18 +473,17 @@ class P2PInterface(P2PConnection):
             last_headers = self.last_message.get('headers')
             if not last_headers:
                 return False
-            return last_headers.headers[0].rehash() == blockhash
+            return last_headers.headers[0].rehash() == int(blockhash, 16)
 
         self.wait_until(test_function, timeout=timeout)
 
-    def wait_for_merkleblock(self, timeout=60):
+    def wait_for_merkleblock(self, blockhash, timeout=60):
         def test_function():
             assert self.is_connected
             last_filtered_block = self.last_message.get('merkleblock')
             if not last_filtered_block:
                 return False
-            # TODO change this method to take a hash value and only return true if the correct block has been received
-            return True
+            return last_filtered_block.merkleblock.header.rehash() == int(blockhash, 16)
 
         wait_until(test_function, timeout=timeout, lock=mininode_lock)
 

test/functional/test_framework/test_node.py

@@ -542,7 +542,7 @@ class TestNode():
 
     def num_connected_mininodes(self):
         """Return number of test framework p2p connections to the node."""
-        return len([peer for peer in self.getpeerinfo() if peer['subver'] == MY_SUBVERSION])
+        return len([peer for peer in self.getpeerinfo() if peer['subver'] == MY_SUBVERSION.decode("utf-8")])
 
     def disconnect_p2ps(self):
         """Close all p2p connections to the node."""

test/functional/wallet_balance.py

@@ -204,8 +204,6 @@ class WalletTest(BitcoinTestFramework):
         self.log.info('Put txs back into mempool of node 1 (not node 0)')
         self.nodes[0].invalidateblock(block_reorg)
         self.nodes[1].invalidateblock(block_reorg)
-        self.sync_blocks()
-        self.nodes[0].syncwithvalidationinterfacequeue()
         assert_equal(self.nodes[0].getbalance(minconf=0), 0)  # wallet txs not in the mempool are untrusted
         self.nodes[0].generatetoaddress(1, ADDRESS_WATCHONLY)
         assert_equal(self.nodes[0].getbalance(minconf=0), 0)  # wallet txs not in the mempool are untrusted

test/functional/wallet_importmulti.py

@@ -63,7 +63,7 @@ class ImportMultiTest(BitcoinTestFramework):
         self.nodes[0].generate(1)
         self.nodes[1].generate(1)
         timestamp = self.nodes[1].getblock(self.nodes[1].getbestblockhash())['mediantime']
-        self.nodes[1].syncwithvalidationinterfacequeue()
+        self.nodes[1].syncwithvalidationinterfacequeue()  # Sync the timestamp to the wallet, so that importmulti works
 
         node0_address1 = self.nodes[0].getaddressinfo(self.nodes[0].getnewaddress())
 

test/functional/wallet_resendwallettransactions.py

@@ -52,6 +52,7 @@ class ResendWalletTransactionsTest(BitcoinTestFramework):
         block.solve()
         node.submitblock(ToHex(block))
 
+        # Set correct m_best_block_time, which is used in ResendWalletTransactions
         node.syncwithvalidationinterfacequeue()
 
         # Transaction should not be rebroadcast within first 12 hours

test/fuzz/test_runner.py

@@ -215,6 +215,8 @@ def merge_inputs(*, fuzz_pool, corpus, test_list, build_dir, merge_dir):
         args = [
             os.path.join(build_dir, 'src', 'test', 'fuzz', 'fuzz'),
             '-merge=1',
+            '-shuffle=0',
+            '-prefer_small=1',
             '-use_value_profile=1',  # Also done by oss-fuzz https://github.com/google/oss-fuzz/issues/1406#issuecomment-387790487
             os.path.join(corpus, t),
             os.path.join(merge_dir, t),