Merge #6264: backport: Merge bitcoin#23142, (partial) bitcoin-core/gui#409, 23333, 23755, (partial) 22981

71689fe6dc (partial) Merge bitcoin/bitcoin#22981: doc: Fix incorrect C++ named args (fanquake)
2b71a9b030 Merge bitcoin/bitcoin#23755: rpc: Quote user supplied strings in error messages (MarcoFalke)
5a441b38de (partial) Merge bitcoin-core/gui#409: Fix window title of wallet loading window (Hennadii Stepanov)
49c87e93a6 Merge bitcoin/bitcoin#23142: Return false on corrupt tx rather than asserting (W. J. van der Laan)

Pull request description:

  backports from bitcoin

ACKs for top commit:
  UdjinM6:
    utACK 71689fe6dc
  knst:
    utACK 71689fe6dc

Tree-SHA512: c68e2a1be5669f4fd8b02001ea81310b41fcac2cc5cc660e67b5140b334669c9a071a4bd5b33232580215607f323af5f87218a3465493675a633e112984296eb

src/bench/rpc_mempool.cpp

@@ -12,7 +12,7 @@
 static void AddTx(const CTransactionRef& tx, const CAmount& fee, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs)
 {
     LockPoints lp;
-    pool.addUnchecked(CTxMemPoolEntry(tx, fee, /* time */ 0, /* height */ 1, /* spendsCoinbase */ false, /* sigOps */ 1, lp));
+    pool.addUnchecked(CTxMemPoolEntry(tx, fee, /*time=*/0, /*entry_height=*/1, /*spends_coinbase=*/false, /*sigops*/1, lp));
 }
 
 static void RpcMempool(benchmark::Bench& bench)

src/net.cpp

@@ -4789,7 +4789,7 @@ void CConnman::PushMessage(CNode* pnode, CSerializedNetMsg&& msg)
     size_t nMessageSize = msg.data.size();
     LogPrint(BCLog::NET, "sending %s (%d bytes) peer=%d\n", msg.m_type, nMessageSize, pnode->GetId());
     if (gArgs.GetBoolArg("-capturemessages", false)) {
-        CaptureMessage(pnode->addr, msg.m_type, msg.data, /* incoming */ false);
+        CaptureMessage(pnode->addr, msg.m_type, msg.data, /*is_incoming=*/false);
     }
 
     TRACE6(net, outbound_message,

src/net_processing.cpp

@@ -5094,7 +5094,7 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic<bool>& interrupt
     );
 
     if (gArgs.GetBoolArg("-capturemessages", false)) {
-        CaptureMessage(pfrom->addr, msg.m_type, MakeUCharSpan(msg.m_recv), /* incoming */ true);
+        CaptureMessage(pfrom->addr, msg.m_type, MakeUCharSpan(msg.m_recv), /* is_incoming */ true);
     }
 
     msg.SetVersion(pfrom->GetCommonVersion());

src/qt/test/test_main.cpp

@@ -74,7 +74,7 @@ int main(int argc, char* argv[])
     #if defined(WIN32)
         if (getenv("QT_QPA_PLATFORM") == nullptr) _putenv_s("QT_QPA_PLATFORM", "minimal");
     #else
-        setenv("QT_QPA_PLATFORM", "minimal", /* overwrite */ 0);
+        setenv("QT_QPA_PLATFORM", "minimal", 0 /* overwrite */);
     #endif
 
     BitcoinApplication app;

src/qt/walletcontroller.cpp

@@ -198,11 +198,12 @@ WalletControllerActivity::~WalletControllerActivity()
     delete m_progress_dialog;
 }
 
