rpc, gui: bumpfee signer support #21576

Sjors · 2021-04-02T18:38:58Z

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

Sjors · 2021-04-02T18:39:56Z

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 · 2021-04-03T01:22:13Z

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.

meshcollider

utACK 0c0d293

src/wallet/rpcwallet.cpp

jonatack · 2021-05-30T09:06:41Z

src/wallet/rpcwallet.cpp

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


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

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

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 · 2021-05-31T15:31:22Z

Rebased, addressed comments and added a test.

meshcollider

re-utACK dda02ce

Sjors · 2021-12-21T02:54:14Z

Oops, fixed.

achow101 · 2021-12-21T20:22:49Z

ACK d5f720f

Sjors · 2022-01-12T15:47:38Z

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

luke-jr · 2022-04-23T15:10:49Z

src/wallet/rpc/spend.cpp

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


Suggested change

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

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

& other such changes

Fixed (after the rebase moved everything around)

Sjors · 2022-04-25T15:56:13Z

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

Sjors · 2022-04-25T16:10:59Z

Ok, it works again now.

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

Github-Pull: bitcoin#21576 Rebased-From: 7e02a33

Specifically this enables the Send button in the fee bump dialog for wallets with external signer support. Similar to 2efdfb8. Github-Pull: bitcoin#21576 Rebased-From: 2c07cfa

furszy

code review ACK 2c07cfa

furszy · 2022-06-05T15:41:53Z

src/wallet/feebumper.cpp

