diff options
author | fanquake <fanquake@gmail.com> | 2022-10-13 11:57:35 +0800 |
---|---|---|
committer | fanquake <fanquake@gmail.com> | 2022-10-13 12:09:44 +0800 |
commit | aa6fb37accc6445855c81b6b236e574586df9c28 (patch) | |
tree | 6858904d4c1c832e76cc217e95372c77ba625fd8 /src/wallet/wallet.cpp | |
parent | 7e5fe034618e4d7dfc1c203cb2e29220389fb1a6 (diff) | |
parent | b01682a812f0841170657708ef0e896b904fcd77 (diff) | |
download | bitcoin-aa6fb37accc6445855c81b6b236e574586df9c28.tar.xz |
Merge bitcoin/bitcoin#26205: wallet: #25768 follow ups
b01682a812f0841170657708ef0e896b904fcd77 refactor: revert m_next_resend to not be std::atomic (stickies-v)
9245f456705b285e2d9afcc01a6155e1b3f92fad wallet: only update m_next_resend when actually resending (stickies-v)
7fbde8af5c06694eecd4ce601109bd826a54bd6f refactor: carve out tx resend timer logic into ShouldResend (stickies-v)
01f3534632d18c772901fb6ce22f6394eae96799 refactor: remove unused locks for ResubmitWalletTransactions (stickies-v)
c6e8e11fb030ef406752761530421a9e2f0f5d4f wallet: fix capitalization in docstring (stickies-v)
Pull request description:
This PR addresses the outstanding comments/issues from #25768:
- capitalization [typo](https://github.com/bitcoin/bitcoin/pull/25768#discussion_r958572522) in docstring
- remove [unused locks](https://github.com/bitcoin/bitcoin/commit/01f3534632d18c772901fb6ce22f6394eae96799) that we previously needed for `ReacceptWalletTransactions()`
- before #25768, only `ResendWalletTransactions()` would reset `m_next_resend` (formerly called `nNextResend`). By unifying it with `ReacceptWalletTransactions()` into `ResubmitWalletTransactions()`, the number of callsites that would reset the `m_next_resend` timer increased
- since `m_next_resend` is only used in case of `relay=true` (formerly `ResendWalletTransactions()`), this is unintuitive
- it leads to [unexpected behaviour](https://github.com/bitcoin/bitcoin/pull/25768#issuecomment-1252619427) such as transactions potentially never being rebroadcasted.
- it makes the ResubmitWalletTransactions()` logic [more complicated than strictly necessary](https://github.com/bitcoin/bitcoin/pull/25768#discussion_r962828563)
- since #25768, we relied on an earlier call of `ResubmitWalletTransactions(relay=false, force=true)` to initialize `m_next_resend()`, I think we can more elegantly do that by just providing `m_next_resend` with a default value
- just to highlight: this commit introduces behaviour change
Note: the `if (!fBroadcastTransactions)` in `CWallet:ShouldResend()` is duplicated on purpose, since it potentially avoids the slightly more expensive `if (!chain().isReadyToBroadcast())` check afterwards. I don't have a strong view on it, so happy to remove that additional check to reduce the diff, too.
ACKs for top commit:
aureleoules:
ACK b01682a812f0841170657708ef0e896b904fcd77
achow101:
ACK b01682a812f0841170657708ef0e896b904fcd77
Tree-SHA512: ac5f1d8858f8dd736dd1480f385984d660c1916b62a42562317020e8f9fd6a30bd8f23d973d47e4c9480d744c5ba39fdbefd69568a5eb0589a8422d7e5971c1c
Diffstat (limited to 'src/wallet/wallet.cpp')
-rw-r--r-- | src/wallet/wallet.cpp | 42 |
1 files changed, 25 insertions, 17 deletions
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 461e64751a..a576b07f50 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -21,6 +21,7 @@ #include <primitives/block.h> #include <primitives/transaction.h> #include <psbt.h> +#include <random.h> #include <script/descriptor.h> #include <script/script.h> #include <script/signingprovider.h> @@ -1903,11 +1904,29 @@ std::set<uint256> 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; +} + +int64_t CWallet::GetDefaultNextResend() { return GetTime() + (12 * 60 * 60) + GetRand(24 * 60 * 60); } + // 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 -// to ensure that our own mempool is aware of our transactions, and to also -// initialize m_next_resend so that the actual rebroadcast is scheduled. There +// to ensure that our own mempool is aware of our transactions. There // is a privacy side effect here as not broadcasting on startup also means that we won't // inform the world of our wallet's state, particularly if the wallet (or node) is not // yet synced. @@ -1934,17 +1953,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); - int submitted_tx_count = 0; { // cs_wallet scope @@ -1957,7 +1965,7 @@ void CWallet::ResubmitWalletTransactions(bool relay, bool force) // Only rebroadcast unconfirmed txs if (!wtx.isUnconfirmed()) continue; - // attempt to rebroadcast all txes more than 5 minutes older than + // Attempt to rebroadcast all txes more than 5 minutes older than // the last block, or all txs if forcing. if (!force && wtx.nTimeReceived > m_best_block_time - 5 * 60) continue; to_submit.insert(&wtx); @@ -1979,7 +1987,9 @@ void CWallet::ResubmitWalletTransactions(bool relay, bool force) void MaybeResendWalletTxs(WalletContext& context) { for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) { + if (!pwallet->ShouldResend()) continue; pwallet->ResubmitWalletTransactions(/*relay=*/true, /*force=*/false); + pwallet->SetNextResend(); } } @@ -3198,14 +3208,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 |