rpc, gui: bumpfee signer support

Sjors commented at 6:38 pm on April 2, 2021: member

The bumpfee RPC call and GUI fee bump interface now work with an external signer.

Sjors commented at 6:39 pm on April 2, 2021: member

I’m not tremendously excited by this approach of overloading bumpfee. But it also seems wrong to use psbtbumpfee here for the simple case of a single signer that has all the required keys. I could add yet another RPC method signerbumpfee?

cc @achow101 @instagibbs

DrahtBot added the label Build system on Apr 2, 2021

DrahtBot added the label RPC/REST/ZMQ on Apr 2, 2021

DrahtBot added the label Wallet on Apr 2, 2021

DrahtBot commented at 1:22 am on April 3, 2021: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	furszy, achow101, jarolrod
Concept ACK	luke-jr
Stale ACK	meshcollider

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26485 (RPC: Accept options as named-only parameters 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 force-pushed on Apr 27, 2021

Sjors marked this as ready for review on Apr 27, 2021

in src/wallet/rpcwallet.cpp:3529 in 0c0d293d31 outdated

3531-        uint256 txid;
3532-        if (feebumper::CommitTransaction(*pwallet, hash, std::move(mtx), errors, txid) != feebumper::Result::OK) {
3533-            throw JSONRPCError(RPC_WALLET_ERROR, errors[0].original);
3534-        }
3535+            // First fill transaction with our data without signing,
3536+            // so external signers are not asked sign more than once.

meshcollider commented at 8:56 am on May 30, 2021:

nit: asked sign -> asked to sign

in src/wallet/rpcwallet.cpp:3532 in 0c0d293d31 outdated

3534-        }
3535+            // First fill transaction with our data without signing,
3536+            // so external signers are not asked sign more than once.
3537+            bool complete;
3538+            pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, false, true);
3539+            const TransactionError err = pwallet->FillPSBT(psbtx, complete, SIGHASH_ALL, true, false);

meshcollider commented at 8:57 am on May 30, 2021:

style nit: would be good to include the usual /* sign */ etc. comments for the bools here.

in src/wallet/rpcwallet.cpp:3537 in 0c0d293d31 outdated

3540+            if (err != TransactionError::OK) {
3541+                throw JSONRPCTransactionError(err);
3542+            }
3543 
3544-        result.pushKV("txid", txid.GetHex());
3545+            CMutableTransaction mtx;

meshcollider commented at 8:58 am on May 30, 2021:

redefinition of mtx?

meshcollider commented at 9:00 am on May 30, 2021: contributor

utACK 0c0d293d31e96c9119e4793df66b375ec9f8495e

in src/wallet/rpcwallet.cpp:3522 in 0c0d293d31 outdated

3520@@ -3521,16 +3521,39 @@ static RPCHelpMan bumpfee_helper(std::string method_name)
3521     // If wallet private keys are enabled, return the new transaction id,
3522     // otherwise return the base64-encoded unsigned PSBT of the new transaction.

jonatack commented at 9:06 am on May 30, 2021:

Do the merged changes in #22021 need to be updated with this pull?

Sjors commented at 1:53 pm on May 31, 2021:

I’ll do a rebase and also address the nits above.

Sjors commented at 3:07 pm on May 31, 2021:

For now these comments are correct. In a multisig setup bumpfee will throw "Transaction incomplete. Try psbtbumpfee instead." (which in turn won’t call the external signer). We can improve that later.

Sjors commented at 3:31 pm on May 31, 2021: member

Rebased, addressed comments and added a test.

Sjors force-pushed on May 31, 2021

meshcollider commented at 1:19 am on June 1, 2021: contributor

re-utACK dda02ce4f99cff37bbd41373df6ecb4541ed66f5

DrahtBot added the label Needs rebase on Jun 17, 2021

Sjors force-pushed on Jun 18, 2021

DrahtBot removed the label Needs rebase on Jun 18, 2021

in src/wallet/rpcwallet.cpp:4200 in c7e4cc0c95 outdated

4194@@ -4174,10 +4195,10 @@ static RPCHelpMan send()
4195             PartiallySignedTransaction psbtx(rawTx);
4196 
4197             // First fill transaction with our data without signing,
4198-            // so external signers are not asked sign more than once.
4199+            // so external signers are not asked to sign more than once.
4200             bool complete;
4201-            pwallet->FillPSBT(psbtx, complete, SIGHASH_DEFAULT, false, true);
4202-            const TransactionError err = pwallet->FillPSBT(psbtx, complete, SIGHASH_DEFAULT, true, false);
4203+            pwallet->FillPSBT(psbtx, complete, SIGHASH_DEFAULT, false /* sign */, true /* bip32derivs */);

luke-jr commented at 1:46 am on September 1, 2021:

Usually we put the meaning-comment before the value.

Sjors commented at 2:05 pm on September 1, 2021:

