-
Notifications
You must be signed in to change notification settings - Fork 38.8k
[wallet] Refactor relay transactions #15728
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[wallet] Refactor relay transactions #15728
Conversation
|
Base PR has been merged. This is ready for review. |
promag
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK afb608b, since the refactor improves code comments and avoids nesting code.
src/wallet/wallet.cpp
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could keep this comment?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is replaced by the Don't relay conflicted or already confirmed transactions comment above the call to GetDepthInMainChain().
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsNo conflicts as of last run. |
ryanofsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK afb608b1e92773aba2698b698bcf62011ff7b783. This isn't a pure refactoring so you may want to point out changes in behavior in the commit description (no longer asserting false if GetBroadcastTransactions if false, no longer printing relay message if p2pEnabled is false).
afb608b to
57516eb
Compare
ryanofsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
utACK 57516ebdfcd288cb6c255fb0fbc94b7fb1e50af7 (just last commits, not base commits). Only change since last review is updating commit description.
This refactors the CWalletTx::RelayWalletTransaction() function to be clearer and adds comments. It also makes two minor behavior changes: - no longer assert if fBroadcastTransactions is false. Just return false from the function. - no longer print the relay message if p2pEnabled is set to false (since the transaction is not actually relayed).
57516eb to
7a9046e
Compare
|
rebased |
|
utACK 7a9046e. |
ryanofsky
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
utACK 7a9046e |
Summary: This refactors the CWalletTx::RelayWalletTransaction() function to be clearer and adds comments. It also makes two minor behavior changes: - no longer assert if fBroadcastTransactions is false. Just return false from the function. - no longer print the relay message if p2pEnabled is set to false (since the transaction is not actually relayed). --- Backport of Core [[bitcoin/bitcoin#15728 | PR15728]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6406
Refactor
CWalletTx::RelayWalletTransaction()function.This was a suggestion from the wallet-node separation PR: #15288 (comment), which we deferred until after the main PR was merged.
There are also makes two minor behavior changes: