Merge #6050: backport: trivial 2024 06 07

6777ab73a2 Merge bitcoin-core/gui#682: Don't directly delete abandoned txes from GUI (Hennadii Stepanov)
6e1a8c1fdc Merge bitcoin/bitcoin#26628: RPC: Reject RPC requests with same named parameter specified multiple times (MarcoFalke)
7c28b01c78 Merge bitcoin/bitcoin#26666: refactor: Deleted unreachable code in httpserver.cpp (MarcoFalke)
478fe51ead Merge bitcoin/bitcoin#26100: doc: clarify that NetPermissionFlags::Implicit is only about whitelists (MarcoFalke)
69b19cbfc0 Merge bitcoin/bitcoin#26546: test: remove unused class `NodePongAdd1` (fanquake)
245df942c4 Merge bitcoin/bitcoin#26380: Revert "test: check importing wallets when blocks are pruned throw an error" (MacroFake)
3db3bd0d31 Merge bitcoin/bitcoin#24269: test: add functional test for `-discover` (Andrew Chow)
c72ef299da Merge bitcoin/bitcoin#25896: wallet: Log when Wallet::SetMinVersion sets a different minversion (Andrew Chow)
12b438cb46 Merge bitcoin/bitcoin#25925: doc: add `{import,list}descriptors` to list of descriptor RPCs (Andrew Chow)
73d64c48a9 Merge bitcoin/bitcoin#25738: depends: use a patch instead of sed in libxcb (fanquake)
1fae0c2bbb Merge bitcoin/bitcoin#25333: test: Fix out-of-range port collisions (MacroFake)
b75089667c Merge bitcoin/bitcoin#25231: ci: Install documented packages for "Win64" CI task (fanquake)

Pull request description:

  ## Issue being fixed or feature implemented
  Batch of trivial backports

  ## What was done?
  Trivial backports

  ## How Has This Been Tested?
  Built; ran tests locally

  ## Breaking Changes
  None

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [ ] 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 6777ab73a2

Tree-SHA512: 60d68c8d0fb9875d0b2421cad97b46a9d4deb79e03ca011d66cc0595ed68bb7ad207f2fd95973a3b7a0b9bca2c2f0deaf0e1116f0312a4d9d315f009228fe485

ci/test/00_setup_env_win64.sh

@@ -9,7 +9,7 @@ export LC_ALL=C.UTF-8
 export CONTAINER_NAME=ci_win64
 export HOST=x86_64-w64-mingw32
 export DPKG_ADD_ARCH="i386"
-export PACKAGES="python3 nsis g++-mingw-w64-x86-64 wine-binfmt wine64 wine32 file"
+export PACKAGES="python3 nsis g++-mingw-w64-x86-64-posix wine-binfmt wine64 wine32 file"
 export RUN_FUNCTIONAL_TESTS=false
 export RUN_SECURITY_TESTS="false"
 export GOAL="deploy"

depends/packages/libxcb.mk

@@ -4,6 +4,7 @@ $(package)_download_path=https://xcb.freedesktop.org/dist
 $(package)_file_name=$(package)-$($(package)_version).tar.xz
 $(package)_sha256_hash=a55ed6db98d43469801262d81dc2572ed124edc3db31059d4e9916eb9f844c34
 $(package)_dependencies=xcb_proto libXau
+$(package)_patches = remove_pthread_stubs.patch
 
 define $(package)_set_vars
 $(package)_config_opts=--disable-static --disable-devel-docs --without-doxygen --without-launchd
@@ -20,7 +21,7 @@ endef
 
 define $(package)_preprocess_cmds
   cp -f $(BASEDIR)/config.guess $(BASEDIR)/config.sub build-aux && \
-  sed "s/pthread-stubs//" -i configure
+  patch -p1 -i $($(package)_patch_dir)/remove_pthread_stubs.patch
 endef
 
 define $(package)_config_cmds

depends/patches/libxcb/remove_pthread_stubs.patch

@@ -0,0 +1,12 @@
+Remove uneeded pthread-stubs dependency
+--- a/configure
++++ b/configure
+@@ -19695,7 +19695,7 @@ fi
+ NEEDED="xau >= 0.99.2"
+ case $host_os in
+ linux*) ;;
+-     *) NEEDED="$NEEDED pthread-stubs" ;;
++     *) NEEDED="$NEEDED" ;;
+ esac
+ 
+ pkg_failed=no

