rpc: separate bumpfee's psbt creation function into psbtbumpfee #18654

achow101 · 2020-04-15T18:52:46Z

Adds a new RPC psbtbumpfee which always creates a psbt. bumpfee will then only be able to create and broadcast fee bumping transactions instead of changing its behavior based on IsWalletSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS).

Split from #18627

DrahtBot · 2020-04-15T23:56:32Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

rpc: Replace OMITTED_NAMED_ARG with OMITTED #19262 (rpc: Replace OMITTED_NAMED_ARG with OMITTED by MarcoFalke)
Refactor: Improve setup_clean_chain semantics #19168 (Refactor: Improve setup_clean_chain semantics by fjahr)
Use shared pointers only in validation interface #18354 (Use shared pointers only in validation interface by bvbfan)
Log RPC parameters (arguments) if -debug=rpcparams #16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
Add loadwallet and createwallet load_on_startup options #15937 (Add loadwallet and createwallet load_on_startup options by ryanofsky)

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.

Sjors · 2020-04-22T18:15:29Z

Concept ACK. I wish there was a way to prevent duplicating walls of help text.

maflcko · 2020-04-22T18:28:25Z

I wish there was a way to prevent duplicating walls of help text.

RPCHelpMan can not be duplicated because it takes the RPC method's name, but anything you pass to RPCHelpMan are basic C++11 structs that can be made const globals (or returned by helper functions like GetBumpFee{HelpText,Args,...}.) and then passed into the rpc help man.

Sjors

What's your vision for (future) descriptor wallets with private keys as e.g. part of a multisig setup (or a coinjoin)? Should those use psbtbumpfee? And in the context of single signature hardware wallet setups, like #16546, can those still use bumpfee, because PSBT is only used internally?

src/wallet/rpcwallet.cpp

achow101 · 2020-04-23T19:57:59Z

What's your vision for (future) descriptor wallets with private keys as e.g. part of a multisig setup (or a coinjoin)? Should those use psbtbumpfee? And in the context of single signature hardware wallet setups, like #16546, can those still use bumpfee, because PSBT is only used internally?

The idea is for bumpfee to be used and available only when the wallet is actually able to create a complete transaction and broadcast it. So for multisigs and coinjoin, it would not be available. For single signature hardware wallets, it should be, but can give a generic error about the device not being available.

psbtbumpfee is always available but also intended for use with wallets where bumpfee is not available.

Sjors · 2020-04-24T18:23:05Z

