wallet: #25768 follow ups #26205
Merged
wallet: #25768 follow ups #26205
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ReacceptWalletTransactions is replaced by ResubmitWalletTransactions which already handles acquiring the necessary locks internally.
maflcko
approved these changes
Sep 29, 2022
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
4d7f86a to
c98730f
Compare
stickies-v
commented
Sep 30, 2022
There was a problem hiding this comment.
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 updatesm_next_resendwhen we're actually resending the transactions - Fixed int truncation bug
- No need for
m_next_resendto bestd::atomicanymore, 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.
maflcko
approved these changes
Sep 30, 2022
c98730f to
6610cc9
Compare
|
Force pushed to remove "performance improvement" from the std::atomic revert commit message, and moved |
aureleoules
reviewed
Sep 30, 2022
There was a problem hiding this comment.
01f3534: 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.
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.
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.
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.
6610cc9 to
b01682a
Compare
|
Force pushed to make |
|
ACK b01682a |
This was referenced Oct 3, 2022
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) |
I think this bugfix should be split out and backported. Ideally doing something more minimal which can be refactored here. |
This was referenced Oct 6, 2022
Closed
|
ACK b01682a |
|
Added to #26133 for backport. |
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Oct 13, 2022
ReacceptWalletTransactions is replaced by ResubmitWalletTransactions which already handles acquiring the necessary locks internally. Github-Pull: bitcoin#26205 Rebased-From: 01f3534
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Oct 13, 2022
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: bitcoin#26205 Rebased-From: 7fbde8a
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Oct 13, 2022
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: bitcoin#26205 Rebased-From: 9245f45
fanquake
pushed a commit
to fanquake/bitcoin
that referenced
this pull request
Oct 13, 2022
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. Github-Pull: bitcoin#26205 Rebased-From: b01682a
achow101
added a commit
that referenced
this pull request
Oct 13, 2022
e2e4c29 tests: Test that PSBT_OUT_TAP_TREE is included correctly (Andrew Chow) 4d42c3a psbt: Only include m_tap_tree if it has scripts (Andrew Chow) d810fde psbt: Change m_tap_tree to store just the tuples (Andrew Chow) a9419ef tests: Test that PSBT_OUT_TAP_TREE is combined correctly (Andrew Chow) 4abd2ab psbt: Fix merging of m_tap_tree (Andrew Chow) 1390c96 [BugFix]: Do not allow deserializing PSBT with empty PSBT_OUT_TAP_TREE (Jeremy Rubin) 9b438f0 refactor: revert m_next_resend to not be std::atomic (stickies-v) 43ced0b wallet: only update m_next_resend when actually resending (stickies-v) fc8f2bf refactor: carve out tx resend timer logic into ShouldResend (stickies-v) a6fb674 refactor: remove unused locks for ResubmitWalletTransactions (stickies-v) 5ad82a0 index: Improve BaseIndex::BlockUntilSyncedToCurrentChain reliability (Ryan Ofsky) 997faf6 contrib: Fix capture_output in getcoins.py (willcl-ark) 7e0bcfb p2p: ProcessHeadersMessage(): fix received_new_header (Larry Ruane) c97d924 Correct sanity-checking script_size calculation (Pieter Wuille) da6fba6 docs: Add 371 to bips.md (Andrew Chow) Pull request description: Will collect backports for rc2 as they become available. Currently: * #25858 * #26124 * #26149 * #26172 * #26205 * #26212 * #26215 ACKs for top commit: dergoegge: ACK e2e4c29 achow101: ACK e2e4c29 instagibbs: ACK e2e4c29 Tree-SHA512: b6374fe202561057dbe1430d4c40f06f721eb568f91e7275ae1ee7747edf780ce64620382d13ecc4b9571d931dc25d226af8284987cf35ff6a6182c5f64eb10c
sidhujag
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 23, 2022
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses the outstanding comments/issues from #25768:
ReacceptWalletTransactions()ResendWalletTransactions()would resetm_next_resend(formerly callednNextResend). By unifying it withReacceptWalletTransactions()intoResubmitWalletTransactions(), the number of callsites that would reset them_next_resendtimer increasedm_next_resendis only used in case ofrelay=true(formerlyResendWalletTransactions()), this is unintuitiveResubmitWalletTransactions(relay=false, force=true)to initializem_next_resend(), I think we can more elegantly do that by just providingm_next_resendwith a default valueNote: the
if (!fBroadcastTransactions)inCWallet:ShouldResend()is duplicated on purpose, since it potentially avoids the slightly more expensiveif (!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.