-void WalletControllerActivity::showProgressDialog(const QString& label_text)
+void WalletControllerActivity::showProgressDialog(const QString& title_text, const QString& label_text)
 {
     assert(!m_progress_dialog);
     m_progress_dialog = new QProgressDialog(m_parent_widget);
 
+    m_progress_dialog->setWindowTitle(title_text);
     m_progress_dialog->setLabelText(label_text);
     m_progress_dialog->setRange(0, 0);
     m_progress_dialog->setCancelButton(nullptr);
@@ -251,7 +252,12 @@ void CreateWalletActivity::askPassphrase()
 
 void CreateWalletActivity::createWallet()
 {
-    showProgressDialog(tr("Creating Wallet <b>%1</b>…").arg(m_create_wallet_dialog->walletName().toHtmlEscaped()));
+    showProgressDialog(
+        //: Title of window indicating the progress of creation of a new wallet.
+        tr("Create Wallet"),
+        /*: Descriptive text of the create wallet progress window which indicates
+            to the user which wallet is currently being created. */
+        tr("Creating Wallet <b>%1</b>…").arg(m_create_wallet_dialog->walletName().toHtmlEscaped()));
 
     std::string name = m_create_wallet_dialog->walletName().toStdString();
     uint64_t flags = 0;
@@ -334,7 +340,12 @@ void OpenWalletActivity::open(const std::string& path)
 {
     QString name = path.empty() ? QString("["+tr("default wallet")+"]") : QString::fromStdString(path);
 
-    showProgressDialog(tr("Opening Wallet <b>%1</b>…").arg(name.toHtmlEscaped()));
+    showProgressDialog(
+        //: Title of window indicating the progress of opening of a wallet.
+        tr("Open Wallet"),
+        /*: Descriptive text of the open wallet progress window which indicates
+            to the user which wallet is currently being opened. */
+        tr("Opening Wallet <b>%1</b>…").arg(name.toHtmlEscaped()));
 
     QTimer::singleShot(0, worker(), [this, path] {
         std::unique_ptr<interfaces::Wallet> wallet = node().walletLoader().loadWallet(path, m_error_message, m_warning_message);

src/qt/walletcontroller.h

@@ -98,7 +98,7 @@ protected:
     interfaces::Node& node() const { return m_wallet_controller->m_node; }
     QObject* worker() const { return m_wallet_controller->m_activity_worker; }
 
-    void showProgressDialog(const QString& label_text);
+    void showProgressDialog(const QString& title_text, const QString& label_text);
     void destroyProgressDialog();
 
     WalletController* const m_wallet_controller;

src/rpc/blockchain.cpp

@@ -1378,7 +1378,7 @@ CoinStatsHashType ParseHashType(const std::string& hash_type_input)
     } else if (hash_type_input == "none") {
         return CoinStatsHashType::NONE;
     } else {
-        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("%s is not a valid hash_type", hash_type_input));
+        throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("'%s' is not a valid hash_type", hash_type_input));
     }
 }
 
@@ -2524,7 +2524,7 @@ static RPCHelpMan getblockstats()
     for (const std::string& stat : stats) {
         const UniValue& value = ret_all[stat];
         if (value.isNull()) {
-            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid selected statistic %s", stat));
+            throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid selected statistic '%s'", stat));
         }
         ret.pushKV(stat, value);
     }

src/rpc/mining.cpp

@@ -1060,7 +1060,7 @@ static RPCHelpMan submitblock()
     bool new_block;
     auto sc = std::make_shared<submitblock_StateCatcher>(block.GetHash());
     RegisterSharedValidationInterface(sc);
-    bool accepted = chainman.ProcessNewBlock(Params(), blockptr, /* fForceProcessing */ true, /* fNewBlock */ &new_block);
+    bool accepted = chainman.ProcessNewBlock(Params(), blockptr, /*force_processing=*/true, /*new_block=*/&new_block);
     UnregisterSharedValidationInterface(sc);
     if (!new_block && accepted) {
         return "duplicate";

src/rpc/rawtransaction.cpp

@@ -1849,7 +1849,7 @@ static RPCHelpMan utxoupdatepsbt()
         }
     }
     // We don't actually need private keys further on; hide them as a precaution.
