Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

bumpfee: Return PSBT when wallet has privkeys disabled #16373

Merged
merged 3 commits into from Jan 7, 2020

Conversation

@instagibbs
Copy link
Member

instagibbs commented Jul 11, 2019

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

@instagibbs instagibbs force-pushed the instagibbs:bump_psbt branch from 85f44ab to 11135a3 Jul 11, 2019
@DrahtBot

This comment has been minimized.

Copy link
Contributor

DrahtBot commented Jul 11, 2019

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

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Jul 11, 2019

Concept ACK. Seems reasonable and reasonably simple.

@promag

This comment has been minimized.

Copy link
Member

promag commented Jul 12, 2019

Concept ACK, how about a new RPC?

@Sjors

This comment has been minimized.

Copy link
Member

Sjors commented Jul 12, 2019

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

This comment has been minimized.

Copy link
Member Author

instagibbs commented Jul 12, 2019

@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

This comment has been minimized.

Copy link
Member Author

instagibbs commented Jul 12, 2019

@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 instagibbs force-pushed the instagibbs:bump_psbt branch from ab65019 to 4a441c0 Jul 12, 2019
@instagibbs

This comment has been minimized.

Copy link
Member Author

instagibbs commented Jul 12, 2019

Pushed tests

@promag

This comment has been minimized.

Copy link
Member

promag commented Jul 12, 2019

@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

This comment has been minimized.

Copy link
Member Author

instagibbs commented Jul 12, 2019

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

This comment has been minimized.

Copy link
Member

Sjors commented Jul 12, 2019

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

This comment has been minimized.

Copy link
Member

promag commented Jul 12, 2019

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

@instagibbs instagibbs force-pushed the instagibbs:bump_psbt branch from 4a441c0 to b5ffbcb Jul 16, 2019
@instagibbs

This comment has been minimized.

Copy link
Member Author

instagibbs commented Jul 16, 2019

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

Copy link
Contributor

ryanofsky left a comment

utACK b5ffbcb. 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

This comment has been minimized.

Copy link
Member

promag commented Jul 23, 2019

@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

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Jul 24, 2019

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

This comment has been minimized.

Copy link
Member Author

instagibbs commented Jul 26, 2019

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

This comment has been minimized.

Copy link
Member

Sjors commented Aug 15, 2019

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

@promag

This comment has been minimized.

Copy link
Member

promag commented Aug 15, 2019

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

@meshcollider

This comment has been minimized.

Copy link
Member

meshcollider commented Aug 16, 2019

Concept ACK with a bit of light code review

@instagibbs instagibbs force-pushed the instagibbs:bump_psbt branch from b5ffbcb to 10c7611 Aug 17, 2019
@instagibbs

This comment has been minimized.

Copy link
Member Author

instagibbs commented Aug 17, 2019

updated to add_to_wallet argument name/logic.

@instagibbs instagibbs force-pushed the instagibbs:bump_psbt branch from 10c7611 to 57fb9f6 Aug 17, 2019
"transaction in PSBT format instead of submitting to the wallet and mempool.\n"
"The wallet will try to sign as much of the PSBT as possible, but may returned something\n"
"unsigned if there are watch-only transactions in the wallet."
},

This comment has been minimized.

Copy link
@luke-jr

luke-jr Aug 19, 2019

Member

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

This comment has been minimized.

Copy link
@instagibbs

instagibbs Aug 21, 2019

Author Member

fwiw that point was discussed above

This comment has been minimized.

Copy link
@luke-jr

luke-jr Sep 2, 2019

Member

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

This comment has been minimized.

Copy link
@promag

promag Sep 2, 2019

Member

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

This comment has been minimized.

Copy link
@Sjors

Sjors Oct 26, 2019

Member

+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 :-)

@@ -48,7 +48,7 @@ static feebumper::Result PreconditionChecks(interfaces::Chain::Lock& locked_chai

// check that original tx consists entirely of our inputs
// if not, we can't bump the fee, because the wallet has no way of knowing the value of the other inputs (thus the fee)
if (!wallet->IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE)) {
if (!wallet->IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE|ISMINE_WATCH_ONLY)) {

This comment has been minimized.

Copy link
@luke-jr

luke-jr Aug 19, 2019

Member

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

This comment has been minimized.

Copy link
@instagibbs

instagibbs Aug 21, 2019

Author Member

seems reasonable, will update

@@ -3323,10 +3323,16 @@ static UniValue bumpfee(const JSONRPCRequest& request)
" \"CONSERVATIVE\""},
},
"options"},
{"add_to_wallet", RPCArg::Type::BOOL, /* default */ "true", "When false, returns the fee-bumped\n"

This comment has been minimized.

Copy link
@luke-jr

luke-jr Aug 19, 2019

Member

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

This comment has been minimized.

Copy link
@instagibbs

instagibbs Aug 21, 2019

Author Member

meh, ~0

@instagibbs

This comment has been minimized.

Copy link
Member Author

instagibbs commented Dec 3, 2019

rebased

Copy link
Member

achow101 left a comment

Concept ACK

@@ -47,7 +47,8 @@ static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWallet

// check that original tx consists entirely of our inputs
// if not, we can't bump the fee, because the wallet has no way of knowing the value of the other inputs (thus the fee)
if (!wallet.IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE)) {
isminefilter filter = wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) ? ISMINE_WATCH_ONLY : ISMINE_SPENDABLE;

