Use wallet RBF default for walletcreatefundedpsbt #15911

Sjors · 2019-04-27T18:05:34Z

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 · 2019-04-27T18:16:30Z

(my initial attempt ddfce96d79809112d8253a8a82cc9bf11c3a1beb 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.

DrahtBot · 2019-04-27T20:55:09Z

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

Github-Pull: bitcoin#15911 Rebased-From: 609685107bfb51d2ca8ab034694ad771ad267649

maflcko · 2019-05-23T19:34:24Z

utACK 9b1b9cf5632be18f3ad1bf2e10ebd004fceb2368

maflcko · 2019-05-25T16:59:11Z

@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);
      |                                                                                                                           ~~~~~^~~

maflcko · 2019-06-21T12:20:14Z

@Sjors ^ 👀

Sjors · 2019-06-21T14:28:45Z

Soon (TM), sorry for the delay.

Sjors · 2019-06-26T00:34:56Z

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

meshcollider

Code review ACK f847fa961c29cf95416ba489d46d2a61368c7c61

test/functional/rpc_psbt.py

Sjors · 2019-06-26T20:07:50Z

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

ACK 4348388

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

l2a5b1 · 2019-07-10T20:06:20Z

src/rpc/rawtransaction_util.cpp

 #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)


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.

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

promag

Concept ACK.

test/functional/rpc_psbt.py

maflcko · 2019-07-18T19:34:33Z

Moved to 0.18.2

Sjors · 2019-07-27T17:40:51Z

Rebased and addressed nits.

achow101 · 2019-07-30T20:35:08Z

Code Review ACK d6b3640

Sjors · 2019-08-01T16:30:50Z

@MarcoFalke @l2a5b1 @meshcollider can you re-ACK?

l2a5b1 · 2019-08-02T11:49:25Z

re-ACK d6b3640

maflcko · 2019-08-02T12:50:52Z

ACK d6b3640

Show signature and timestamp

Signature:

Timestamp of file with hash 388665302574fab8386985975f35ab1e4733960dbf1495ad6009328b30e160eb -

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 · 2019-08-14T08:20:18Z

@Sjors Are you interested in backporting this?

Sjors · 2019-08-14T11:44:44Z

@fanquake backporting to 0.18 in #16608

…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

Github-Pull: bitcoin#15911 Partial-Rebased-From: 0580701e3e6503f0c3e6b844b2c0c48fa7c8caa4

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

Sjors force-pushed the 2019/04/walletcreatefundedpsbt branch from aafdfde to 6096851 Compare April 27, 2019 18:29

fanquake added the RPC/REST/ZMQ label Apr 27, 2019

luke-jr suggested changes May 1, 2019

View reviewed changes

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

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

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

[rpc] walletcreatefundedpsbt: use wallet default RBF

fb40737

Github-Pull: bitcoin#15911 Rebased-From: 609685107bfb51d2ca8ab034694ad771ad267649

Sjors mentioned this pull request May 12, 2019

walletcreatefundedpsbt ignores walletrbf setting #15878

Closed

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

Sjors mentioned this pull request May 12, 2019

[WIP] External signer support (e.g. hardware wallet) #14912

Closed

maflcko added the Needs backport label May 16, 2019

maflcko added this to the 0.18.1 milestone May 16, 2019

maflcko added Wallet Waiting for author labels Jun 21, 2019

CLEANSKY97 approved these changes Jun 22, 2019

View reviewed changes

Sjors force-pushed the 2019/04/walletcreatefundedpsbt branch from 9b1b9cf to f847fa9 Compare June 26, 2019 00:34

meshcollider removed the Waiting for author label Jun 26, 2019

meshcollider reviewed Jun 26, 2019

View reviewed changes

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

Sjors force-pushed the 2019/04/walletcreatefundedpsbt branch from f847fa9 to 07299aa Compare June 26, 2019 20:07

l2a5b1 reviewed Jul 10, 2019

View reviewed changes

l2a5b1 reviewed Jul 11, 2019

View reviewed changes

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

Sjors mentioned this pull request Jul 12, 2019

The ultimate send RPC #16378

Merged

3 tasks

promag reviewed Jul 14, 2019

View reviewed changes

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

maflcko removed this from the 0.18.1 milestone Jul 18, 2019

maflcko added this to the 0.18.2 milestone Jul 18, 2019

fanquake added the Waiting for author label Jul 25, 2019

DrahtBot added the Needs rebase label Jul 27, 2019

Sjors added 3 commits July 27, 2019 19:24

[rpc] walletcreatefundedpsbt: use wallet default RBF

4fcb698

[doc] rpc: remove "fallback to" from RBF default help

9ed062b

[test] walletcreatefundedpsbt: check RBF is disabled when -walletrbf=0

d6b3640

Sjors force-pushed the 2019/04/walletcreatefundedpsbt branch from 17f0cb0 to d6b3640 Compare July 27, 2019 17:38

DrahtBot removed the Needs rebase label Jul 27, 2019

maflcko merged commit d6b3640 into bitcoin:master Aug 2, 2019

fanquake removed the Waiting for author label Aug 2, 2019

Sjors deleted the 2019/04/walletcreatefundedpsbt branch August 2, 2019 15:40

Sjors mentioned this pull request Aug 14, 2019

[0.18] Backport #15911: Use wallet RBF default for walletcreatefundedpsbt #16608

Merged

fanquake removed the Needs backport label Aug 14, 2019

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Sep 3, 2019

QA: rpc_psbt: Test wallet default RBF

365a50c

Github-Pull: bitcoin#15911 Partial-Rebased-From: 0580701e3e6503f0c3e6b844b2c0c48fa7c8caa4

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021

Use wallet RBF default for walletcreatefundedpsbt #15911

Use wallet RBF default for walletcreatefundedpsbt #15911

Uh oh!

Conversation

Sjors commented Apr 27, 2019 • edited by maflcko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Apr 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Apr 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Uh oh!

Uh oh!

maflcko commented May 23, 2019

Uh oh!

maflcko commented May 25, 2019

Uh oh!

maflcko commented Jun 21, 2019

Uh oh!

Sjors commented Jun 21, 2019

Uh oh!

Sjors commented Jun 26, 2019

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Jun 26, 2019

Uh oh!

l2a5b1 left a comment

Choose a reason for hiding this comment

Uh oh!

l2a5b1 Jul 10, 2019

Choose a reason for hiding this comment

Uh oh!

Sjors Jul 11, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

maflcko commented Jul 18, 2019

Uh oh!

Sjors commented Jul 27, 2019

Uh oh!

achow101 commented Jul 30, 2019

Uh oh!

Sjors commented Aug 1, 2019

Uh oh!

l2a5b1 commented Aug 2, 2019

Uh oh!

maflcko commented Aug 2, 2019

Uh oh!

fanquake commented Aug 14, 2019

Uh oh!

Sjors commented Aug 14, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

Sjors commented Apr 27, 2019 •

edited by maflcko

Loading

Sjors commented Apr 27, 2019 •

edited

Loading

DrahtBot commented Apr 27, 2019 •

edited

Loading