doc/descriptors.md

@@ -11,13 +11,16 @@ Supporting RPCs are:
   addresses.
 - `listunspent` outputs a specialized descriptor for the reported unspent outputs.
 - `getaddressinfo` outputs a descriptor for solvable addresses (since v0.18).
-- `importmulti` takes as input descriptors to import into the wallet
+- `importmulti` takes as input descriptors to import into a legacy wallet
   (since v0.18).
 - `generatetodescriptor` takes as input a descriptor and generates coins to it
   (`regtest` only, since v0.19).
 - `utxoupdatepsbt` takes as input descriptors to add information to the psbt
   (since v0.19).
-- `createmultisig` and `addmultisigaddress` return descriptors as well (since v0.20)
+- `createmultisig` and `addmultisigaddress` return descriptors as well (since v0.20).
+- `importdescriptors` takes as input descriptors to import into a descriptor wallet
+  (since v0.21).
+- `listdescriptors` outputs descriptors imported into a descriptor wallet (since v22).
 
 This document describes the language. For the specifics on usage, see the RPC
 documentation for the functions mentioned above.

doc/release-notes-6050.md

@@ -0,0 +1,4 @@
+JSON-RPC
+---
+
+The JSON-RPC server now rejects requests where a parameter is specified multiple times with the same name, instead of silently overwriting earlier parameter values with later ones. (dash#6050)

src/httpserver.cpp

@@ -190,19 +190,16 @@ std::string RequestMethodString(HTTPRequest::RequestMethod m)
     switch (m) {
     case HTTPRequest::GET:
         return "GET";
-        break;
     case HTTPRequest::POST:
         return "POST";
-        break;
     case HTTPRequest::HEAD:
         return "HEAD";
-        break;
     case HTTPRequest::PUT:
         return "PUT";
-        break;
-    default:
+    case HTTPRequest::UNKNOWN:
         return "unknown";
-    }
+    } // no default case, so the compiler can warn about missing cases
+    assert(false);
 }
 
 /** HTTP request callback */
@@ -623,19 +620,14 @@ HTTPRequest::RequestMethod HTTPRequest::GetRequestMethod() const
     switch (evhttp_request_get_command(req)) {
     case EVHTTP_REQ_GET:
         return GET;
-        break;
     case EVHTTP_REQ_POST:
         return POST;
-        break;
     case EVHTTP_REQ_HEAD:
         return HEAD;
-        break;
     case EVHTTP_REQ_PUT:
         return PUT;
-        break;
     default:
         return UNKNOWN;
-        break;
     }
 }
 

src/net_permissions.h

@@ -34,7 +34,8 @@ enum class NetPermissionFlags : uint32_t {
     // unlimited amounts of addrs.
     Addr = (1U << 7),
 
-    // True if the user did not specifically set fine grained permissions
+    // True if the user did not specifically set fine-grained permissions with
+    // the -whitebind or -whitelist configuration options.
     Implicit = (1U << 31),
     All = BloomFilter | ForceRelay | Relay | NoBan | Mempool | Download | Addr,
 };

src/qt/transactionview.cpp

@@ -453,9 +453,6 @@ void TransactionView::abandonTx()
 
     // Abandon the wallet transaction over the walletModel
     model->wallet().abandonTransaction(hash);
-
-    // Update the table
-    model->getTransactionTableModel()->updateTransaction(hashQStr, CT_UPDATED, false);
 }
 
 void TransactionView::resendTx()

src/rpc/client.cpp

