rpc: separate bumpfee's psbt creation function into psbtbumpfee

achow101 commented at 6:52 pm on April 15, 2020: member

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

achow101 renamed this:
~~rpc: separate bumpfee'~~
rpc: separate bumpfee's psbt creation function into psbtbumpfee
on Apr 15, 2020

DrahtBot added the label RPC/REST/ZMQ on Apr 15, 2020

DrahtBot added the label Tests on Apr 15, 2020

DrahtBot added the label Wallet on Apr 15, 2020

DrahtBot commented at 11:56 pm on April 15, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#19262 (rpc: Replace OMITTED_NAMED_ARG with OMITTED by MarcoFalke)
#19168 (Refactor: Improve setup_clean_chain semantics by fjahr)
#18354 (Use shared pointers only in validation interface by bvbfan)
#16365 (Log RPC parameters (arguments) if -debug=rpcparams by LarryRuane)
#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 commented at 6:15 pm on April 22, 2020: member

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

MarcoFalke commented at 6:28 pm on April 22, 2020: member

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.

in src/wallet/rpcwallet.cpp:3569 in 1f22b55fad outdated

3564+                        }},
3565+                    }
3566+                },
3567+                RPCExamples{
3568+            "\nBump the fee, get the new transaction\'s psbt\n" +
3569+                    HelpExampleCli("bumpfee", "<txid>")

Sjors commented at 3:46 pm on April 23, 2020:

/bumpfee/psbtbumpfee

achow101 commented at 7:55 pm on April 23, 2020:

Done

Sjors commented at 3:55 pm on April 23, 2020: member

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?

achow101 force-pushed on Apr 23, 2020

achow101 commented at 7:57 pm on April 23, 2020: member

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 commented at 6:23 pm on April 24, 2020: member

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.

DrahtBot added the label Needs rebase on Apr 27, 2020

promag commented at 11:00 am on April 27, 2020: member

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

achow101 force-pushed on Apr 27, 2020

DrahtBot removed the label Needs rebase on Apr 27, 2020

DrahtBot added the label Needs rebase on May 4, 2020

achow101 force-pushed on May 4, 2020

DrahtBot removed the label Needs rebase on May 4, 2020

luke-jr commented at 10:06 pm on May 18, 2020: member

Thoughts on using the same function for both calls, and simply checking request.strMethod to decide to bail out early?

achow101 commented at 8:50 pm on May 19, 2020: member

Thoughts on using the same function for both calls, and simply checking request.strMethod to decide to bail out early?

That’s kind of what the first commit is doing. Most of the duplication is in the help text, which I don’t think can be really deduplicated since some differences are required.

luke-jr referenced this in commit bebb8871b8 on May 20, 2020

luke-jr referenced this in commit 6f339da07f on May 20, 2020

luke-jr commented at 10:53 pm on May 20, 2020: member

I was thinking something more like this: https://github.com/bitcoin/bitcoin/compare/0.20...luke-jr:rpc_psbtbumpfee-0.20

Sjors commented at 9:07 am on May 28, 2020: member

I like @luke-jr’s approach. We can always duplicate the code if it these branches become too tedious.

Sjors commented at 10:37 am on June 8, 2020: member

See also #19192 for another approach to de-duplicating RPC help copy.

DrahtBot added the label Needs rebase on Jun 11, 2020

achow101 force-pushed on Jun 12, 2020

achow101 commented at 4:42 pm on June 12, 2020: member

Rebased this and changed it to be similar to @luke-jr suggestion.

achow101 force-pushed on Jun 12, 2020

DrahtBot removed the label Needs rebase on Jun 12, 2020

DrahtBot added the label Needs rebase on Jun 12, 2020

in src/wallet/rpcwallet.cpp:3283 in b8f0e96ed5 outdated

3279@@ -3277,11 +3280,18 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3280                     }
3281                 },
3282                 RPCExamples{
3283-            "\nBump the fee, get the new transaction\'s txid\n" +
3284+            "\nBump the fee, get the new transaction\'s" + std::string(want_psbt ? "psbt" : "txid\n") +

luke-jr commented at 7:01 pm on June 12, 2020:

Missing space after “transaction's”

luke-jr commented at 7:02 pm on June 12, 2020:

“\n” should be outside conditional

achow101 commented at 8:55 pm on June 12, 2020:

Fixed.

achow101 commented at 8:55 pm on June 12, 2020:

Fixed.

in src/wallet/rpcwallet.cpp:3284 in b8f0e96ed5 outdated

3279@@ -3277,11 +3280,18 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3280                     }
3281                 },
3282                 RPCExamples{
3283-            "\nBump the fee, get the new transaction\'s txid\n" +
3284+            "\nBump the fee, get the new transaction\'s" + std::string(want_psbt ? "psbt" : "txid\n") +
3285                     HelpExampleCli("bumpfee", "<txid>")

luke-jr commented at 7:02 pm on June 12, 2020:

Example should use request.strMethod

achow101 commented at 8:55 pm on June 12, 2020:

Fixed.

in src/wallet/rpcwallet.cpp:3273 in b8f0e96ed5 outdated

3268@@ -3266,7 +3269,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3269                 },
3270                 RPCResult{
3271                     RPCResult::Type::OBJ, "", "", {
3272-                        {RPCResult::Type::STR, "psbt", "The base64-encoded unsigned PSBT of the new transaction. Only returned when wallet private keys are disabled."},
3273+                        {RPCResult::Type::STR, "psbt", "The base64-encoded unsigned PSBT of the new transaction." + std::string(want_psbt ? "" : " Only returned when wallet private keys are disabled. (DEPRECATED)")},
3274                         {RPCResult::Type::STR_HEX, "txid", "The id of the new transaction. Only returned when wallet private keys are enabled."},

luke-jr commented at 7:03 pm on June 12, 2020:

Needs to be hidden for psbtbumpfee.

achow101 commented at 8:55 pm on June 12, 2020:

Oh.. I’ve grabbed your commit that adds RPCResult::Type::HIDDEN to handle this.

luke-jr changes_requested

luke-jr referenced this in commit ecb7e79b0d on Jun 12, 2020

achow101 force-pushed on Jun 12, 2020

DrahtBot removed the label Needs rebase on Jun 12, 2020

MarcoFalke removed the label Tests on Jun 13, 2020

in src/wallet/rpcwallet.cpp:3268 in de0ad3a23c outdated

3263@@ -3261,8 +3264,8 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3264                 },
3265                 RPCResult{
3266                     RPCResult::Type::OBJ, "", "", {
3267-                        {RPCResult::Type::STR, "psbt", "The base64-encoded unsigned PSBT of the new transaction. Only returned when wallet private keys are disabled."},
3268-                        {RPCResult::Type::STR_HEX, "txid", "The id of the new transaction. Only returned when wallet private keys are enabled."},
3269+                        {RPCResult::Type::STR, "psbt", "The base64-encoded unsigned PSBT of the new transaction." + std::string(want_psbt ? "" : " Only returned when wallet private keys are disabled. (DEPRECATED)")},
3270+                        {(want_psbt ? RPCResult::Type::HIDDEN : RPCResult::Type::STR_HEX), "txid", "The id of the new transaction. Only returned when wallet private keys are enabled."},

MarcoFalke commented at 12:46 pm on June 13, 2020:

I think it doesn’t make sense to push an rpc result into the help manager and then tell the manager to discard it. Wouldn’t it make more sense to not push it in the first place? This should be trivial to achieve with Cat

0RPCResult::Type::OBJ, "", "", Cat(Cat({
1    {STR, "psbt", ...},
2  },
3  std::vector<RPCResult>{want_psbt ? {} : {STR_HEX, "txid", ...}}),
4  {
5    {STR_AMOUNT, "origfee" ...},
6    {STR_AMOUNT, "fee", ...},
7    ....
8  }),

(I didnt’ compile this to check)

achow101 commented at 4:27 pm on June 13, 2020:

I agree that it doesn’t make sense to push and then hide. I was originally trying to do something similar to this but couldn’t get it to work. I was also unable to get your suggestion to work. It fails with

0wallet/rpcwallet.cpp:3270:58: error: no matching function for call to ‘std::vector<RPCResult>::vector(<brace-enclosed initializer list>)’
1 3270 |                         std::vector<RPCResult>{want_psbt ? {} : {RPCResult::Type::STR_HEX, "txid", "The id of the new transaction. Only returned when wallet private keys are enabled."}

I tried a few different variations of this but nothing would work.

MarcoFalke commented at 4:55 pm on June 13, 2020:

Ah, not your fault. That {} means both “constructor” and “initializer list” is one of the countless design flaws of c++. {{}} should work.

This diff compiles (haven’t tested what it doesn, though):

 0diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
 1index de21f23da3..e5ecc6a451 100644
 2--- a/src/wallet/rpcwallet.cpp
 3+++ b/src/wallet/rpcwallet.cpp
 4@@ -3263,16 +3263,16 @@ static UniValue bumpfee(const JSONRPCRequest& request)
 5                         "options"},
 6                 },
 7                 RPCResult{
 8-                    RPCResult::Type::OBJ, "", "", {
 9+                    RPCResult::Type::OBJ, "", "", Cat(Cat({
10                         {RPCResult::Type::STR, "psbt", "The base64-encoded unsigned PSBT of the new transaction." + std::string(want_psbt ? "" : " Only returned when wallet private keys are disabled. (DEPRECATED)")},
11-                        {(want_psbt ? RPCResult::Type::HIDDEN : RPCResult::Type::STR_HEX), "txid", "The id of the new transaction. Only returned when wallet private keys are enabled."},
12+                        },want_psbt ? std::vector<RPCResult>{} : std::vector<RPCResult>{{RPCResult::Type::STR_HEX, "txid", "The id of the new transaction. Only returned when wallet private keys are enabled."}}),{
13                         {RPCResult::Type::STR_AMOUNT, "origfee", "The fee of the replaced transaction."},
14                         {RPCResult::Type::STR_AMOUNT, "fee", "The fee of the new transaction."},
15                         {RPCResult::Type::ARR, "errors", "Errors encountered during processing (may be empty).",
16                         {
17                             {RPCResult::Type::STR, "", ""},
18                         }},
19-                    }
20+                    })
21                 },
22                 RPCExamples{
23             "\nBump the fee, get the new transaction\'s" + std::string(want_psbt ? "psbt" : "txid") + "\n" +

achow101 commented at 5:56 pm on June 13, 2020:

Done. Dropped the commit adding HIDDEN

achow101 force-pushed on Jun 13, 2020

in src/wallet/rpcwallet.cpp:3231 in 6d56e5e779 outdated

3225@@ -3226,8 +3226,11 @@ UniValue signrawtransactionwithwallet(const JSONRPCRequest& request)
3226 
3227 static UniValue bumpfee(const JSONRPCRequest& request)
3228 {
3229-            RPCHelpMan{"bumpfee",
3230+    bool want_psbt = request.strMethod == "psbtbumpfee";
3231+
3232+            RPCHelpMan{request.strMethod,

Sjors commented at 10:12 am on June 16, 2020:

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

achow101 commented at 9:19 pm on June 16, 2020:

Done

Sjors commented at 10:32 am on June 16, 2020: member

Reviewed 70e8422b585afa1ec623c62312764eb91dde4cd3

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:

0static UniValue psbtbumpfee(const JSONRPCRequest& request)
1{
2    return bumpfee(request);
3}

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

achow101 force-pushed on Jun 16, 2020

achow101 commented at 9:19 pm on June 16, 2020: member

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 commented at 3:38 pm on June 17, 2020: member

utACK 39d39f7b77c5ef39ec9e877715a06ce962c442d6

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.

DrahtBot added the label Needs rebase on Jun 25, 2020

Add psbtbumpfee RPC 4638224f64

Hide bumpfee's psbt creation behavior behind -deprecatedrpc

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

431071c28a

moveonly: Fix indentation in bumpfee RPC

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

79d6332e9e

achow101 force-pushed on Jun 25, 2020

DrahtBot removed the label Needs rebase on Jun 25, 2020

Sjors commented at 11:52 am on July 6, 2020: member

re-utACK 79d6332

meshcollider commented at 10:45 am on July 11, 2020: contributor

utACK 79d6332e9e4fc01e6418247c31e31b4faa1b3b84

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

instagibbs commented at 4:15 pm on July 13, 2020: member

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

t-bast referenced this in commit 3d4e00fe18 on Jul 27, 2020

luke-jr commented at 1:03 am on August 10, 2020: member

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

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

achow101 commented at 3:56 pm on August 10, 2020: member

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.

in src/wallet/rpcwallet.cpp:3394 in 4638224f64 outdated

3392@@ -3382,7 +3393,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3393 
3394     // If wallet private keys are enabled, return the new transaction id,

fjahr commented at 10:28 am on August 12, 2020:

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

in src/wallet/rpcwallet.cpp:3252 in 79d6332e9e

3259-                "Alternatively, the user can specify a fee_rate (" + CURRENCY_UNIT + " per kB) for the new transaction.\n"
3260-                "At a minimum, the new fee rate must be high enough to pay an additional new relay fee (incrementalfee\n"
3261-                "returned by getnetworkinfo) to enter the node's mempool.\n",
3262+    RPCHelpMan{request.strMethod,
3263+        "\nBumps the fee of an opt-in-RBF transaction T, replacing it with a new transaction B.\n"
3264+        + std::string(want_psbt ? "Returns a PSBT instead of creating and signing a new transaction.\n" : "") +

fjahr commented at 9:55 pm on August 12, 2020:

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.

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

fjahr approved

fjahr commented at 10:18 pm on August 12, 2020: member

Code review ACK 79d6332e9e4fc01e6418247c31e31b4faa1b3b84

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

meshcollider merged this on Aug 13, 2020

meshcollider closed this on Aug 13, 2020

sidhujag referenced this in commit 7f858cac62 on Aug 14, 2020

luke-jr referenced this in commit e4043d5f49 on Aug 15, 2020

DrahtBot locked this on Feb 15, 2022

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

Conflicts