Skip to content
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

Use wallet RBF default for walletcreatefundedpsbt #15911

Merged
merged 3 commits into from Aug 2, 2019

Conversation

@Sjors
Copy link
Member

commented Apr 27, 2019

The walletcreatefundedpsbt RPC call currently ignores -walletrbf and defaults to not use RBF. This PR fixes that.

This PR also replaces UniValue in ConstructTransaction with a bool in preparation of moving this helper method out of the RPC codebase entirely. This may be a bit overkill, but does slightly simplify it.

Fixes #15878

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Apr 27, 2019

(my initial attempt ddfce96 used Optional<Bool>) Given that wallet->m_signal_rbf is a boolean I think we should just stop treating replaceable as an optional parameter. In that case ConstructTransaction can take a boolean parameter and be simplified a bit.

@Sjors Sjors force-pushed the Sjors:2019/04/walletcreatefundedpsbt branch from aafdfde to 6096851 Apr 27, 2019

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16378 ([WIP] The ultimate send RPC by Sjors)
  • #16377 ([rpc] don't automatically append inputs in walletcreatefundedpsbt & fundrawtransaction by Sjors)
  • #14641 (rpc: Add min_conf option to fund transaction calls by promag)
  • #11413 ([wallet] [rpc] sendtoaddress/sendmany: Add explicit feerate option by kallewoof)

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.

src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Outdated Show resolved Hide resolved

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request May 1, 2019

@Sjors Sjors force-pushed the Sjors:2019/04/walletcreatefundedpsbt branch 2 times, most recently from 9a27860 to 9b1b9cf May 12, 2019

@MarcoFalke MarcoFalke added this to the 0.18.1 milestone May 16, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented May 23, 2019

utACK 9b1b9cf

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented May 25, 2019

@Sjors Please squash the first and third commit, as they don't compile:

  CXX      wallet/libbitcoin_wallet_a-rpcwallet.o
wallet/rpcwallet.cpp: In function ‘UniValue walletcreatefundedpsbt(const JSONRPCRequest&)’:
wallet/rpcwallet.cpp:4137:143: error: cannot convert ‘const UniValue’ to ‘bool’
 4137 |     CMutableTransaction rawTx = ConstructTransaction(request.params[0], request.params[1], request.params[2], request.params[3]["replaceable"]);
      |                                                                                                                                               ^
      |                                                                                                                                               |
      |                                                                                                                                               const UniValue
In file included from wallet/rpcwallet.cpp:20:
./rpc/rawtransaction_util.h:30:128: note:   initializing argument 4 of ‘CMutableTransaction ConstructTransaction(const UniValue&, const UniValue&, const UniValue&, bool)’
   30 | CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, bool rbf);
      |                                                                                                                           ~~~~~^~~
@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

@Sjors ^ 👀

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2019

Soon (TM), sorry for the delay.

@Sjors Sjors force-pushed the Sjors:2019/04/walletcreatefundedpsbt branch from 9b1b9cf to f847fa9 Jun 26, 2019

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

Rebased and squashed first and last commit. @MarcoFalke hopefully good to go.

@meshcollider
Copy link
Member

left a comment

Code review ACK f847fa9

test/functional/rpc_psbt.py Outdated Show resolved Hide resolved

@Sjors Sjors force-pushed the Sjors:2019/04/walletcreatefundedpsbt branch from f847fa9 to 07299aa Jun 26, 2019

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jun 26, 2019

Fixed the typo. You can see the diff with:

PREV=f847fa9 N=2 && git range-diff `git merge-base --all HEAD $PREV`...$PREV HEAD~$N...HEAD
@l2a5b1
Copy link
Contributor

left a comment

ACK 4348388

+1 for finding this issue and fixing it. Feel free to ignore my review comments.

@@ -18,7 +18,7 @@
#include <util/rbf.h>
#include <util/strencodings.h>

CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, const UniValue& rbf)
CMutableTransaction ConstructTransaction(const UniValue& inputs_in, const UniValue& outputs_in, const UniValue& locktime, bool rbf)

This comment has been minimized.

Copy link
@l2a5b1

l2a5b1 Jul 10, 2019

Contributor

Will ConstructTransaction be moved out of the RPC module?

Without this change the issue could probably have been addressed with a small update in walletcreatefundedpsbt.

If ConstructTransaction stays in the RPC codebase forever then this change has also introduced duplicate code at two call sites for no reason.

This comment has been minimized.

Copy link
@Sjors

Sjors Jul 11, 2019

Author Member

I expect more stuff to be moved out of the RPC over time, but for now this was the simplest fix, also easiest to backport.

test/functional/rpc_psbt.py Show resolved Hide resolved
test/functional/rpc_psbt.py Outdated Show resolved Hide resolved
@Sjors Sjors referenced this pull request Jul 12, 2019
0 of 5 tasks complete
@promag
Copy link
Member

left a comment

Concept ACK.

test/functional/rpc_psbt.py Outdated Show resolved Hide resolved
test/functional/rpc_psbt.py Outdated Show resolved Hide resolved
test/functional/rpc_psbt.py Outdated Show resolved Hide resolved

@MarcoFalke MarcoFalke removed this from the 0.18.1 milestone Jul 18, 2019

@MarcoFalke MarcoFalke added this to the 0.18.2 milestone Jul 18, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

Moved to 0.18.2

Sjors added some commits Apr 27, 2019

@Sjors Sjors force-pushed the Sjors:2019/04/walletcreatefundedpsbt branch from 17f0cb0 to d6b3640 Jul 27, 2019

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2019

Rebased and addressed nits.

@DrahtBot DrahtBot removed the Needs rebase label Jul 27, 2019

@achow101

This comment has been minimized.

Copy link
Member

