highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-03-27 07:55:39 +01:00
Code Issues Packages Projects Releases Wiki Activity

Merge bitcoin/bitcoin#26205: wallet: #25768 follow ups

Browse Source 
b01682a812 refactor: revert m_next_resend to not be std::atomic (stickies-v)
9245f45670 wallet: only update m_next_resend when actually resending (stickies-v)
7fbde8af5c refactor: carve out tx resend timer logic into ShouldResend (stickies-v)
01f3534632 refactor: remove unused locks for ResubmitWalletTransactions (stickies-v)
c6e8e11fb0 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](01f3534632) 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 b01682a812
  achow101:
    ACK b01682a812

Tree-SHA512: ac5f1d8858f8dd736dd1480f385984d660c1916b62a42562317020e8f9fd6a30bd8f23d973d47e4c9480d744c5ba39fdbefd69568a5eb0589a8422d7e5971c1c
This commit is contained in:
fanquake
2022-10-13 11:57:35 +08:00
parent 7e5fe03461 b01682a812
commit aa6fb37acc
3 changed files with 36 additions and 34 deletions
Download Patch File Download Diff File Expand all files Collapse all files

20
src/wallet/rpc/backup.cpp
View File

@@ -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;
@@ -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.");
@@ -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.");