Ok, that makes sense. Can you add some code comments around the bumpfee lines of code that need to be removed during deprecation (or wrap it in an if statement, even though that's currently redundant)? Maybe also clarify your intention in a source comment.

promag

Concept ACK. Could update doc/psbt.md. Needs release note tag.

src/wallet/rpcwallet.cpp

Sjors

Reviewed 70e8422

psbtbumpfee doesn't show up in the help list. You can fix that by adding a short wrapper and referencing that from CRPCCommand, or perhaps @MarcoFalke has a better idea:

static UniValue psbtbumpfee(const JSONRPCRequest& request)
{
    return bumpfee(request);
}

When a wallet has private keys, the PSBT isn't signed. That's either a bug or something to document.

Sjors · 2020-06-16T10:12:08Z

src/wallet/rpcwallet.cpp

-            RPCHelpMan{"bumpfee",
+    bool want_psbt = request.strMethod == "psbtbumpfee";
+
+            RPCHelpMan{request.strMethod,


nit: fix pre-existing indentation (in separate commit)

achow101 · 2020-06-16T21:19:29Z

psbtbumpfee doesn't show up in the help list. You can fix that by adding a short wrapper and referencing that from CRPCCommand, or perhaps @MarcoFalke has a better idea:

That's interesting. I've added the wrapper as you suggested.

When a wallet has private keys, the PSBT isn't signed. That's either a bug or something to document.

I think this is existing behavior.

Sjors · 2020-06-17T15:38:43Z

utACK 39d39f7

I think this is existing behavior.

Not really; if your wallet had private keys, it would never create a PSBT, signed or otherwise. But it can wait for another PR.

With psbtbumpfee, we can deprecate bumpfee's psbt creation behavior. So put that behind a -deprecatedrpc

Review this with -w to see that nothing actually changes.

Sjors · 2020-07-06T11:52:14Z

re-utACK 79d6332

meshcollider

utACK 79d6332

I wouldn't call the lack of signing the PSBT a bug, but something to document maybe, sure.

instagibbs · 2020-07-13T16:15:23Z

concept/approach ACK, these are advanced uses that can be adapted to easily.

Allow activating anchor outputs and have fully operating channels during normal operation (open, add/fulfill/fail htlcs, close). Interop testing has been done with lnd, and there is only one pending issue during mutual close, where they incorrectly compute the closing amounts, which they should fix soon. However, anchor outputs should NOT be activated yet as unilateral close scenario are not fully handled yet. We don't do any kind of automatic fee bumping either; this will be done later, once we have PSBT support and once bitcoind offers the `psbtbumpfee` RPC (see bitcoin/bitcoin#18654).

luke-jr · 2020-08-10T01:03:49Z

That's interesting. I've added the wrapper as you suggested.

Please at least add comments as to why it's there?

achow101 · 2020-08-10T15:56:05Z

That's interesting. I've added the wrapper as you suggested.

Please at least add comments as to why it's there?

Will do if I need to push again.

fjahr

Code review ACK 79d6332

Feel free to ignore my nits, maybe consider them for a follow-up.

fjahr · 2020-08-12T10:28:43Z

src/wallet/rpcwallet.cpp

@@ -3382,7 +3393,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)

    // If wallet private keys are enabled, return the new transaction id,


nit: This comment doesn't seem to fit here 100% anymore after the check for the wallet flag was moved forward.

fjahr · 2020-08-12T21:55:09Z

src/wallet/rpcwallet.cpp

-                "returned by getnetworkinfo) to enter the node's mempool.\n",
+    RPCHelpMan{request.strMethod,
+        "\nBumps the fee of an opt-in-RBF transaction T, replacing it with a new transaction B.\n"
+        + std::string(want_psbt ? "Returns a PSBT instead of creating and signing a new transaction.\n" : "") +


nit: Could be slightly improved if you have to retouch since the 'creating a new transaction' part is still true. It is just not signed and committed if I see that correctly.

Suggested change

+ std::string(want_psbt ? "Returns a PSBT instead of creating and signing a new transaction.\n" : "") +

+ std::string(want_psbt ? "Returns a PSBT with the new, unsigned transaction.\n" : "") +

…nto psbtbumpfee 79d6332 moveonly: Fix indentation in bumpfee RPC (Andrew Chow) 431071c Hide bumpfee's psbt creation behavior behind -deprecatedrpc (Andrew Chow) 4638224 Add psbtbumpfee RPC (Andrew Chow) Pull request description: Adds a new RPC `psbtbumpfee` which always creates a psbt. `bumpfee` will then only be able to create and broadcast fee bumping transactions instead of changing its behavior based on `IsWalletSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)`. Split from bitcoin#18627 ACKs for top commit: Sjors: re-utACK 79d6332 meshcollider: utACK 79d6332 fjahr: Code review ACK 79d6332 Tree-SHA512: 1c92c4b4461bb30e78be3ee73165f624398ef33996ce36043b61a8931be667030d0fca12fd0b30097b78c56e4e9092c69582b237cbdac51d56f6be23d8c0f1bb

…eid from bumpfee Needed for psbtbumpfee to show in help Github-Pull: bitcoin#18654 Rebased-From: 4638224

achow101 changed the title ~~rpc: separate bumpfee'~~ rpc: separate bumpfee's psbt creation function into psbtbumpfee Apr 15, 2020

This was referenced Apr 15, 2020

gui: Add bumpFeePSBT action instead of changing normal bumpfee behavior #18655

Closed

rpc: gui: Don't change behavior based on private keys disabled, instead add new buttons/rpcs/menu items #18627

Closed

DrahtBot added RPC/REST/ZMQ Tests Wallet labels Apr 15, 2020

This was referenced Apr 22, 2020

Allow simple multiwallet rpc calls #18734

Closed

External signer support - Wallet Box edition #16546

Merged

Sjors reviewed Apr 23, 2020

View reviewed changes

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

achow101 force-pushed the psbtbumpfee branch from 550337c to db34ea1 Compare April 23, 2020 19:55

DrahtBot mentioned this pull request Apr 23, 2020

UI external signer support (e.g. hardware wallet) #16549

Closed

DrahtBot added the Needs rebase label Apr 27, 2020

promag reviewed Apr 27, 2020

View reviewed changes

achow101 force-pushed the psbtbumpfee branch from db34ea1 to 9eae517 Compare April 27, 2020 19:44

DrahtBot removed the Needs rebase label Apr 27, 2020

DrahtBot mentioned this pull request Apr 28, 2020

Use shared pointers only in validation interface #18354

Closed

DrahtBot added the Needs rebase label May 4, 2020

achow101 force-pushed the psbtbumpfee branch from 9eae517 to 61d6e41 Compare May 4, 2020 18:14

DrahtBot removed the Needs rebase label May 4, 2020

DrahtBot removed the Needs rebase label Jun 12, 2020

maflcko removed the Tests label Jun 13, 2020

maflcko reviewed Jun 13, 2020

View reviewed changes

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

achow101 force-pushed the psbtbumpfee branch from de0ad3a to 70e8422 Compare June 13, 2020 17:56

Sjors reviewed Jun 16, 2020

View reviewed changes

achow101 force-pushed the psbtbumpfee branch from 70e8422 to 39d39f7 Compare June 16, 2020 21:18

DrahtBot mentioned this pull request Jun 16, 2020

rpc: Replace OMITTED_NAMED_ARG with OMITTED #19262

Closed

DrahtBot mentioned this pull request Jun 25, 2020

Remove the automatic creation and loading of the default wallet #15454

Merged

DrahtBot added the Needs rebase label Jun 25, 2020

achow101 added 3 commits June 25, 2020 15:32

Add psbtbumpfee RPC

4638224

Hide bumpfee's psbt creation behavior behind -deprecatedrpc

431071c

With psbtbumpfee, we can deprecate bumpfee's psbt creation behavior. So put that behind a -deprecatedrpc

moveonly: Fix indentation in bumpfee RPC

79d6332

Review this with -w to see that nothing actually changes.

achow101 force-pushed the psbtbumpfee branch from 39d39f7 to 79d6332 Compare June 25, 2020 22:11

DrahtBot removed the Needs rebase label Jun 25, 2020

meshcollider reviewed Jul 11, 2020

View reviewed changes

DrahtBot mentioned this pull request Jul 13, 2020

Log RPC parameters (arguments) if -debug=rpcparams #16365

Closed

achow101 mentioned this pull request Jul 17, 2020

Question: PSBT and RBF #19481

Closed

t-bast mentioned this pull request Jul 21, 2020

Activate anchor output in channels ACINQ/eclair#1491

Merged

fjahr approved these changes Aug 12, 2020

View reviewed changes

meshcollider merged commit 8a85377 into bitcoin:master Aug 13, 2020

luke-jr added a commit to bitcoinknots/bitcoin that referenced this pull request Aug 15, 2020

RPC/Wallet: Add wrapper psbtbumpfee function to get a different uniqu…

e4043d5

…eid from bumpfee Needed for psbtbumpfee to show in help Github-Pull: bitcoin#18654 Rebased-From: 4638224

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

		@@ -3382,7 +3393,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)

		// If wallet private keys are enabled, return the new transaction id,

	+ std::string(want_psbt ? "Returns a PSBT instead of creating and signing a new transaction.\n" : "") +
	+ std::string(want_psbt ? "Returns a PSBT with the new, unsigned transaction.\n" : "") +

rpc: separate bumpfee's psbt creation function into psbtbumpfee #18654

rpc: separate bumpfee's psbt creation function into psbtbumpfee #18654

Uh oh!

Conversation

achow101 commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

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

Uh oh!

Uh oh!

maflcko commented Apr 22, 2020

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Apr 23, 2020

Uh oh!

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

Uh oh!

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 Jun 16, 2020

Choose a reason for hiding this comment

Uh oh!

achow101 commented Jun 16, 2020

Uh oh!

Sjors commented Jun 17, 2020

Uh oh!

Sjors commented Jul 6, 2020

Uh oh!

meshcollider left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jul 13, 2020

Uh oh!

luke-jr commented Aug 10, 2020

Uh oh!

achow101 commented Aug 10, 2020

Uh oh!

fjahr left a comment

Choose a reason for hiding this comment

Uh oh!

fjahr Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

fjahr Aug 12, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

achow101 commented Apr 15, 2020 •

edited

Loading

DrahtBot commented Apr 15, 2020 •

edited

Loading

Sjors commented Apr 22, 2020 •

edited

Loading

Sjors commented Apr 24, 2020 •

edited

Loading

meshcollider left a comment •

edited

Loading