That indeed seems more common (*/, has 93 results vs 441 for , /* )

in src/wallet/rpcwallet.cpp:3552 in c7e4cc0c95 outdated

3555+
3556+            if (!complete) {
3557+                throw JSONRPCError(RPC_WALLET_ERROR, "Transaction incomplete. Try psbtbumpfee instead.");
3558+            }
3559+            CTransactionRef tx(MakeTransactionRef(std::move(mtx)));
3560+            result.pushKV("txid", tx->GetHash().GetHex());

luke-jr commented at 1:51 am on September 1, 2021:

Prefer to assign txid here and do a single shared pushKV later.

luke-jr approved

luke-jr commented at 2:00 am on September 1, 2021: member

utACK

luke-jr commented at 2:04 am on September 1, 2021: member

On further reflection, any reason not to put these details in feebumper::SignTransaction so it’s more easily reused in the GUI?

Sjors force-pushed on Sep 1, 2021

Sjors commented at 2:54 pm on September 1, 2021: member

@luke-jr good idea: now it works with GUI too!

Sjors renamed this:
~~rpc: bumpfee signer support~~
rpc, gui: bumpfee signer support
on Sep 1, 2021

Sjors force-pushed on Sep 1, 2021

luke-jr referenced this in commit 2f54a8d7f7 on Oct 10, 2021

DrahtBot added the label Needs rebase on Dec 8, 2021

Sjors force-pushed on Dec 8, 2021

Sjors commented at 5:46 am on December 8, 2021: member

Rebased after #23667 (manually re-applied changes from wallet/rpcwallet.cpp to wallet/rpc/spend.cpp)

DrahtBot removed the label Needs rebase on Dec 8, 2021

Sjors force-pushed on Dec 9, 2021

achow101 commented at 9:35 pm on December 20, 2021: member

There was a bad rebase, you’ve re-added src/wallet/rpcwallet.cpp.

Sjors force-pushed on Dec 21, 2021

Sjors commented at 2:54 am on December 21, 2021: member

Oops, fixed.

achow101 commented at 8:22 pm on December 21, 2021: member

ACK d5f720fb48d3eeb64101af1d5614a3650643bb1e

DrahtBot added the label Needs rebase on Jan 9, 2022

Sjors commented at 3:47 pm on January 12, 2022: member

Rebased after bitcoin-core/gui#441, though not tested yet.

Sjors force-pushed on Jan 12, 2022

DrahtBot removed the label Needs rebase on Jan 12, 2022

DrahtBot added the label Needs rebase on Mar 30, 2022

in src/wallet/rpc/spend.cpp:1130 in ea8f5db52b outdated

1128+            // so external signers are not asked to sign more than once.
1129             bool complete;
1130-            pwallet->FillPSBT(psbtx, complete, SIGHASH_DEFAULT, false, true);
1131-            const TransactionError err = pwallet->FillPSBT(psbtx, complete, SIGHASH_DEFAULT, true, false);
1132+            pwallet->FillPSBT(psbtx, complete, SIGHASH_DEFAULT, /* sign */ false, /* bip32derivs */ true);
1133+            const TransactionError err = pwallet->FillPSBT(psbtx, complete, SIGHASH_DEFAULT, /* sign */ true, /* bip32derivs */ false);

luke-jr commented at 3:10 pm on April 23, 2022:

0            const TransactionError err = pwallet->FillPSBT(psbtx, complete, SIGHASH_DEFAULT, /*sign=*/true, /*bip32derivs=*/false);

& other such changes

Sjors commented at 3:45 pm on April 25, 2022:

Fixed (after the rebase moved everything around)

rpc: document bools in FillPSBT() calls 304ece9945

Sjors force-pushed on Apr 25, 2022

Sjors commented at 3:56 pm on April 25, 2022: member

Rebased, but for the GUI to work I first need to apply the same change as bitcoin-core/gui#555

Sjors force-pushed on Apr 25, 2022

Sjors commented at 4:10 pm on April 25, 2022: member

Ok, it works again now.

rpc: bumpfee signer support 7e02a33297

gui: bumpfee signer support

Specifically this enables the Send button in the fee bump dialog for wallets with external signer support. Similar to 2efdfb88aab6496dcf2b98e0de30635bc6bade85.

2c07cfacd1

Sjors force-pushed on Apr 25, 2022

DrahtBot removed the label Needs rebase on Apr 25, 2022

luke-jr referenced this in commit b2424b5573 on May 21, 2022

luke-jr referenced this in commit ab92f28122 on May 21, 2022

in src/wallet/feebumper.cpp:254 in 2c07cfacd1

250+        bool complete;
251+        wallet.FillPSBT(psbtx, complete, SIGHASH_ALL, false /* sign */, true /* bip32derivs */);
252+        const TransactionError err = wallet.FillPSBT(psbtx, complete, SIGHASH_ALL, true /* sign */, false  /* bip32derivs */);
253+        if (err != TransactionError::OK) return false;
254+        complete = FinalizeAndExtractPSBT(psbtx, mtx);
255+        return complete;

furszy commented at 3:41 pm on June 5, 2022:

nit: as this is the same code that we have in “rpc/send” –> FinishTransaction, could decouple it into a new function and use it from both sides.

Sjors commented at 2:58 pm on July 18, 2022:

I looked into deduplicating the part where we fill without signing and then call fill, but it’s not that easy. The following doesn’t work: https://github.com/Sjors/bitcoin/commit/5d55117e53c6a460222dea99842edc4fda47622c (because it won’t add the bip32 info for some reason)

furszy commented at 3:51 pm on June 5, 2022: member

code review ACK 2c07cfac

jarolrod commented at 5:23 am on October 14, 2022: member

Approach ACK

I was able to successfully sign for a fee increase on my external signer. Want to continue to do more testing.

fanquake removed the label Build system on Oct 31, 2022

achow101 commented at 9:15 pm on December 15, 2022: member

ACK 2c07cfacd1745844a1d3c57f2e8617549b9815d7

jarolrod approved

jarolrod commented at 1:54 am on December 17, 2022: member

tACK 2c07cfa

I have tested and reviewed the code and agree it can be merged.

achow101 merged this on Dec 20, 2022

achow101 closed this on Dec 20, 2022

sidhujag referenced this in commit ee9fae8586 on Dec 21, 2022

Sjors deleted the branch on Jan 2, 2023

bitcoin locked this on Jan 2, 2024

rpc, gui: bumpfee signer support #21576

Reviews

Conflicts