Skip to content

Commit

Permalink
Merge bitcoin#26205: wallet: bitcoin#25768 follow ups
Browse files Browse the repository at this point in the history 
b01682a refactor: revert m_next_resend to not be std::atomic (stickies-v)
9245f45 wallet: only update m_next_resend when actually resending (stickies-v)
7fbde8a refactor: carve out tx resend timer logic into ShouldResend (stickies-v)
01f3534 refactor: remove unused locks for ResubmitWalletTransactions (stickies-v)
c6e8e11 wallet: fix capitalization in docstring (stickies-v)

Pull request description:

  This PR addresses the outstanding comments/issues from bitcoin#25768:

  - capitalization [typo](bitcoin#25768 (comment)) in docstring
  - remove [unused locks](bitcoin@01f3534) that we previously needed for `ReacceptWalletTransactions()`
  - before bitcoin#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](bitcoin#25768 (comment)) such as transactions potentially never being rebroadcasted.
    - it makes the ResubmitWalletTransactions()` logic [more complicated than strictly necessary](bitcoin#25768 (comment))
    - since bitcoin#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 b01682a
  achow101:
    ACK b01682a

Tree-SHA512: ac5f1d8858f8dd736dd1480f385984d660c1916b62a42562317020e8f9fd6a30bd8f23d973d47e4c9480d744c5ba39fdbefd69568a5eb0589a8422d7e5971c1c
  • Loading branch information
@fanquake
fanquake committed Oct 13, 2022
2 parents 7e5fe03 + b01682a commit aa6fb37
Show file tree
Hide file tree
Showing 3 changed files with 36 additions and 34 deletions.
20 changes: 4 additions & 16 deletions src/wallet/rpc/backup.cpp
View file
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -1406,10 +1400,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.");
Expand Down Expand Up @@ -1700,10 +1691,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.");
Expand Down
42 changes: 25 additions & 17 deletions src/wallet/wallet.cpp
View file
Expand Up @@ -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>
Expand Down Expand Up @@ -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.
Expand All @@ -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
Expand All @@ -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);
Expand All @@ -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();
}
}

Expand Down Expand Up @@ -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
Expand Down
8 changes: 7 additions & 1 deletion src/wallet/wallet.h
View file
Expand Up @@ -250,7 +250,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
int nWalletVersion GUARDED_BY(cs_wallet){FEATURE_BASE};

/** The next scheduled rebroadcast of wallet transactions. */
std::atomic<int64_t> m_next_resend{};
int64_t m_next_resend{GetDefaultNextResend()};
/** Whether this wallet will submit newly created transactions to the node's mempool and
* prompt rebroadcasts (see ResendWalletTransactions()). */
bool fBroadcastTransactions = false;
Expand Down Expand Up @@ -348,6 +348,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
*/
static bool AttachChain(const std::shared_ptr<CWallet>& wallet, interfaces::Chain& chain, const bool rescan_required, bilingual_str& error, std::vector<bilingual_str>& warnings);

static int64_t GetDefaultNextResend();

public:
/**
* Main wallet lock.
Expand Down Expand Up @@ -537,6 +539,10 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
};
ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate, const bool save_progress);
void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) override;
/** Set the next time this wallet should resend transactions to 12-36 hours from now, ~1 day on average. */
void SetNextResend() { m_next_resend = GetDefaultNextResend(); }
/** 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<OutputType>& change_type, const std::vector<CRecipient>& vecSend) const;
Expand Down

0 comments on commit aa6fb37

Please sign in to comment.