This comment has been minimized.

Copy link
@achow101

achow101 Dec 5, 2019

Member

Could you use ISMINE_ALL to avoid conflicts with future ScriptPubKeyMan?

This comment has been minimized.

Copy link
@instagibbs

instagibbs Dec 6, 2019

Author Member

I'm checking for legacy spkm instead.

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Dec 6, 2019

ACK 99513d7

@fanquake fanquake requested a review from Sjors Dec 6, 2019
Copy link
Member

jonatack left a comment

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 Outdated Show resolved Hide resolved
src/wallet/rpcwallet.cpp Show resolved Hide resolved
test/functional/wallet_bumpfee.py Outdated Show resolved Hide resolved
@instagibbs instagibbs force-pushed the instagibbs:bump_psbt branch from 99513d7 to d105af7 Dec 9, 2019
@instagibbs

This comment has been minimized.

Copy link
Member Author

instagibbs commented Dec 9, 2019

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

@instagibbs instagibbs force-pushed the instagibbs:bump_psbt branch from d105af7 to 7f26d0d Dec 9, 2019
Copy link
Member

jonatack left a comment

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 7f26d0d from previous 99513d7 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.

"keypool": False
}])
assert_equal(result[0], {'success': True})
assert_equal(result[1], {'success': True})

This comment has been minimized.

Copy link
@jonatack

jonatack Dec 17, 2019

Member

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)

This comment has been minimized.

Copy link
@instagibbs

instagibbs Dec 18, 2019

Author Member

done

test/functional/wallet_bumpfee.py Show resolved Hide resolved
test/functional/wallet_bumpfee.py Outdated Show resolved Hide resolved
@@ -47,7 +47,8 @@ static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWallet

// check that original tx consists entirely of our inputs
// if not, we can't bump the fee, because the wallet has no way of knowing the value of the other inputs (thus the fee)
if (!wallet.IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE)) {
isminefilter filter = wallet.GetLegacyScriptPubKeyMan() && wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) ? ISMINE_WATCH_ONLY : ISMINE_SPENDABLE;
if (!wallet.IsAllFromMe(*wtx.tx, filter)) {
errors.push_back("Transaction contains inputs that don't belong to this wallet");

This comment has been minimized.

Copy link
@jonatack

jonatack Dec 17, 2019

Member

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})
assert_equal(len(watcher.decodepsbt(psbt)["tx"]["vin"]), 1)

# Bump fee, obnoxiously high to add additional watchonly input
bumped_psbt = watcher.bumpfee(original_txid, {"fee_rate":0.005})

This comment has been minimized.

Copy link
@jonatack

jonatack Dec 17, 2019

Member

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

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

This comment has been minimized.

Copy link
Member

jonatack commented Dec 17, 2019

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

@instagibbs instagibbs force-pushed the instagibbs:bump_psbt branch from 7f26d0d to 091a876 Dec 18, 2019
@instagibbs

This comment has been minimized.

Copy link
Member Author

instagibbs commented Dec 18, 2019

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

@achow101

This comment has been minimized.

Copy link
Member

achow101 commented Jan 3, 2020

ACK 091a876

@fanquake fanquake requested review from Sjors and removed request for Sjors and meshcollider Jan 4, 2020
@Sjors
Sjors approved these changes Jan 4, 2020
Copy link
Member

Sjors left a comment

Tested ACK 091a876

Copy link
Member

meshcollider left a comment

utACK 091a876

meshcollider added a commit that referenced this pull request Jan 7, 2020
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
@meshcollider meshcollider merged commit 091a876 into bitcoin:master Jan 7, 2020
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
sidhujag added a commit to syscoin/syscoin that referenced this pull request Jan 8, 2020
…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
fanquake added a commit that referenced this pull request Jan 22, 2020
…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
sidhujag added a commit to syscoin/syscoin that referenced this pull request Jan 24, 2020
…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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.