From 49c87e93a622cb127e91848a08b6574826f8b777 Mon Sep 17 00:00:00 2001 From: "W. J. van der Laan" Date: Fri, 1 Oct 2021 10:30:26 +0200 Subject: [PATCH 1/4] Merge bitcoin/bitcoin#23142: Return false on corrupt tx rather than asserting 0ab4c3b27265401c59e40adc494041927dc9dbe3 Return false on corrupt tx rather than asserting (Samuel Dobson) Pull request description: Takes up #19793 Rather than asserting, we log an error and return CORRUPT so that the user is informed. This type of error isn't critical so it isn't worth `assert`ing. ACKs for top commit: achow101: ACK 0ab4c3b27265401c59e40adc494041927dc9dbe3 laanwj: Code review ACK 0ab4c3b27265401c59e40adc494041927dc9dbe3 ryanofsky: Code review ACK 0ab4c3b27265401c59e40adc494041927dc9dbe3. There may be room for more improvements later like better error messages or easier recovery options, but changing from an assert to an error seems like a clear improvement, and this seems to avoid all the pitfalls of the last PR that tried this. Tree-SHA512: 4a1a412e7c473d176c4e09123b85f390a6b0ea195e78d28ebd50b13814b7852f8225a172511a2efb6affb555b11bd4e667c19eb8c78b060c5444b62f0fae5f7a --- src/wallet/walletdb.cpp | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/src/wallet/walletdb.cpp b/src/wallet/walletdb.cpp index 9042bd25bc..af8b041ad2 100644 --- a/src/wallet/walletdb.cpp +++ b/src/wallet/walletdb.cpp @@ -324,6 +324,7 @@ public: std::map m_descriptor_caches; std::map, CKey> m_descriptor_keys; std::map, std::pair>> 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. From 5a441b38defff1837e5872432734e16b55923d58 Mon Sep 17 00:00:00 2001 From: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com> Date: Wed, 6 Oct 2021 21:29:46 +0300 Subject: [PATCH 2/4] (partial) Merge bitcoin-core/gui#409: Fix window title of wallet loading window 01bff8f0494ba1c02c087b7186c892785f326fd9 qt: Fix WalletControllerActivity progress dialog title (Shashwat) Pull request description: Throughout the GUI, the title of the window, tells about the purpose of the window. This was not true for the title of wallet loading wallet. This PR fixes this issue by renaming the wallet loading window title to 'Open Wallet' Changes introduced in this PR (Runned Bitcoin-GUI on signet network) |Master|PR| |---|---| |![Screenshot from 2021-08-24 00-02-18](https://user-images.githubusercontent.com/85434418/130500309-2f0af2c9-55f0-4609-a92b-3156800fa92e.png)|![Screenshot from 2021-09-07 18-19-10](https://user-images.githubusercontent.com/85434418/132351394-1ee4a36c-3ba9-4d1a-a8f3-f17804fb856a.png)| ACKs for top commit: jarolrod: ACK 01bff8f hebasto: ACK 01bff8f0494ba1c02c087b7186c892785f326fd9, tested on Linux Mint 20.2 (Qt 5.12.8). Tree-SHA512: cd21c40752eb1c0afb5ec61b8a40e900bc3aa05749963f7957ece6024e4957f5bb37e0eb4f95aac488f5e08aea51fe13b023b05d8302a08c88dcc6790410ba64 --- src/qt/walletcontroller.cpp | 17 ++++++++++++++--- src/qt/walletcontroller.h | 2 +- 2 files changed, 15 insertions(+), 4 deletions(-) diff --git a/src/qt/walletcontroller.cpp b/src/qt/walletcontroller.cpp index d8c60b9e93..2133999b9b 100644 --- a/src/qt/walletcontroller.cpp +++ b/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 %1…").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 %1…").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 %1…").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 %1…").arg(name.toHtmlEscaped())); QTimer::singleShot(0, worker(), [this, path] { std::unique_ptr wallet = node().walletLoader().loadWallet(path, m_error_message, m_warning_message); diff --git a/src/qt/walletcontroller.h b/src/qt/walletcontroller.h index 07c781791f..f65455e7c8 100644 --- a/src/qt/walletcontroller.h +++ b/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; From 2b71a9b030363d10f4d0e7b1f5453a054fbb5792 Mon Sep 17 00:00:00 2001 From: MarcoFalke Date: Mon, 13 Dec 2021 17:13:28 +0100 Subject: [PATCH 3/4] Merge bitcoin/bitcoin#23755: rpc: Quote user supplied strings in error messages fa24a3df8796cbf4eeb35d950a4c848d605e5b22 rpc: Quote user supplied strings in error messages (MarcoFalke) Pull request description: I can't see a downside doing this and this fixes a fuzzing crash Background: This is a follow-up to commit 926fc2a0d4ff64cf2ff8e1dfa64eca2ebd24e090, which introduced the "starts_with-hack". Maybe an alternative to the hack would be to assign a unique error code to internal bugs? However, I think this can be done in an separate pull request and the changes here make sense even on their own. ACKs for top commit: fanquake: ACK fa24a3df8796cbf4eeb35d950a4c848d605e5b22 - to fix the fuzzers. Tree-SHA512: d998626406a64396a037a6d1fce22fce3dadb7567c2f9638e450ebe8fb8ae77d134e15dd02555326732208f698d77b0028bc62be9ceee9c43282b61fe95fccbd --- src/rpc/blockchain.cpp | 4 ++-- src/rpc/util.cpp | 2 +- test/functional/rpc_blockchain.py | 2 +- test/functional/rpc_getblockstats.py | 8 ++++---- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/src/rpc/blockchain.cpp b/src/rpc/blockchain.cpp index 16055175fb..28d075aaba 100644 --- a/src/rpc/blockchain.cpp +++ b/src/rpc/blockchain.cpp @@ -1377,7 +1377,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)); } } @@ -2520,7 +2520,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); } diff --git a/src/rpc/util.cpp b/src/rpc/util.cpp index 3b02da632f..3741a1b9a9 100644 --- a/src/rpc/util.cpp +++ b/src/rpc/util.cpp @@ -255,7 +255,7 @@ CPubKey AddrToPubKey(const FillableSigningProvider& keystore, const std::string& } const PKHash *pkhash = std::get_if(&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)) { diff --git a/test/functional/rpc_blockchain.py b/test/functional/rpc_blockchain.py index fedeb1f3d8..fb1c019769 100755 --- a/test/functional/rpc_blockchain.py +++ b/test/functional/rpc_blockchain.py @@ -299,7 +299,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") diff --git a/test/functional/rpc_getblockstats.py b/test/functional/rpc_getblockstats.py index ebcf4e2ebb..19295b70e5 100755 --- a/test/functional/rpc_getblockstats.py +++ b/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') From 71689fe6dcfce9f7fe42e75767009f98fa5850e2 Mon Sep 17 00:00:00 2001 From: fanquake Date: Thu, 18 Nov 2021 07:37:28 +0800 Subject: [PATCH 4/4] (partial) Merge bitcoin/bitcoin#22981: doc: Fix incorrect C++ named args fac49470ca36ff944a613f4358386bf8e0967427 doc: Fix incorrect C++ named args (MarcoFalke) Pull request description: Incorrect named args are source of bugs, like #22979. Fix that by correcting them and adjust the format, so that clang-tidy can check it. ACKs for top commit: fanquake: ACK fac49470ca36ff944a613f4358386bf8e0967427 - `run-clang-tidy` works for me now. Tree-SHA512: 2694e17a1586394baf30bbc479a913e4bad361221e8470b8739caf30aacea736befc73820f3fe56f6207d9f5d969323278d43a647f58c3497e8e44cad79f8934 --- src/bench/rpc_mempool.cpp | 2 +- src/net.cpp | 2 +- src/net_processing.cpp | 2 +- src/qt/test/test_main.cpp | 2 +- src/rpc/mining.cpp | 2 +- src/rpc/rawtransaction.cpp | 4 ++-- src/test/denialofservice_tests.cpp | 4 ++-- src/test/fuzz/tx_pool.cpp | 2 +- src/test/validation_block_tests.cpp | 2 +- 9 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/bench/rpc_mempool.cpp b/src/bench/rpc_mempool.cpp index 013f56342e..a874fab0f7 100644 --- a/src/bench/rpc_mempool.cpp +++ b/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) diff --git a/src/net.cpp b/src/net.cpp index 4fcbbfb3c1..e8645f9b83 100644 --- a/src/net.cpp +++ b/src/net.cpp @@ -4279,7 +4279,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, diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 3d134aa7fa..1ae3eaba36 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -5060,7 +5060,7 @@ bool PeerManagerImpl::ProcessMessages(CNode* pfrom, std::atomic& 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()); diff --git a/src/qt/test/test_main.cpp b/src/qt/test/test_main.cpp index 78d8331eb3..34ea983441 100644 --- a/src/qt/test/test_main.cpp +++ b/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; diff --git a/src/rpc/mining.cpp b/src/rpc/mining.cpp index 026ceb2fc2..9a41712b0a 100644 --- a/src/rpc/mining.cpp +++ b/src/rpc/mining.cpp @@ -1060,7 +1060,7 @@ static RPCHelpMan submitblock() bool new_block; auto sc = std::make_shared(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"; diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index d3c37f7345..208529414e 100644 --- a/src/rpc/rawtransaction.cpp +++ b/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. diff --git a/src/test/denialofservice_tests.cpp b/src/test/denialofservice_tests.cpp index d34f15b78e..408a112ff6 100644 --- a/src/test/denialofservice_tests.cpp +++ b/src/test/denialofservice_tests.cpp @@ -330,7 +330,7 @@ BOOST_AUTO_TEST_CASE(peer_discouragement) /*nKeyedNetGroupIn=*/1, /*nLocalHostNonceIn=*/1, CAddress(), - /*pszDest=*/"", + /*addrNameIn=*/"", ConnectionType::INBOUND, /*inbound_onion=*/false}; nodes[1]->SetCommonVersion(PROTOCOL_VERSION); @@ -361,7 +361,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); diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index cbdffe29d8..c689b52cc6 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/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 all_txids; tx_pool.queryHashes(all_txids); assert(all_txids.size() < info_all.size()); diff --git a/src/test/validation_block_tests.cpp b/src/test/validation_block_tests.cpp index 352421a815..e532da6289 100644 --- a/src/test/validation_block_tests.cpp +++ b/src/test/validation_block_tests.cpp @@ -221,7 +221,7 @@ BOOST_AUTO_TEST_CASE(mempool_locks_reorg) { bool ignored; auto ProcessBlock = [&](std::shared_ptr 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