bumpfee: Return PSBT when wallet has privkeys disabled #16373

instagibbs · 2019-07-11T18:03:57Z

The main use-case here is for using with watch-only wallets with PSBT-signing cold wallets of all kinds.

DrahtBot · 2019-07-11T19:52:49Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17809 (rpc: Auto-format RPCResult by MarcoFalke)
#17804 (doc: Misc RPC help fixes by MarcoFalke)
#17566 (Switch to weight units for all feerates computation by darosior)
#17492 (QT: bump fee returns PSBT on clipboard for watchonly-only wallets by instagibbs)

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.

ryanofsky · 2019-07-11T20:04:35Z

Concept ACK. Seems reasonable and reasonably simple.

promag · 2019-07-12T00:41:33Z

Concept ACK, how about a new RPC?

Sjors · 2019-07-12T08:42:56Z

Concept ACK. In general it's useful to be able to opt-out of sending a transaction. There could be a boolean option broadcast (default yes) for that. When a transaction is incomplete, returning a PSBT makes sense; perhaps not necesaary to make that explicit. When it's complete, returning both a hex and a psbt makes sense to me.

instagibbs · 2019-07-12T14:07:30Z

@Sjors it's not quite not a "not broadcast" argument, since it's also not being submitted to wallet(walletbroadcast for reference). Going to keep the name/process for now unless I get a better suggestion.

instagibbs · 2019-07-12T14:09:50Z

@promag an additional RPC seems pretty heavy-weight. I think this feature is unobtrusive as-is. I'm -0 on the suggestion for now.

instagibbs · 2019-07-12T14:13:53Z

Pushed tests

promag · 2019-07-12T14:16:56Z

@instagibbs just asking because there's quite some PSBT calls:

analyzepsbt
combinepsbt
converttopsbt
createpsbt
decodepsbt
finalizepsbt
joinpsbts
utxoupdatepsbt
walletcreatefundedpsbt
walletprocesspsbt

One disadvantage with this approach is that if you use the new option in a <0.19 it will commit the transaction, otherwise this looks fine.

instagibbs · 2019-07-12T14:21:33Z

re:PSBT calls, it's a bit odd that for this one you don't actually supply the PSBT to bump(it's a fully-formed tx in wallet/mempool).

One disadvantage with this approach is that if you use the new option in a <0.19 it will commit the transaction

Ok that's a solid point. I'll see about splitting it out.

Sjors · 2019-07-12T14:31:06Z

you don't actually supply the PSBT to bump(it's a fully-formed tx in wallet/mempool).

But the mempool doesn't track BIP32 paths.

promag · 2019-07-12T15:29:06Z

@instagibbs a new parameter instead of the options key wouldn't have the above issue.

instagibbs · 2019-07-16T00:28:18Z

made the parameter its own top-level named argument to avoid potential footguns, h/t @promag

ryanofsky

utACK b5ffbcb18d377238fba0ab4262d2d874e5d43d6d. Could add release notes advertising the new feature, but I don't think there are backwards compatibility concerns, so this shouldn't be strictly necessary.

Following promag's comment #16373 (comment), I could imagine wanting to add a bumpfeepsbt alias in the future to make the interface more consistent and discoverable for psbt users.

One disadvantage with this approach is that if you use the new option in a <0.19 it will commit the transaction, otherwise this looks fine.

Not really relevant anymore, but I don't think this is true. It seems like unrecognized options would always cause errors due to "strict" RPCTypeCheckObj checking: https://github.com/bitcoin/bitcoin/blob/v0.18.0/src/wallet/rpcwallet.cpp#L3282

promag · 2019-07-23T19:12:31Z

@ryanofsky oh right, strict option! I still prefer the new parameter along with named parameters.

BTW, what if the new parameter is commit = true instead?

ryanofsky · 2019-07-24T17:09:50Z

BTW, what if the new parameter is commit = true instead?

Not sure if this question is for me, but that seems fine. I don't have any preference between options vs params or return_psbt vs commit, they all seem fine. I do think having separate bumpfee and bumpfeepsbt methods would be most discoverable solution and most consistent with other psbt functions, but I know our current RPC aliasing and help infrastructure make this cumbersome and probably not worth the complexity right now.

instagibbs · 2019-07-26T17:42:18Z

I find commit easier to remember but not particularly self-descriptive for a user. -0 on the change, but others feel free to bikeshed :)

Sjors · 2019-08-15T17:15:55Z

I ended up with add_to_wallet in #16378, and if the transaction is complete I return the hex in addition to PSBT.

promag · 2019-08-15T23:22:31Z

Yap, my point is that the new option "decides what to do" instead of "what to return". add_to_wallet LGTM.

meshcollider · 2019-08-16T21:56:08Z

Concept ACK with a bit of light code review

instagibbs · 2019-08-17T15:07:19Z

updated to add_to_wallet argument name/logic.

luke-jr · 2019-08-19T23:56:51Z

src/wallet/rpcwallet.cpp

This seems like it shouldn't be a positional parameter...

fwiw that point was discussed above

I don't understand the point above. We call RPCTypeCheckObj with fStrict=true, so unrecognised keys will error...

Yeah I missed that (the strict option) when I left my comment and then @ryanofsky corrected me.

+1 for moving to options dictionary.

It could also default to true for watch-only wallets, though not for external signer wallets (#16546), so maybe keep it simple :-)

luke-jr · 2019-08-19T23:57:29Z

src/wallet/feebumper.cpp

Maybe ISMINE_WATCH_ONLY should only be or'd when add_to_wallet is false?

seems reasonable, will update

luke-jr · 2019-08-19T23:58:24Z

src/wallet/rpcwallet.cpp

Suggest making the option leave it unsigned too. User can always pass it back in to a signing RPC.

jonatack

ACK 99513d7. Code review, built, ran tests and rpc help, poked around with the new test and assertions to verify changing them caused failure. Feel free to ignore the nits below.

The new test code fails on master at line 347:
bumped_psbt = watcher.bumpfee(original_txid)
with "Transaction contains inputs that don't belong to this wallet (-4)".

src/wallet/rpcwallet.cpp

test/functional/wallet_bumpfee.py

instagibbs · 2019-12-09T21:59:51Z

Found a bug, we weren't allowed to select new watchonly inputs. Fixed, enhanced test to catch this, and fixed @jonatack nits

jonatack

A few comments while looking through these changes.

The first commit 1d3c9a1 now does two things:
1. it updates the logic within the 4 call sites in wallet/feebumper.cpp that refer to ISMINE_SPENDABLE: PreconditionChecks, CheckFeeRate, CreateTotalBumpTransaction, and CreateRateBumpTransaction
2. (new to this commit) it adds setting coin_control.fAllowWatchOnly in wallet/rpcwallet.cpp bumpfee; coin_control is a parameter in wallet/feebumper CreateTotalBumpTransaction and CreateRateBumpTransaction
Diff of new test in last commit 7f26d0d064a2f55cafbb220750fe977c81031113 from previous 99513d717c09dd6d4dfe15d168dda7129db30e20 version:

-    funding_address = watcher.getnewaddress(address_type='bech32')
-    peer_node.sendtoaddress(funding_address, 0.001)
+    funding_address1 = watcher.getnewaddress(address_type='bech32')
+    funding_address2 = watcher.getnewaddress(address_type='bech32')
+    peer_node.sendmany("", {funding_address1: 0.001, funding_address2: 0.001})
     peer_node.generate(1)
     test.sync_all()
 
-    # Create PSBT for to be bumped transaction
+    # Create single-input PSBT for transaction to be bumped
     psbt = watcher.walletcreatefundedpsbt([], {dest_address:0.0005}, 0, {"feeRate": 0.00001}, True)['psbt']
     psbt_signed = signer.walletprocesspsbt(psbt=psbt, sign=True, sighashtype="ALL", bip32derivs=True)
     psbt_final = watcher.finalizepsbt(psbt_signed["psbt"])
     original_txid = watcher.sendrawtransaction(psbt_final["hex"])
+    assert_equal(len(watcher.decodepsbt(psbt)["tx"]["vin"]), 1)
 
-    # Bump fee
-    bumped_psbt = watcher.bumpfee(original_txid)
-    assert "psbt" in bumped_psbt
+    # Bump fee, obnoxiously high to add additional watchonly input
+    bumped_psbt = watcher.bumpfee(original_txid, {"fee_rate":0.005})
+    assert_greater_than(len(watcher.decodepsbt(bumped_psbt['psbt'])["tx"]["vin"]), 1)

Some questions/comments below, feel free to ignore any nits.

jonatack · 2019-12-17T14:28:17Z

test/functional/wallet_bumpfee.py

can do more compact tests that are stricter

pub_change_desc = rbf_node.getdescriptorinfo(priv_change_desc)["descriptor"] + success = [{'success': True}, {'success': True}] + # Create a wallet with private keys that can sign PSBTs rbf_node.createwallet(wallet_name="signer", disable_private_keys=False, blank=True) signer = rbf_node.get_wallet_rpc("signer") @@ -305,8 +307,7 @@ def test_watchonly_psbt(test, peer_node, rbf_node, dest_address): "internal": True, "keypool": False }]) - assert_equal(result[0], {'success': True}) - assert_equal(result[1], {'success': True}) + assert_equal(result, success) # Create another wallet with just the public keys, which creates PSBTs rbf_node.createwallet(wallet_name="watcher", disable_private_keys=True, blank=True) @@ -329,8 +330,7 @@ def test_watchonly_psbt(test, peer_node, rbf_node, dest_address): "keypool": True, "watchonly": True }]) - assert_equal(result[0], {'success': True}) - assert_equal(result[1], {'success': True}) + assert_equal(result, success)

test/functional/wallet_bumpfee.py

jonatack · 2019-12-17T18:56:26Z

src/wallet/feebumper.cpp

For info, the new test when run on master ab4e6ad without the other changes raises on feebumping with this feebumper::Result PreconditionChecks error "Transaction contains inputs that don't belong to this wallet", in the same place as my earlier comment #16373 (review):

# wallet_bumpfee.py 348 # Create single-input PSBT for transaction to be bumped 349 bumped_psbt = watcher.bumpfee(original_txid, {"fee_rate":0.005})

jonatack · 2019-12-17T18:57:14Z

test/functional/wallet_bumpfee.py

For info, this is where the new test fails on current master without the other changes.

src/wallet/rpcwallet.cpp

jonatack · 2019-12-17T19:46:31Z

Rebased 7f26d0d064a2f55cafbb220750fe977c81031113 to master, built, and ran all tests successfully.

Co-authored-by: Sjors Provoost <[email protected]>

instagibbs · 2019-12-18T14:05:14Z

fixed up tests and slight code cleanup as per @jonatack nits

achow101 · 2020-01-03T23:00:27Z

ACK 091a876

Sjors

Tested ACK 091a876

meshcollider

utACK 091a876

091a876 Test watchonly wallet bumpfee with PSBT return (Gregory Sanders) e9b4f94 bumpfee: Return PSBT when wallet has privkeys disabled (Gregory Sanders) 75a5e47 Change bumpfee to use watch-only funds for legacy watchonly wallets (Gregory Sanders) Pull request description: The main use-case here is for using with watch-only wallets with PSBT-signing cold wallets of all kinds. ACKs for top commit: achow101: ACK 091a876 Sjors: Tested ACK 091a876 meshcollider: utACK 091a876 Tree-SHA512: f7cf663e1af0b029e5c99eac88c5fdc3bc9e9a3841da8a608e8a9957e9bcf6a78864b8c2706fcaf78a480ffe11badd80c4fad29f97c0bb929e0470fafda5c22e

…sabled 091a876 Test watchonly wallet bumpfee with PSBT return (Gregory Sanders) e9b4f94 bumpfee: Return PSBT when wallet has privkeys disabled (Gregory Sanders) 75a5e47 Change bumpfee to use watch-only funds for legacy watchonly wallets (Gregory Sanders) Pull request description: The main use-case here is for using with watch-only wallets with PSBT-signing cold wallets of all kinds. ACKs for top commit: achow101: ACK 091a876 Sjors: Tested ACK 091a876 meshcollider: utACK 091a876 Tree-SHA512: f7cf663e1af0b029e5c99eac88c5fdc3bc9e9a3841da8a608e8a9957e9bcf6a78864b8c2706fcaf78a480ffe11badd80c4fad29f97c0bb929e0470fafda5c22e

…ly wallets 3c30d71 QT: Change bumpFee asserts to simple error message (Gregory Sanders) e3b19d8 QT: bump fee returns PSBT on clipboard for watchonly-only wallets (Gregory Sanders) Pull request description: Very small set of changes to support PSBT-based fee bumping on watchonly wallets in QT. quasi-companion to #16373 ACKs for top commit: gwillen: code review ACK 3c30d71 promag: Code review ACK 3c30d71. Sjors: utACK 3c30d71 achow101: ACK 3c30d71 Tree-SHA512: 7a706141e46d7fd0ad513a08a96c16f2e7e531427a6776b689362f82e32cbd9d4b7eeb98f6936aa3f9347d23ccc94128516fcffa695efacd9cac43606ea916e2

…only-only wallets 3c30d71 QT: Change bumpFee asserts to simple error message (Gregory Sanders) e3b19d8 QT: bump fee returns PSBT on clipboard for watchonly-only wallets (Gregory Sanders) Pull request description: Very small set of changes to support PSBT-based fee bumping on watchonly wallets in QT. quasi-companion to bitcoin#16373 ACKs for top commit: gwillen: code review ACK 3c30d71 promag: Code review ACK 3c30d71. Sjors: utACK 3c30d71 achow101: ACK 3c30d71 Tree-SHA512: 7a706141e46d7fd0ad513a08a96c16f2e7e531427a6776b689362f82e32cbd9d4b7eeb98f6936aa3f9347d23ccc94128516fcffa695efacd9cac43606ea916e2

…sabled 091a876 Test watchonly wallet bumpfee with PSBT return (Gregory Sanders) e9b4f94 bumpfee: Return PSBT when wallet has privkeys disabled (Gregory Sanders) 75a5e47 Change bumpfee to use watch-only funds for legacy watchonly wallets (Gregory Sanders) Pull request description: The main use-case here is for using with watch-only wallets with PSBT-signing cold wallets of all kinds. ACKs for top commit: achow101: ACK 091a876 Sjors: Tested ACK 091a876 meshcollider: utACK 091a876 Tree-SHA512: f7cf663e1af0b029e5c99eac88c5fdc3bc9e9a3841da8a608e8a9957e9bcf6a78864b8c2706fcaf78a480ffe11badd80c4fad29f97c0bb929e0470fafda5c22e

…only-only wallets 3c30d71 QT: Change bumpFee asserts to simple error message (Gregory Sanders) e3b19d8 QT: bump fee returns PSBT on clipboard for watchonly-only wallets (Gregory Sanders) Pull request description: Very small set of changes to support PSBT-based fee bumping on watchonly wallets in QT. quasi-companion to bitcoin#16373 ACKs for top commit: gwillen: code review ACK 3c30d71 promag: Code review ACK 3c30d71. Sjors: utACK 3c30d71 achow101: ACK 3c30d71 Tree-SHA512: 7a706141e46d7fd0ad513a08a96c16f2e7e531427a6776b689362f82e32cbd9d4b7eeb98f6936aa3f9347d23ccc94128516fcffa695efacd9cac43606ea916e2

instagibbs force-pushed the bump_psbt branch from 85f44ab to 11135a3 Compare July 11, 2019 18:05

DrahtBot added RPC/REST/ZMQ Wallet labels Jul 11, 2019

fanquake added the Needs Conceptual Review label Jul 11, 2019

fanquake removed the Needs Conceptual Review label Jul 12, 2019

instagibbs force-pushed the bump_psbt branch from ab65019 to 4a441c0 Compare July 12, 2019 14:11

instagibbs force-pushed the bump_psbt branch from 4a441c0 to b5ffbcb Compare July 16, 2019 00:22

ryanofsky approved these changes Jul 23, 2019

View reviewed changes

instagibbs force-pushed the bump_psbt branch from b5ffbcb to 10c7611 Compare August 17, 2019 15:07

instagibbs force-pushed the bump_psbt branch from 10c7611 to 57fb9f6 Compare August 17, 2019 20:11

luke-jr suggested changes Aug 19, 2019

View reviewed changes

fanquake requested a review from Sjors December 6, 2019 18:28

jonatack reviewed Dec 9, 2019

View reviewed changes

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

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

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

instagibbs force-pushed the bump_psbt branch from 99513d7 to d105af7 Compare December 9, 2019 21:59

instagibbs force-pushed the bump_psbt branch from d105af7 to 7f26d0d Compare December 9, 2019 22:04

jonatack reviewed Dec 17, 2019

View reviewed changes

instagibbs and others added 3 commits December 18, 2019 09:03

Change bumpfee to use watch-only funds for legacy watchonly wallets

75a5e47

bumpfee: Return PSBT when wallet has privkeys disabled

e9b4f94

Test watchonly wallet bumpfee with PSBT return

091a876

Co-authored-by: Sjors Provoost <[email protected]>

instagibbs force-pushed the bump_psbt branch from 7f26d0d to 091a876 Compare December 18, 2019 14:03

fanquake requested review from Sjors and removed request for Sjors and meshcollider January 4, 2020 00:15

Sjors approved these changes Jan 4, 2020

View reviewed changes

meshcollider reviewed Jan 7, 2020

View reviewed changes

meshcollider merged commit 091a876 into bitcoin:master Jan 7, 2020

This was referenced Feb 19, 2020

rpc: sendtoaddress and sendmany should return PSBT for watch-only wallet #18180

Closed

rpc: sendmany and sendtoaddress return PSBT for wallets without private keys #18201

Closed

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

t-bast mentioned this pull request Jul 17, 2020

Question: PSBT and RBF #19481

Closed

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

bumpfee: Return PSBT when wallet has privkeys disabled #16373

bumpfee: Return PSBT when wallet has privkeys disabled #16373

Uh oh!

Conversation

instagibbs commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

ryanofsky commented Jul 11, 2019

Uh oh!

promag commented Jul 12, 2019

Uh oh!

Sjors commented Jul 12, 2019

Uh oh!

instagibbs commented Jul 12, 2019

Uh oh!

instagibbs commented Jul 12, 2019

Uh oh!

instagibbs commented Jul 12, 2019

Uh oh!

promag commented Jul 12, 2019

Uh oh!

instagibbs commented Jul 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Jul 12, 2019

Uh oh!

promag commented Jul 12, 2019

Uh oh!

instagibbs commented Jul 16, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

promag commented Jul 23, 2019

Uh oh!

ryanofsky commented Jul 24, 2019

Uh oh!

instagibbs commented Jul 26, 2019

Uh oh!

Sjors commented Aug 15, 2019

Uh oh!

promag commented Aug 15, 2019

Uh oh!

meshcollider commented Aug 16, 2019

Uh oh!

instagibbs commented Aug 17, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

promag Sep 2, 2019 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonatack left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

instagibbs commented Dec 9, 2019

Uh oh!

jonatack left a comment

instagibbs commented Jul 11, 2019 •

edited

Loading

DrahtBot commented Jul 11, 2019 •

edited

Loading

instagibbs commented Jul 12, 2019 •

edited

Loading

promag Sep 2, 2019 •

edited

Loading

jonatack Dec 17, 2019 •

edited

Loading

jonatack Dec 17, 2019 •

edited

Loading