+        // Make a blank psbt
+        PartiallySignedTransaction psbtx(mtx);
+
+        // First fill transaction with our data without signing,
+        // so external signers are not asked to sign more than once.
+        bool complete;
+        wallet.FillPSBT(psbtx, complete, SIGHASH_ALL, false /* sign */, true /* bip32derivs */);
+        const TransactionError err = wallet.FillPSBT(psbtx, complete, SIGHASH_ALL, true /* sign */, false  /* bip32derivs */);
+        if (err != TransactionError::OK) return false;
+        complete = FinalizeAndExtractPSBT(psbtx, mtx);
+        return complete;


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.

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: Sjors@5d55117 (because it won't add the bip32 info for some reason)

jarolrod

Approach ACK

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

achow101 · 2022-12-15T21:15:00Z

ACK 2c07cfa

jarolrod

tACK 2c07cfa

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

2c07cfa gui: bumpfee signer support (Sjors Provoost) 7e02a33 rpc: bumpfee signer support (Sjors Provoost) 304ece9 rpc: document bools in FillPSBT() calls (Sjors Provoost) Pull request description: The `bumpfee` RPC call and GUI fee bump interface now work with an external signer. ACKs for top commit: achow101: ACK 2c07cfa furszy: code review ACK 2c07cfa jarolrod: tACK 2c07cfa Tree-SHA512: 0c7b931f76fac67c9e33b9b935f29af6f69ac67a5ffcc586ed2f1676feac427735b1d971723b29ef332bb6fb5762949598ebbf728587e8f0ded95a9bfbb3e7a4

DrahtBot added Build system RPC/REST/ZMQ Wallet labels Apr 2, 2021

Sjors force-pushed the 2021/04/signer_bumpfee branch from e4818a6 to 0c0d293 Compare April 27, 2021 07:46

Sjors marked this pull request as ready for review April 27, 2021 07:54

meshcollider reviewed May 30, 2021

View reviewed changes

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

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

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

jonatack reviewed May 30, 2021

View reviewed changes

Sjors force-pushed the 2021/04/signer_bumpfee branch from 0c0d293 to dda02ce Compare May 31, 2021 15:31

meshcollider reviewed Jun 1, 2021

View reviewed changes

DrahtBot mentioned this pull request Jun 4, 2021

Basic Taproot signing support for descriptor wallets #21365

Merged

DrahtBot added the Needs rebase label Jun 17, 2021

Sjors force-pushed the 2021/04/signer_bumpfee branch from dda02ce to c7e4cc0 Compare June 18, 2021 15:24

DrahtBot removed the Needs rebase label Jun 18, 2021

This was referenced Jul 24, 2021

rpc: Allow walletprocesspsbt to sign without finalizing #22513

Merged

psbt: Taproot fields for PSBT #22558

Merged

DrahtBot added the Needs rebase label Jan 9, 2022

Sjors force-pushed the 2021/04/signer_bumpfee branch from d5f720f to ea8f5db Compare January 12, 2022 15:52

DrahtBot removed the Needs rebase label Jan 12, 2022

DrahtBot mentioned this pull request Feb 10, 2022

Add 'sendall' RPC née sweep #24118

Merged

DrahtBot added the Needs rebase label Mar 30, 2022

luke-jr reviewed Apr 23, 2022

View reviewed changes

rpc: document bools in FillPSBT() calls

304ece9

Sjors force-pushed the 2021/04/signer_bumpfee branch from ea8f5db to 3e25b8e Compare April 25, 2022 15:46

Sjors force-pushed the 2021/04/signer_bumpfee branch from 3e25b8e to ed5538e Compare April 25, 2022 16:09

Sjors added 2 commits April 25, 2022 18:13

rpc: bumpfee signer support

7e02a33

gui: bumpfee signer support

2c07cfa

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

Sjors force-pushed the 2021/04/signer_bumpfee branch from ed5538e to 2c07cfa Compare April 25, 2022 16:14

DrahtBot removed the Needs rebase label Apr 25, 2022

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request May 21, 2022

rpc: bumpfee signer support

b2424b5

Github-Pull: bitcoin#21576 Rebased-From: 7e02a33

furszy reviewed Jun 5, 2022

View reviewed changes

jarolrod reviewed Oct 14, 2022

View reviewed changes

fanquake removed the Build system label Oct 31, 2022

DrahtBot mentioned this pull request Nov 12, 2022

RPC: Accept options as named-only parameters #26485

Merged

jarolrod approved these changes Dec 17, 2022

View reviewed changes

achow101 merged commit cbcad79 into bitcoin:master Dec 20, 2022

Sjors deleted the 2021/04/signer_bumpfee branch January 2, 2023 11:07

bitcoin locked and limited conversation to collaborators Jan 2, 2024

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

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

rpc, gui: bumpfee signer support #21576

rpc, gui: bumpfee signer support #21576

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Reviews

Conflicts

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!

Uh oh!

Uh oh!

Uh oh!

jonatack May 30, 2021

Choose a reason for hiding this comment

Uh oh!

Sjors May 31, 2021

Choose a reason for hiding this comment

Uh oh!

Sjors May 31, 2021 • 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!

Sjors commented May 31, 2021

Uh oh!

meshcollider left a comment

Choose a reason for hiding this comment

Uh oh!

Sjors commented Dec 21, 2021

Uh oh!

achow101 commented Dec 21, 2021

Uh oh!

Sjors commented Jan 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr Apr 23, 2022

Choose a reason for hiding this comment

Uh oh!

Sjors Apr 25, 2022

Choose a reason for hiding this comment

Uh oh!

Sjors commented Apr 25, 2022

Uh oh!

Sjors commented Apr 25, 2022

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

furszy Jun 5, 2022

Choose a reason for hiding this comment

Uh oh!

Sjors Jul 18, 2022 • 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!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Dec 15, 2022

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Sjors commented Apr 2, 2021 •

edited

Loading

Sjors commented Apr 2, 2021 •

edited

Loading

DrahtBot commented Apr 3, 2021 •

edited

Loading

meshcollider left a comment •

edited

Loading

Sjors May 31, 2021 •

edited

Loading

Sjors commented Jan 12, 2022 •

edited

Loading

Sjors Jul 18, 2022 •

edited

Loading