@@ -305,6 +305,9 @@ UniValue RPCConvertNamedValues(const std::string &strMethod, const std::vector<s
         std::string name = s.substr(0, pos);
         std::string value = s.substr(pos+1);
 
+        // Intentionally overwrite earlier named values with later ones as a
+        // convenience for scripts and command line users that want to merge
+        // options.
         if (!rpcCvtTable.convert(strMethod, name)) {
             // insert string value directly
             params.pushKV(name, value);
@@ -315,7 +318,10 @@ UniValue RPCConvertNamedValues(const std::string &strMethod, const std::vector<s
     }
 
     if (!positional_args.empty()) {
-        params.pushKV("args", positional_args);
+        // Use __pushKV instead of pushKV to avoid overwriting an explicit
+        // "args" value with an implicit one. Let the RPC server handle the
+        // request as given.
+        params.__pushKV("args", positional_args);
     }
 
     return params;

src/rpc/server.cpp

@@ -436,7 +436,10 @@ static inline JSONRPCRequest transformNamedArguments(const JSONRPCRequest& in, c
     const std::vector<UniValue>& values = in.params.getValues();
     std::unordered_map<std::string, const UniValue*> argsIn;
     for (size_t i=0; i<keys.size(); ++i) {
-        argsIn[keys[i]] = &values[i];
+        auto [_, inserted] = argsIn.emplace(keys[i], &values[i]);
+        if (!inserted) {
+            throw JSONRPCError(RPC_INVALID_PARAMETER, "Parameter " + keys[i] + " specified multiple times");
+        }
     }
     // Process expected parameters. If any parameters were left unspecified in
     // the request before a parameter that was specified, null values need to be

src/test/rpc_tests.cpp

@@ -85,11 +85,15 @@ BOOST_FIXTURE_TEST_SUITE(rpc_tests, RPCTestingSetup)
 
 BOOST_AUTO_TEST_CASE(rpc_namedparams)
 {
-    const std::vector<std::string> arg_names{{"arg1", "arg2", "arg3", "arg4", "arg5"}};
+    const std::vector<std::string> arg_names{"arg1", "arg2", "arg3", "arg4", "arg5"};
 
     // Make sure named arguments are transformed into positional arguments in correct places separated by nulls
     BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"arg2": 2, "arg4": 4})"), arg_names).write(), "[null,2,null,4]");
 
+    // Make sure named argument specified multiple times raises an exception
+    BOOST_CHECK_EXCEPTION(TransformParams(JSON(R"({"arg2": 2, "arg2": 4})"), arg_names), UniValue,
+                          HasJSON(R"({"code":-8,"message":"Parameter arg2 specified multiple times"})"));
+
     // Make sure named and positional arguments can be combined.
     BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"arg5": 5, "args": [1, 2], "arg4": 4})"), arg_names).write(), "[1,2,null,4,5]");
 
@@ -101,7 +105,7 @@ BOOST_AUTO_TEST_CASE(rpc_namedparams)
     BOOST_CHECK_EXCEPTION(TransformParams(JSON(R"({"args": [1,2,3], "arg4": 4, "arg2": 2})"), arg_names), UniValue,
                           HasJSON(R"({"code":-8,"message":"Parameter arg2 specified twice both as positional and named argument"})"));
 
-    // Make sure extra positional arguments can be passed through to the method implemenation, as long as they don't overlap with named arguments.
+    // Make sure extra positional arguments can be passed through to the method implementation, as long as they don't overlap with named arguments.
     BOOST_CHECK_EQUAL(TransformParams(JSON(R"({"args": [1,2,3,4,5,6,7,8,9,10]})"), arg_names).write(), "[1,2,3,4,5,6,7,8,9,10]");
     BOOST_CHECK_EQUAL(TransformParams(JSON(R"([1,2,3,4,5,6,7,8,9,10])"), arg_names).write(), "[1,2,3,4,5,6,7,8,9,10]");
 }

src/wallet/wallet.cpp

@@ -470,6 +470,7 @@ void CWallet::SetMinVersion(enum WalletFeature nVersion, WalletBatch* batch_in)
     LOCK(cs_wallet);
     if (nWalletVersion >= nVersion)
         return;
+    WalletLogPrintf("Setting minversion to %d\n", nVersion);
     nWalletVersion = nVersion;
 
     {

test/functional/feature_bind_extra.py

@@ -18,12 +18,12 @@ from test_framework.test_framework import (
     SkipTest,
 )
 from test_framework.util import (
-    PORT_MIN,
-    PORT_RANGE,
     assert_equal,
+    p2p_port,
     rpc_port,
 )
 
+
 class BindExtraTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
@@ -33,11 +33,6 @@ class BindExtraTest(BitcoinTestFramework):
         self.num_nodes = 2
 
     def setup_network(self):
-        # Override setup_network() because we want to put the result of
-        # p2p_port() in self.extra_args[], before the nodes are started.
-        # p2p_port() is not usable in set_test_params() because PortSeed.n is
-        # not set at that time.
-
         # Due to OS-specific network stats queries, we only run on Linux.
         self.log.info("Checking for Linux")
         if not sys.platform.startswith('linux'):
@@ -45,8 +40,8 @@ class BindExtraTest(BitcoinTestFramework):
 
         loopback_ipv4 = addr_to_hex("127.0.0.1")
 
-        # Start custom ports after p2p and rpc ports.
-        port = PORT_MIN + 2 * PORT_RANGE
+        # Start custom ports by reusing unused p2p ports
+        port = p2p_port(self.num_nodes)
 
         # Array of tuples [command line arguments, expected bind addresses].
         self.expected = []

test/functional/feature_discover.py

@@ -0,0 +1,75 @@
+#!/usr/bin/env python3
+# Copyright (c) 2022 The Bitcoin Core developers
+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""Test -discover command."""
+
+import socket
+
+from test_framework.test_framework import BitcoinTestFramework
+from test_framework.util import assert_equal
+
+
+def is_valid_ipv4_address(address):
+    try:
+        socket.inet_aton(address)
+    except socket.error:
+        return False
+    return True
+
+
+def is_valid_ipv6_address(address):
+    try:
+        socket.inet_pton(socket.AF_INET6, address)
+    except socket.error:
+        return False
+    return True
+
+
+class DiscoverTest(BitcoinTestFramework):
+    def set_test_params(self):
+        self.setup_clean_chain = True
+        self.bind_to_localhost_only = False
+        self.num_nodes = 1
+
+    def validate_addresses(self, addresses_obj):
+        for address_obj in addresses_obj:
+            address = address_obj['address']
+            self.log.info(f"Validating {address}")
+            valid = (is_valid_ipv4_address(address)
+                     or is_valid_ipv6_address(address))
+            assert_equal(valid, True)
+
+    def test_local_addresses(self, test_case, *, expect_empty=False):
+        self.log.info(f"Restart node with {test_case}")
+        self.restart_node(0, test_case)
+        network_info = self.nodes[0].getnetworkinfo()
+        network_enabled = [n for n in network_info['networks']
+                           if n['reachable'] and n['name'] in ['ipv4', 'ipv6']]
+        local_addrs = list(network_info["localaddresses"])
+        if expect_empty or not network_enabled:
+            assert_equal(local_addrs, [])
+        elif len(local_addrs) > 0:
+            self.validate_addresses(local_addrs)
+
+    def run_test(self):
+        test_cases = [
+            ["-listen", "-discover"],
+            ["-discover"],
+        ]
+
+        test_cases_empty = [
+            ["-discover=0"],
+            ["-listen", "-discover=0"],
+            [],
+        ]
+
+        for test_case in test_cases:
+            self.test_local_addresses(test_case, expect_empty=False)
+
+        for test_case in test_cases_empty:
+            self.test_local_addresses(test_case, expect_empty=True)
+
+
+if __name__ == '__main__':
+    DiscoverTest().main()

test/functional/feature_proxy.py

@@ -40,19 +40,15 @@ addnode connect to a CJDNS address
 """
 
 import socket
-import os
 
 from test_framework.socks5 import Socks5Configuration, Socks5Command, Socks5Server, AddressType
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
-    PORT_MIN,
-    PORT_RANGE,
     assert_equal,
+    p2p_port,
 )
 from test_framework.netutil import test_ipv6_local
 
-RANGE_BEGIN = PORT_MIN + 2 * PORT_RANGE  # Start after p2p and rpc ports
-
 # Networks returned by RPC getpeerinfo.
 NET_UNROUTABLE = "not_publicly_routable"
 NET_IPV4 = "ipv4"
@@ -75,19 +71,19 @@ class ProxyTest(BitcoinTestFramework):
         # Create two proxies on different ports
         # ... one unauthenticated
         self.conf1 = Socks5Configuration()
-        self.conf1.addr = ('127.0.0.1', RANGE_BEGIN + (os.getpid() % 1000))
+        self.conf1.addr = ('127.0.0.1', p2p_port(self.num_nodes))
         self.conf1.unauth = True
         self.conf1.auth = False
         # ... one supporting authenticated and unauthenticated (Tor)
         self.conf2 = Socks5Configuration()
