Skip to content

Conversation

@jnewbery
Copy link
Contributor

@jnewbery jnewbery commented Apr 3, 2019

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:

  • 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).

@jnewbery
Copy link
Contributor Author

jnewbery commented Apr 3, 2019

The first three commits are from #15632. Please review that PR first.

Base PR has been merged. This is ready for review.

@fanquake fanquake added the Wallet label Apr 3, 2019
Copy link
Contributor

@promag promag left a 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could keep this comment?

Copy link
Contributor Author

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().

@DrahtBot
Copy link
Contributor

DrahtBot commented Apr 3, 2019

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

Conflicts

No conflicts as of last run.

Copy link
Contributor

@ryanofsky ryanofsky left a 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).

@jnewbery jnewbery force-pushed the 2019_03_refactor_relay_transactions branch from afb608b to 57516eb Compare April 3, 2019 14:42
@jnewbery
Copy link
Contributor Author

jnewbery commented Apr 3, 2019

Commit log and PR description updated.

This will require rebase since #15632 has been updated. I don't intend to do that until #15632 is merged to avoid multiple iterations of rebase/review.

Copy link
Contributor

@ryanofsky ryanofsky left a 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).
@jnewbery jnewbery force-pushed the 2019_03_refactor_relay_transactions branch from 57516eb to 7a9046e Compare April 10, 2019 13:20
@jnewbery
Copy link
Contributor Author

rebased

@promag
Copy link
Contributor

promag commented Apr 10, 2019

utACK 7a9046e.

Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 7a9046e. No changes at all, just rebase after base PR #15632 was merged

@meshcollider
Copy link
Contributor

utACK 7a9046e

@meshcollider meshcollider merged commit 7a9046e into bitcoin:master Apr 11, 2019
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jun 6, 2020
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
kwvg added a commit to kwvg/dash that referenced this pull request Dec 12, 2021
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants