From c97d924880eaad136c5f7776f05bf887657ccca7 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Wed, 21 Sep 2022 09:17:45 -0400 Subject: Correct sanity-checking script_size calculation GitHub-Pull: #26149 Rebased-From: 648f6950cd8d9ac767d76a1e302f37c611936a7a --- src/script/miniscript.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/script/miniscript.h b/src/script/miniscript.h index ab25fa67b7..c4f41e0adf 100644 --- a/src/script/miniscript.h +++ b/src/script/miniscript.h @@ -1221,7 +1221,7 @@ inline NodeRef Parse(Span in, const Ctx& ctx) // n = 1 here because we read the first WRAPPED_EXPR before reaching THRESH to_parse.emplace_back(ParseContext::THRESH, 1, k); to_parse.emplace_back(ParseContext::WRAPPED_EXPR, -1, -1); - script_size += 2 + (k > 16); + script_size += 2 + (k > 16) + (k > 0x7f) + (k > 0x7fff) + (k > 0x7fffff); } else if (Const("andor(", in)) { to_parse.emplace_back(ParseContext::ANDOR, -1, -1); to_parse.emplace_back(ParseContext::CLOSE_BRACKET, -1, -1); -- cgit v1.2.3 From 7e0bcfbfef61cb688bc92a96003c1219cad67935 Mon Sep 17 00:00:00 2001 From: Larry Ruane Date: Sat, 24 Sep 2022 00:00:17 -0600 Subject: p2p: ProcessHeadersMessage(): fix received_new_header Follow-up to #25717. The commit "Utilize anti-DoS headers download strategy" changed how this bool variable is computed, so that its value is now the opposite of what it should be. GitHub-Pull: #26172 Rebased-From: bdcafb913398f0cdaff9c880618f9ebfc85c7693 --- src/net_processing.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'src') diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 74700580ad..1c44ec9903 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -2843,7 +2843,7 @@ void PeerManagerImpl::ProcessHeadersMessage(CNode& pfrom, Peer& peer, // If we don't have the last header, then this peer will have given us // something new (if these headers are valid). - bool received_new_header{last_received_header != nullptr}; + bool received_new_header{last_received_header == nullptr}; // Now process all the headers. BlockValidationState state; -- cgit v1.2.3 From 5ad82a09b409d416236092062a4201e238dfd68b Mon Sep 17 00:00:00 2001 From: Ryan Ofsky Date: Fri, 30 Sep 2022 08:44:04 -0400 Subject: index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability Since commit f08c9fb0c6a799e3cb75ca5f763a746471625beb from PR https://github.com/bitcoin/bitcoin/pull/21726, index `BlockUntilSyncedToCurrentChain` behavior has been less reliable, and there has also been a race condition in the `coinstatsindex_initial_sync` unit test. It seems better for `BlockUntilSyncedToCurrentChain` to actually wait for the last connected block to be fully processed, than to be able to return before prune locks are set, so this switches the order of `m_best_block_index = block;` and `UpdatePruneLock` statements in `SetBestBlockIndex` to make it more reliable. Also since commit f08c9fb0c6a799e3cb75ca5f763a746471625beb, there has been a race condition in the `coinstatsindex_initial_sync` test. Before that commit, the atomic index best block pointer `m_best_block_index` was updated as the last step of `BaseIndex::BlockConnected`, so `BlockUntilSyncedToCurrentChain` could safely be used in tests to wait for the last `BlockConnected` notification to be finished before stopping and destroying the index. But after that commit, calling `BlockUntilSyncedToCurrentChain` is no longer sufficient, and there is a race between the test shutdown code which destroys the index object and the new code introduced in that commit calling `AllowPrune()` and `GetName()` on the index object. Reproducibility instructions for this are in https://github.com/bitcoin/bitcoin/issues/25365#issuecomment-1259744133 This commit fixes the `coinstatsindex_initial_sync` race condition, even though it will require an additional change to silence TSAN false positives, https://github.com/bitcoin/bitcoin/pull/26188, after it is fixed. So this partially addresses but does not resolve the bug reporting TSAN errors https://github.com/bitcoin/bitcoin/issues/25365. There is no known race condition outside of test code currently, because the bitcoind `Shutdown` function calls `FlushBackgroundCallbacks` not `BlockUntilSyncedToCurrentChain` to safely shut down. Co-authored-by: Vasil Dimov Co-authored-by: MacroFake Github-Pull: #26215 Rebased-From: 8891949bdcb25093d3a6703ae8228c3c3687d3a4 --- src/index/base.cpp | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) (limited to 'src') diff --git a/src/index/base.cpp b/src/index/base.cpp index 88c2ce98fa..3eea09b17d 100644 --- a/src/index/base.cpp +++ b/src/index/base.cpp @@ -298,6 +298,10 @@ void BaseIndex::BlockConnected(const std::shared_ptr& block, const } interfaces::BlockInfo block_info = kernel::MakeBlockInfo(pindex, block.get()); if (CustomAppend(block_info)) { + // Setting the best block index is intentionally the last step of this + // function, so BlockUntilSyncedToCurrentChain callers waiting for the + // best block index to be updated can rely on the block being fully + // processed, and the index object being safe to delete. SetBestBlockIndex(pindex); } else { FatalError("%s: Failed to write block %s to index", @@ -414,10 +418,17 @@ IndexSummary BaseIndex::GetSummary() const void BaseIndex::SetBestBlockIndex(const CBlockIndex* block) { assert(!node::fPruneMode || AllowPrune()); - m_best_block_index = block; if (AllowPrune() && block) { node::PruneLockInfo prune_lock; prune_lock.height_first = block->nHeight; WITH_LOCK(::cs_main, m_chainstate->m_blockman.UpdatePruneLock(GetName(), prune_lock)); } + + // Intentionally set m_best_block_index as the last step in this function, + // after updating prune locks above, and after making any other references + // to *this, so the BlockUntilSyncedToCurrentChain function (which checks + // m_best_block_index as an optimization) can be used to wait for the last + // BlockConnected notification and safely assume that prune locks are + // updated and that the index object is safe to delete. + m_best_block_index = block; } -- cgit v1.2.3 From a6fb674f966df27c09dc3d2b81040ce2965b2d7e Mon Sep 17 00:00:00 2001 From: stickies-v Date: Tue, 27 Sep 2022 19:21:51 +0100 Subject: refactor: remove unused locks for ResubmitWalletTransactions ReacceptWalletTransactions is replaced by ResubmitWalletTransactions which already handles acquiring the necessary locks internally. Github-Pull: #26205 Rebased-From: 01f3534632d18c772901fb6ce22f6394eae96799 --- src/wallet/rpc/backup.cpp | 20 ++++---------------- src/wallet/wallet.cpp | 4 +--- 2 files changed, 5 insertions(+), 19 deletions(-) (limited to 'src') diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp index 35df151e84..a971331a70 100644 --- a/src/wallet/rpc/backup.cpp +++ b/src/wallet/rpc/backup.cpp @@ -293,10 +293,7 @@ RPCHelpMan importaddress() if (fRescan) { RescanWallet(*pwallet, reserver); - { - LOCK(pwallet->cs_wallet); - pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true); - } + pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true); } return UniValue::VNULL; @@ -474,10 +471,7 @@ RPCHelpMan importpubkey() if (fRescan) { RescanWallet(*pwallet, reserver); - { - LOCK(pwallet->cs_wallet); - pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true); - } + pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true); } return UniValue::VNULL; @@ -1395,10 +1389,7 @@ RPCHelpMan importmulti() } if (fRescan && fRunScan && requests.size()) { int64_t scannedTime = pwallet->RescanFromTime(nLowestTimestamp, reserver, true /* update */); - { - LOCK(pwallet->cs_wallet); - pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true); - } + pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true); if (pwallet->IsAbortingRescan()) { throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted by user."); @@ -1689,10 +1680,7 @@ RPCHelpMan importdescriptors() // Rescan the blockchain using the lowest timestamp if (rescan) { int64_t scanned_time = pwallet->RescanFromTime(lowest_timestamp, reserver, true /* update */); - { - LOCK(pwallet->cs_wallet); - pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true); - } + pwallet->ResubmitWalletTransactions(/*relay=*/false, /*force=*/true); if (pwallet->IsAbortingRescan()) { throw JSONRPCError(RPC_MISC_ERROR, "Rescan aborted by user."); diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 5889de2e36..712814fdb6 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -3196,14 +3196,12 @@ bool CWallet::UpgradeWallet(int version, bilingual_str& error) void CWallet::postInitProcess() { - LOCK(cs_wallet); - // Add wallet transactions that aren't already in a block to mempool // Do this here as mempool requires genesis block to be loaded ResubmitWalletTransactions(/*relay=*/false, /*force=*/true); // Update wallet transactions with current mempool transactions. - chain().requestMempoolTransactions(*this); + WITH_LOCK(cs_wallet, chain().requestMempoolTransactions(*this)); } bool CWallet::BackupWallet(const std::string& strDest) const -- cgit v1.2.3 From fc8f2bfa3abc284ae3c1127fcf36535603ecc891 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 30 Sep 2022 11:03:53 +0100 Subject: refactor: carve out tx resend timer logic into ShouldResend Moves the logic of whether or not transactions should actually be resent out of the function that's resending them. This reduces responsibilities of ResubmitWalletTransactions and allows carving out the updating of m_next_resend in a future commit. Github-Pull: #26205 Rebased-From: 7fbde8af5c06694eecd4ce601109bd826a54bd6f --- src/wallet/wallet.cpp | 26 ++++++++++++++++++-------- src/wallet/wallet.h | 2 ++ 2 files changed, 20 insertions(+), 8 deletions(-) (limited to 'src') diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 712814fdb6..2c1cc3f3b3 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -1903,6 +1903,23 @@ std::set CWallet::GetTxConflicts(const CWalletTx& wtx) const return result; } +bool CWallet::ShouldResend() const +{ + // Don't attempt to resubmit if the wallet is configured to not broadcast + if (!fBroadcastTransactions) return false; + + // During reindex, importing and IBD, old wallet transactions become + // unconfirmed. Don't resend them as that would spam other nodes. + // We only allow forcing mempool submission when not relaying to avoid this spam. + if (!chain().isReadyToBroadcast()) return false; + + // Do this infrequently and randomly to avoid giving away + // that these are our transactions. + if (GetTime() < m_next_resend) return false; + + return true; +} + // Resubmit transactions from the wallet to the mempool, optionally asking the // mempool to relay them. On startup, we will do this for all unconfirmed // transactions but will not ask the mempool to relay them. We do this on startup @@ -1934,14 +1951,6 @@ void CWallet::ResubmitWalletTransactions(bool relay, bool force) // even if forcing. if (!fBroadcastTransactions) return; - // During reindex, importing and IBD, old wallet transactions become - // unconfirmed. Don't resend them as that would spam other nodes. - // We only allow forcing mempool submission when not relaying to avoid this spam. - if (!force && relay && !chain().isReadyToBroadcast()) return; - - // Do this infrequently and randomly to avoid giving away - // that these are our transactions. - if (!force && GetTime() < m_next_resend) return; // resend 12-36 hours from now, ~1 day on average. m_next_resend = GetTime() + (12 * 60 * 60) + GetRand(24 * 60 * 60); @@ -1979,6 +1988,7 @@ void CWallet::ResubmitWalletTransactions(bool relay, bool force) void MaybeResendWalletTxs(WalletContext& context) { for (const std::shared_ptr& pwallet : GetWallets(context)) { + if (!pwallet->ShouldResend()) continue; pwallet->ResubmitWalletTransactions(/*relay=*/true, /*force=*/false); } } diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index 6a1b76505c..9ee077b060 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -537,6 +537,8 @@ public: }; ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional max_height, const WalletRescanReserver& reserver, bool fUpdate, const bool save_progress); void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) override; + /** Return true if all conditions for periodically resending transactions are met. */ + bool ShouldResend() const; void ResubmitWalletTransactions(bool relay, bool force); OutputType TransactionChangeType(const std::optional& change_type, const std::vector& vecSend) const; -- cgit v1.2.3 From 43ced0b436b05ed12489a99bbac89f3b4c9ac035 Mon Sep 17 00:00:00 2001 From: stickies-v Date: Fri, 30 Sep 2022 11:11:31 +0100 Subject: wallet: only update m_next_resend when actually resending We only want to relay our resubmitted transactions once every 12-36h. By separating the timer update logic out of ResubmitWalletTransactions and into MaybeResendWalletTxs we avoid non-relay calls (previously in the separate ReacceptWalletTransactions function) from resetting that timer. Github-Pull: #26205 Rebased-From: 9245f456705b285e2d9afcc01a6155e1b3f92fad --- src/wallet/wallet.cpp | 10 +++++----- src/wallet/wallet.h | 6 +++++- 2 files changed, 10 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 2c1cc3f3b3..ac7bf46a14 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include