commented Jul 30, 2019

Code Review ACK d6b3640

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2019

@l2a5b1

This comment has been minimized.

Copy link
Contributor

commented Aug 2, 2019

re-ACK d6b3640

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Aug 2, 2019

ACK d6b3640

Show signature and timestamp

Signature:

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK d6b3640ac732f6f66a8cb6761084d1beecc8a876
-----BEGIN PGP SIGNATURE-----

iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUjtHQwAlvgQph2uP/hqvyh4EGclSdMka8jVYUY56nEWpMuESGsZdo+OecyPiahc
HP2P5wvPIwUS6LF+HXzCLw9yxZY6KKZIqYOO2nG2mcP9KycMvcncopG+QyqOUxp/
hFVueMAoNFo8xfpodLq7/LjEIWt32F5FHyaLr4jBQZcxJo5nFBGtwBpzRVcK00LE
+YdXVacxCu9Q74ET99AlbBbQk5kVBuIuK1TYFsPQzQFsgp+L0ysOx697VSDFOHEW
4RcTw/wrIbmg0ItVWq6Eq2ufJmHogY5mzurRPxli3ybm4+bSHVQbyiC+0sh++qx/
E1p03kuygA+lhImxwB3SpRBqUCyf+uR9vc7yMPjQ0p5izkOCNS5XHl31wwkGBl80
bJgF8KgqkkHQrHn/o/PzxLiixkIbZw0K+KU7+4Cl7r3Qio3b6SCCBF94tW7jg8hx
EsjpGUS7m+z4qZt91zPndcbCSxF5brfN/14LsVcYEoTF9YiU9Y6VbMSHEDrTG+vO
u+gEf0y3
=31Xg
-----END PGP SIGNATURE-----

Timestamp of file with hash 388665302574fab8386985975f35ab1e4733960dbf1495ad6009328b30e160eb -

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Aug 2, 2019

Merge bitcoin#15911: Use wallet RBF default for walletcreatefundedpsbt
d6b3640 [test] walletcreatefundedpsbt: check RBF is disabled when -walletrbf=0 (Sjors Provoost)
9ed062b [doc] rpc: remove "fallback to" from RBF default help (Sjors Provoost)
4fcb698 [rpc] walletcreatefundedpsbt: use wallet default RBF (Sjors Provoost)

Pull request description:

  The `walletcreatefundedpsbt` RPC call currently ignores `-walletrbf` and defaults to not use RBF. This PR fixes that.

  This PR also replaces UniValue in `ConstructTransaction` with a `bool` in preparation of moving this helper method out of the RPC codebase entirely. This may be a bit overkill, but does slightly simplify it.

  Fixes bitcoin#15878

ACKs for top commit:
  achow101:
    Code Review ACK d6b3640
  l2a5b1:
    re-ACK d6b3640
  MarcoFalke:
    ACK d6b3640

Tree-SHA512: 55b9bccd1ef36b54f6b34793017dc0721103099ad3761b3b04862291ee13d6915915d4dbb1a8567924fa56e5e95dfe10eec070e06701610e70c87f8ea92b2a00

@MarcoFalke MarcoFalke merged commit d6b3640 into bitcoin:master Aug 2, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@Sjors Sjors deleted the Sjors:2019/04/walletcreatefundedpsbt branch Aug 2, 2019

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 3, 2019

Merge bitcoin#15911: Use wallet RBF default for walletcreatefundedpsbt
d6b3640 [test] walletcreatefundedpsbt: check RBF is disabled when -walletrbf=0 (Sjors Provoost)
9ed062b [doc] rpc: remove "fallback to" from RBF default help (Sjors Provoost)
4fcb698 [rpc] walletcreatefundedpsbt: use wallet default RBF (Sjors Provoost)

Pull request description:

  The `walletcreatefundedpsbt` RPC call currently ignores `-walletrbf` and defaults to not use RBF. This PR fixes that.

  This PR also replaces UniValue in `ConstructTransaction` with a `bool` in preparation of moving this helper method out of the RPC codebase entirely. This may be a bit overkill, but does slightly simplify it.

  Fixes bitcoin#15878

ACKs for top commit:
  achow101:
    Code Review ACK d6b3640
  l2a5b1:
    re-ACK d6b3640
  MarcoFalke:
    ACK d6b3640

Tree-SHA512: 55b9bccd1ef36b54f6b34793017dc0721103099ad3761b3b04862291ee13d6915915d4dbb1a8567924fa56e5e95dfe10eec070e06701610e70c87f8ea92b2a00
@fanquake

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

@Sjors Are you interested in backporting this?

@Sjors

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

@fanquake backporting to 0.18 in #16608

MarcoFalke added a commit that referenced this pull request Aug 19, 2019

Merge #16608: [0.18] Backport #15911: Use wallet RBF default for wall…
…etcreatefundedpsbt

576580f [test] walletcreatefundedpsbt: check RBF is disabled when -walletrbf=0 (Sjors Provoost)
0942a60 [doc] rpc: remove "fallback to" from RBF default help (Sjors Provoost)
ee950ec [rpc] walletcreatefundedpsbt: use wallet default RBF (Sjors Provoost)

Pull request description:

  Backport of #15911 for 0.18 branch.

  The original PR changed `rawtransaction_util.cpp`, whereas the backport changes `rawtransaction.cpp` to avoid having to also backport #15638.

ACKs for top commit:
  meshcollider:
    re-utACK 576580f
  MarcoFalke:
    ACK 576580f

Tree-SHA512: 51c40fc4db4f8739e93e931aeac572779d5ade0bfeb3bcb0082301cf73b4f48e31d4ccbe40421dc65d0ec091d6393685c0c9b8f76584c59249f13642e80a6da5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.