-    HidingSigningProvider public_provider(&provider, /* nosign */ true, /* nobip32derivs */ false);
+    HidingSigningProvider public_provider(&provider, /*hide_secret=*/true, /*hide_origin=*/false);
 
     // Fetch previous transactions (inputs):
     CCoinsView viewDummy;
@@ -1881,7 +1881,7 @@ static RPCHelpMan utxoupdatepsbt()
         // Update script/keypath information using descriptor data.
         // Note that SignPSBTInput does a lot more than just constructing ECDSA signatures
         // we don't actually care about those here, in fact.
-        SignPSBTInput(public_provider, psbtx, i, /* sighash_type */ SIGHASH_ALL);
+        SignPSBTInput(public_provider, psbtx, i, /*sighash=*/ SIGHASH_ALL);
     }
 
     // Update script/keypath information using descriptor data.

src/rpc/util.cpp

@@ -255,7 +255,7 @@ CPubKey AddrToPubKey(const FillableSigningProvider& keystore, const std::string&
     }
     const PKHash *pkhash = std::get_if<PKHash>(&dest);
     if (!pkhash) {
-        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("%s does not refer to a key", addr_in));
+        throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, strprintf("'%s' does not refer to a key", addr_in));
     }
     CPubKey vchPubKey;
     if (!keystore.GetPubKey(ToKeyID(*pkhash), vchPubKey)) {

src/test/denialofservice_tests.cpp

@@ -329,7 +329,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
                          /*nKeyedNetGroupIn=*/1,
                          /*nLocalHostNonceIn=*/1,
                          CAddress(),
-                         /*pszDest=*/"",
+                         /*addrNameIn=*/"",
                          ConnectionType::INBOUND,
                          /*inbound_onion=*/false};
     nodes[1]->SetCommonVersion(PROTOCOL_VERSION);
@@ -360,7 +360,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement)
                          /*nKeyedNetGroupIn=*/1,
                          /*nLocalHostNonceIn=*/1,
                          CAddress(),
-                         /*pszDest=*/"",
+                         /*addrNameIn=*/"",
                          ConnectionType::OUTBOUND_FULL_RELAY,
                          /*inbound_onion=*/false};
     nodes[2]->SetCommonVersion(PROTOCOL_VERSION);

src/test/fuzz/tx_pool.cpp