-        self.conf2.addr = ('127.0.0.1', RANGE_BEGIN + 1000 + (os.getpid() % 1000))
+        self.conf2.addr = ('127.0.0.1', p2p_port(self.num_nodes + 1))
         self.conf2.unauth = True
         self.conf2.auth = True
         if self.have_ipv6:
             # ... one on IPv6 with similar configuration
             self.conf3 = Socks5Configuration()
             self.conf3.af = socket.AF_INET6
-            self.conf3.addr = ('::1', RANGE_BEGIN + 2000 + (os.getpid() % 1000))
+            self.conf3.addr = ('::1', p2p_port(self.num_nodes + 2))
             self.conf3.unauth = True
             self.conf3.auth = True
         else:

test/functional/feature_pruning.py

@@ -372,8 +372,6 @@ class PruneTest(BitcoinTestFramework):
         self.restart_node(2, extra_args=["-dip3params=2000:2000", "-dip8params=2000", "-disablegovernance", "-txindex=0", "-prune=550"], expected_stderr=EXPECTED_STDERR_NO_GOV_PRUNE)
         self.log.info("Success")
 
-        assert_raises_rpc_error(-4, "Importing wallets is disabled when blocks are pruned", self.nodes[2].importwallet, "abc")
-
         # check that wallet loads successfully when restarting a pruned node after IBD.
         # this was reported to fail in #7494.
         self.log.info("Syncing node 5 to test wallet")

test/functional/interface_bitcoin_cli.py

@@ -89,6 +89,10 @@ class TestBitcoinCli(BitcoinTestFramework):
         assert_raises_rpc_error(-8, "Parameter arg1 specified twice both as positional and named argument", self.nodes[0].cli.echo, 0, 1, arg1=1)
         assert_raises_rpc_error(-8, "Parameter arg1 specified twice both as positional and named argument", self.nodes[0].cli.echo, 0, None, 2, arg1=1)
 
+        self.log.info("Test that later cli named arguments values silently overwrite earlier ones")
+        assert_equal(self.nodes[0].cli("-named", "echo", "arg0=0", "arg1=1", "arg2=2", "arg1=3").send_cli(), ['0', '3', '2'])
+        assert_raises_rpc_error(-8, "Parameter args specified multiple times", self.nodes[0].cli("-named", "echo", "args=[0,1,2,3]", "4", "5", "6", ).send_cli)
+
         user, password = get_auth_cookie(self.nodes[0].datadir, self.chain)
 
         self.log.info("Test -stdinrpcpass option")

test/functional/p2p_getaddr_caching.py

@@ -42,8 +42,8 @@ class AddrTest(BitcoinTestFramework):
     def set_test_params(self):
         self.num_nodes = 1
         # Use some of the remaining p2p ports for the onion binds.
-        self.onion_port1 = p2p_port(1)
-        self.onion_port2 = p2p_port(2)
+        self.onion_port1 = p2p_port(self.num_nodes)
+        self.onion_port2 = p2p_port(self.num_nodes + 1)
         self.extra_args = [
             [f"-bind=127.0.0.1:{self.onion_port1}=onion", f"-bind=127.0.0.1:{self.onion_port2}=onion"],
         ]

test/functional/p2p_ping.py

@@ -12,7 +12,9 @@ from test_framework.p2p import P2PInterface
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import assert_equal
 
+
 PING_INTERVAL = 2 * 60
+TIMEOUT_INTERVAL = 20 * 60
 
 
 class msg_pong_corrupt(msg_pong):
@@ -20,19 +22,11 @@ class msg_pong_corrupt(msg_pong):
         return b""
 
 
-class NodePongAdd1(P2PInterface):
-    def on_ping(self, message):
-        self.send_message(msg_pong(message.nonce + 1))
-
-
 class NodeNoPong(P2PInterface):
     def on_ping(self, message):
         pass
 
 
-TIMEOUT_INTERVAL = 20 * 60
-
-
 class PingPongTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True

test/functional/test_runner.py

@@ -284,6 +284,7 @@ BASE_SCRIPTS = [
     'feature_governance_objects.py',
     'feature_governance.py --legacy-wallet',
     'rpc_uptime.py',
+    'feature_discover.py',
     'wallet_resendwallettransactions.py --legacy-wallet',
     'wallet_resendwallettransactions.py --descriptors',
     'wallet_fallbackfee.py --legacy-wallet',