wallet: #25768 follow ups

stickies-v commented at 5:04 pm on September 29, 2022: contributor

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

capitalization typo in docstring
remove unused locks 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 such as transactions potentially never being rebroadcasted.
- it makes the ResubmitWalletTransactions()` logic more complicated than strictly necessary
- 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.

wallet: fix capitalization in docstring c6e8e11fb0

refactor: remove unused locks for ResubmitWalletTransactions

ReacceptWalletTransactions is replaced by ResubmitWalletTransactions
which already handles acquiring the necessary locks internally.

01f3534632

in src/wallet/wallet.h:544 in 4d7f86a380 outdated

539@@ -537,6 +540,10 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
540     };
541     ScanResult ScanForWalletTransactions(const uint256& start_block, int start_height, std::optional<int> max_height, const WalletRescanReserver& reserver, bool fUpdate, const bool save_progress);
542     void transactionRemovedFromMempool(const CTransactionRef& tx, MemPoolRemovalReason reason, uint64_t mempool_sequence) override;
543+    /** Set the next time this wallet should resend transactions, or use the default. */
544+    void SetNextResend(int next_resend = GetDefaultNextResend()) { m_next_resend = next_resend; }

maflcko commented at 5:22 pm on September 29, 2022:

Any reason to add a function that is only called once?

stickies-v commented at 5:40 pm on September 29, 2022:

Because m_next_resend is a private member, so I figured we’d prefer using a setter instead of making the variable public? Happy to take the other approach though, no strong preference.

maflcko commented at 5:48 pm on September 29, 2022:

Sounds good. If you keep the function, the argument can be removed. If you keep the argument, the type needs to be adjusted to be not truncating.

stickies-v commented at 11:23 am on September 30, 2022:

I’ve kept the function but removed the argument for now. Can easily be added when people actually need it, no need to overengineer the interface until then indeed. Thanks.

in src/wallet/wallet.h:352 in 4d7f86a380 outdated

348@@ -348,6 +349,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
349      */
350     static bool AttachChain(const std::shared_ptr<CWallet>& wallet, interfaces::Chain& chain, const bool rescan_required, bilingual_str& error, std::vector<bilingual_str>& warnings);
351 
352+    static int GetDefaultNextResend() { return GetTime() + (12 * 60 * 60) + GetRand(24 * 60 * 60); }

maflcko commented at 5:24 pm on September 29, 2022:

Any reason to truncate the result?

stickies-v commented at 5:45 pm on September 29, 2022:

What do you mean with truncating here? I just moved the existing logic into a new function, that on its own shouldn’t be any behavior change.

maflcko commented at 5:49 pm on September 29, 2022:

int32_t (aka int) has less bits than int64_t

stickies-v commented at 11:22 am on September 30, 2022:

Oh yes of course, thanks - fixed!

in src/wallet/wallet.h:254 in 4d7f86a380 outdated

250@@ -250,7 +251,7 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
251     int nWalletVersion GUARDED_BY(cs_wallet){FEATURE_BASE};
252 
253     /** The next scheduled rebroadcast of wallet transactions. */
254-    std::atomic<int64_t> m_next_resend{};
255+    std::atomic<int64_t> m_next_resend{GetDefaultNextResend()};

maflcko commented at 5:26 pm on September 29, 2022:

No longer needs to be atomic?

stickies-v commented at 11:22 am on September 30, 2022:

Makes sense, I’ve reverted that in a separate commit.

in src/wallet/wallet.cpp:1992 in 4d7f86a380 outdated

1983@@ -1979,7 +1984,9 @@ void CWallet::ResubmitWalletTransactions(bool relay, bool force)
1984 void MaybeResendWalletTxs(WalletContext& context)
1985 {
1986     for (const std::shared_ptr<CWallet>& pwallet : GetWallets(context)) {
1987+        if (!pwallet->ShouldResend()) continue;
1988         pwallet->ResubmitWalletTransactions(/*relay=*/true, /*force=*/false);
1989+        pwallet->SetNextResend();

maflcko commented at 5:27 pm on September 29, 2022:

Might be good to have bugfixes in separate commits from refactoring changes

stickies-v commented at 11:21 am on September 30, 2022:

Good idea, I’ve separated it out into two separate commits, one bugfix and one refactoring.

luke-jr commented at 0:01 am on October 6, 2022:

The “bugfix” commit is full of refactoring still

luke-jr commented at 0:02 am on October 6, 2022:

Also, cherry-picking the bugfix commit by itself is broken: it will reset m_next_resend to 12-36 hours, every minute, even if nothing is done/rebroadcast.

maflcko approved

maflcko added this to the milestone 24.0 on Sep 29, 2022

DrahtBot added the label Wallet on Sep 29, 2022

DrahtBot commented at 0:39 am on September 30, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26238 (clang-tidy: fixup named argument comments by fanquake)
#25907 (wallet: rpc to add automatically generated descriptors by achow101)
#23544 (rpc, wallet: addhdseed, infer seed when importing descriptor with xpub by Sjors)
#23417 (wallet, spkm: Move key management from DescriptorScriptPubKeyMan to wallet level KeyManager by achow101)
#22341 (rpc: add getxpub by Sjors)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

stickies-v force-pushed on Sep 30, 2022

stickies-v commented at 11:33 am on September 30, 2022: contributor

Force pushed to address MarcoFalke’s review comments - thank you!

Split out “wallet: move resend timer logic out of ResubmitWalletTransactions” into two commits: one refactoring commit (that introduces ShouldResend() but has no behaviour change) and one bugfix commit that only updates m_next_resend when we’re actually resending the transactions
Fixed int truncation bug
No need for m_next_resend to be std::atomic anymore, since it’s now only called from a single thread (MaybeResendWalletTxs())
Made SetNextResend() argument-less since we’re never actually passing the optional argument, can easily add back later.

in src/wallet/wallet.h:352 in c98730f27a outdated

348@@ -348,6 +349,8 @@ class CWallet final : public WalletStorage, public interfaces::Chain::Notificati
349      */
350     static bool AttachChain(const std::shared_ptr<CWallet>& wallet, interfaces::Chain& chain, const bool rescan_required, bilingual_str& error, std::vector<bilingual_str>& warnings);
351 
352+    static int64_t GetDefaultNextResend() { return GetTime() + (12 * 60 * 60) + GetRand(24 * 60 * 60); }

maflcko commented at 11:34 am on September 30, 2022:

nit: Could put the impl into the cpp file to avoid a header include in the header?

maflcko approved

maflcko commented at 11:37 am on September 30, 2022: member

lgtm. I wouldn’t call the last commit a “performance” improvement. Just a code clarity change.

in src/wallet/wallet.cpp:1906 in 14b8fe1765 outdated

1902@@ -1903,6 +1903,23 @@ std::set<uint256> CWallet::GetTxConflicts(const CWalletTx& wtx) const
1903     return result;
1904 }
1905 
1906+bool CWallet::ShouldResend()

aureleoules commented at 11:45 am on September 30, 2022:

nit in 14b8fe1765322e62bf782ff419b71d7eea9bdb56

0bool CWallet::ShouldResend() const

stickies-v commented at 1:53 pm on September 30, 2022:

Good catch, thx. Used to have side effects in ShouldResend() in an earlier implementation and forgot to update the constness.

stickies-v force-pushed on Sep 30, 2022

stickies-v commented at 11:49 am on September 30, 2022: contributor

Force pushed to remove “performance improvement” from the std::atomic revert commit message, and moved GetDefaultNextResend() implementation to wallet.cpp to reduce header includes in the header - thanks for both MarcoFalke.

in src/wallet/wallet.cpp:1946 in 6610cc99a6 outdated

1960-
1961-    // Do this infrequently and randomly to avoid giving away
1962-    // that these are our transactions.
1963-    if (!force && GetTime() < m_next_resend) return;
1964-    // resend 12-36 hours from now, ~1 day on average.
1965-    m_next_resend = GetTime() + (12 * 60 * 60) + GetRand(24 * 60 * 60);

aureleoules commented at 12:16 pm on September 30, 2022:

By removing this line, calls such as importaddress, importpubkey, importmulti and importdescriptors with rescan enabled won’t update the timeout because now the timeout is only reset in MaybeResendWalletTxs with SetNextResend, is this normal?

See https://github.com/bitcoin/bitcoin/blob/6610cc99a6a479ca22d272a1d583ab2a8422b7ae/src/wallet/rpc/backup.cpp#L471-L475

stickies-v commented at 1:52 pm on September 30, 2022:

Yes, this is intentional. Prior to #25768, we had two functions ReacceptWalletTransactions() (which did not update/read nNextResend) and ResendWalletTransactions() (which did update/read nNextResend, only called from MaybeUpdateWalletTxs()), which got merged into a single parameterized ResubmitWalletTransactions() (and updates/reads m_next_resend).

Since the purpose of m_next_resend is to prevent overly frequent relaying of transactions, we should only update the timer when relaying is triggered, so this PR just undoes that behaviour change introduced in #25768.

I tried to summarize this rationale in the commit message of 3e5d3f8705154e09ecf3639f6d261595992fc5b5 - lmk if that message is unclear?

aureleoules commented at 2:11 pm on September 30, 2022:

Great, that makes sense since these RPC calls don’t relay transactions. Your commit message is also clear with this in mind :+1:.

aureleoules commented at 12:18 pm on September 30, 2022: member

01f3534632d18c772901fb6ce22f6394eae96799: I verified that ResubmitWalletTransactions already locks the mutex so no need to lock it in the parent functions.

Left a comment, the rest looks good.

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.

7fbde8af5c

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.

9245f45670

refactor: revert m_next_resend to not be std::atomic

Since m_next_resend is now only called from MaybeResendWalletTxs()
we don't have any potential race conditions anymore, so the usage
of std::atomic can be reverted.

b01682a812

stickies-v force-pushed on Sep 30, 2022

stickies-v commented at 1:54 pm on September 30, 2022: contributor

Force pushed to make CWallet::ShouldResend() const - ty aureleoules!

aureleoules commented at 2:11 pm on September 30, 2022: member

ACK b01682a812f0841170657708ef0e896b904fcd77

maflcko requested review from achow101 on Sep 30, 2022

luke-jr commented at 5:42 pm on October 5, 2022: member

I verified that ResubmitWalletTransactions already locks the mutex so no need to lock it in the parent functions.

That’s not necessarily a given. The parents may need to lock things for atomicity (with other changes/calls), or lock ordering consistency. (I didn’t look at this particular case, just noting the general logic does not hold)

luke-jr commented at 11:55 pm on October 5, 2022: member

it leads to #25768 (comment) such as transactions potentially never being rebroadcasted.

I think this bugfix should be split out and backported. Ideally doing something more minimal which can be refactored here.

achow101 commented at 8:07 pm on October 12, 2022: member

ACK b01682a812f0841170657708ef0e896b904fcd77

fanquake merged this on Oct 13, 2022

fanquake closed this on Oct 13, 2022

fanquake added the label Needs backport (24.x) on Oct 13, 2022

fanquake removed the label Needs backport (24.x) on Oct 13, 2022

fanquake commented at 3:47 pm on October 13, 2022: member

Added to #26133 for backport.

fanquake referenced this in commit a6fb674f96 on Oct 13, 2022

fanquake referenced this in commit fc8f2bfa3a on Oct 13, 2022

fanquake referenced this in commit 43ced0b436 on Oct 13, 2022

fanquake referenced this in commit 9b438f06ec on Oct 13, 2022

achow101 referenced this in commit 885366c67a on Oct 13, 2022

sidhujag referenced this in commit 15e781feaf on Oct 23, 2022

stickies-v deleted the branch on Jul 31, 2023

bitcoin locked this on Jul 30, 2024

wallet: #25768 follow ups #26205

Conflicts