@@ -93,7 +93,7 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, con
     const auto info_all = tx_pool.infoAll();
     if (!info_all.empty()) {
         const auto& tx_to_remove = *PickValue(fuzzed_data_provider, info_all).tx;
-        WITH_LOCK(tx_pool.cs, tx_pool.removeRecursive(tx_to_remove, /* dummy */ MemPoolRemovalReason::BLOCK));
+        WITH_LOCK(tx_pool.cs, tx_pool.removeRecursive(tx_to_remove, MemPoolRemovalReason::BLOCK /* dummy */));
         std::vector<uint256> all_txids;
         tx_pool.queryHashes(all_txids);
         assert(all_txids.size() < info_all.size());

src/test/validation_block_tests.cpp

@@ -221,7 +221,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg)
 {
     bool ignored;
     auto ProcessBlock = [&](std::shared_ptr<const CBlock> block) -> bool {
-        return Assert(m_node.chainman)->ProcessNewBlock(Params(), block, /* fForceProcessing */ true, /* fNewBlock */ &ignored);
+        return Assert(m_node.chainman)->ProcessNewBlock(Params(), block, /*force_processing=*/true, /*new_block=*/&ignored);
     };
 
     // Process all mined blocks

src/wallet/walletdb.cpp

@@ -324,6 +324,7 @@ public:
     std::map<uint256, DescriptorCache> m_descriptor_caches;
     std::map<std::pair<uint256, CKeyID>, CKey> m_descriptor_keys;
     std::map<std::pair<uint256, CKeyID>, std::pair<CPubKey, std::vector<unsigned char>>> m_descriptor_crypt_keys;
+    bool tx_corrupt{false};
 
     CWalletScanState() {
     }
@@ -358,7 +359,13 @@ ReadKeyValue(CWallet* pwallet, CDataStream& ssKey, CDataStream& ssValue,
             // LoadToWallet call below creates a new CWalletTx that fill_wtx
             // callback fills with transaction metadata.
             auto fill_wtx = [&](CWalletTx& wtx, bool new_tx) {
-                assert(new_tx);
+                if(!new_tx) {
+                    // There's some corruption here since the tx we just tried to load was already in the wallet.
+                    // We don't consider this type of corruption critical, and can fix it by removing tx data and
+                    // rescanning.
+                    wss.tx_corrupt = true;
+                    return false;
+                }
                 ssValue >> wtx;
                 if (wtx.GetHash() != hash)
                     return false;
@@ -797,6 +804,11 @@ DBErrors WalletBatch::LoadWallet(CWallet* pwallet)
                 } else if (strType == DBKeys::FLAGS) {
                     // reading the wallet flags can only fail if unknown flags are present
                     result = DBErrors::TOO_NEW;
+                } else if (wss.tx_corrupt) {
+                    pwallet->WalletLogPrintf("Error: Corrupt transaction found. This can be fixed by removing transactions from wallet and rescanning.\n");
+                    // Set tx_corrupt back to false so that the error is only printed once (per corrupt tx)
+                    wss.tx_corrupt = false;
+                    result = DBErrors::CORRUPT;
                 } else {
                     // Leave other errors alone, if we try to fix them we might make things worse.
                     fNoncriticalErrors = true; // ... but do warn the user there is something wrong.

test/functional/rpc_blockchain.py

@@ -300,7 +300,7 @@ class BlockchainTest(BitcoinTestFramework):
             assert 'muhash' not in r
 
         # Unknown hash_type raises an error
-        assert_raises_rpc_error(-8, "foohash is not a valid hash_type", node.gettxoutsetinfo, "foohash")
+        assert_raises_rpc_error(-8, "'foo hash' is not a valid hash_type", node.gettxoutsetinfo, "foo hash")
 
     def _test_getblockheader(self):
         self.log.info("Test getblockheader")

test/functional/rpc_getblockstats.py

@@ -143,17 +143,17 @@ class GetblockstatsTest(BitcoinTestFramework):
         inv_sel_stat = 'asdfghjkl'
         inv_stats = [
             [inv_sel_stat],
-            ['minfee' , inv_sel_stat],
+            ['minfee', inv_sel_stat],
             [inv_sel_stat, 'minfee'],
             ['minfee', inv_sel_stat, 'maxfee'],
         ]
         for inv_stat in inv_stats:
-            assert_raises_rpc_error(-8, 'Invalid selected statistic %s' % inv_sel_stat,
+            assert_raises_rpc_error(-8, f"Invalid selected statistic '{inv_sel_stat}'",
                                     self.nodes[0].getblockstats, hash_or_height=1, stats=inv_stat)
 
         # Make sure we aren't always returning inv_sel_stat as the culprit stat
-        assert_raises_rpc_error(-8, 'Invalid selected statistic aaa%s' % inv_sel_stat,
-                                self.nodes[0].getblockstats, hash_or_height=1, stats=['minfee' , 'aaa%s' % inv_sel_stat])
+        assert_raises_rpc_error(-8, f"Invalid selected statistic 'aaa{inv_sel_stat}'",
+                                self.nodes[0].getblockstats, hash_or_height=1, stats=['minfee', f'aaa{inv_sel_stat}'])
         # Mainchain's genesis block shouldn't be found on regtest
         assert_raises_rpc_error(-5, 'Block not found', self.nodes[0].getblockstats,
                                 hash_or_height='000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f')