bumpfee: Return PSBT when wallet has privkeys disabled

instagibbs commented at 6:03 pm on July 11, 2019: member

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

instagibbs force-pushed on Jul 11, 2019

DrahtBot added the label RPC/REST/ZMQ on Jul 11, 2019

DrahtBot added the label Wallet on Jul 11, 2019

DrahtBot commented at 7:52 pm on July 11, 2019: 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:

#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 commented at 8:04 pm on July 11, 2019: member

Concept ACK. Seems reasonable and reasonably simple.

fanquake added the label Needs Conceptual Review on Jul 11, 2019

promag commented at 0:41 am on July 12, 2019: member

Concept ACK, how about a new RPC?

Sjors commented at 8:42 am on July 12, 2019: member

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.

fanquake removed the label Needs Conceptual Review on Jul 12, 2019

instagibbs commented at 2:07 pm on July 12, 2019: member

@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 commented at 2:09 pm on July 12, 2019: member

@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 force-pushed on Jul 12, 2019

instagibbs commented at 2:13 pm on July 12, 2019: member

Pushed tests

promag commented at 2:16 pm on July 12, 2019: member

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

0analyzepsbt
1combinepsbt
2converttopsbt
3createpsbt
4decodepsbt
5finalizepsbt
6joinpsbts
7utxoupdatepsbt
8walletcreatefundedpsbt
9walletprocesspsbt

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 commented at 2:21 pm on July 12, 2019: member

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 commented at 2:31 pm on July 12, 2019: member

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 commented at 3:29 pm on July 12, 2019: member

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

instagibbs force-pushed on Jul 16, 2019

instagibbs commented at 0:28 am on July 16, 2019: member

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

ryanofsky approved

ryanofsky commented at 7:01 pm on July 23, 2019: member

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 commented at 7:12 pm on July 23, 2019: member

@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 commented at 5:09 pm on July 24, 2019: member

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 commented at 5:42 pm on July 26, 2019: member

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 commented at 5:15 pm on August 15, 2019: member

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

promag commented at 11:22 pm on August 15, 2019: member

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

meshcollider commented at 9:56 pm on August 16, 2019: contributor

Concept ACK with a bit of light code review

instagibbs force-pushed on Aug 17, 2019

instagibbs commented at 3:07 pm on August 17, 2019: member

updated to add_to_wallet argument name/logic.

instagibbs force-pushed on Aug 17, 2019

in src/wallet/rpcwallet.cpp:3323 in 57fb9f6fce outdated

3322@@ -3323,10 +3323,16 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3323             "         \"CONSERVATIVE\""},
3324                         },
3325                         "options"},
3326+                    {"add_to_wallet", RPCArg::Type::BOOL, /* default */ "true", "When false, returns the fee-bumped\n"
3327+    "transaction in PSBT format instead of submitting to the wallet and mempool.\n"
3328+    "The wallet will try to sign as much of the PSBT as possible, but may returned something\n"
3329+    "unsigned if there are watch-only transactions in the wallet."
3330+                    },

luke-jr commented at 11:56 pm on August 19, 2019:

This seems like it shouldn’t be a positional parameter…

instagibbs commented at 8:51 pm on August 21, 2019:

fwiw that point was discussed above

luke-jr commented at 2:30 pm on September 2, 2019:

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

promag commented at 2:37 pm on September 2, 2019:

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

Sjors commented at 9:37 am on October 26, 2019:

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

in src/wallet/feebumper.cpp:51 in 57fb9f6fce outdated

47@@ -48,7 +48,7 @@ static feebumper::Result PreconditionChecks(interfaces::Chain::Lock& locked_chai
48 
49     // check that original tx consists entirely of our inputs
50     // 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)
51-    if (!wallet->IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE)) {
52+    if (!wallet->IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE|ISMINE_WATCH_ONLY)) {

luke-jr commented at 11:57 pm on August 19, 2019:

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

instagibbs commented at 8:51 pm on August 21, 2019:

seems reasonable, will update

in src/wallet/rpcwallet.cpp:3319 in 57fb9f6fce outdated

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

luke-jr commented at 11:58 pm on August 19, 2019:

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

instagibbs commented at 8:52 pm on August 21, 2019:

meh, ~0

luke-jr changes_requested

laanwj added the label Feature on Sep 30, 2019

DrahtBot added the label Needs rebase on Oct 15, 2019

ryanofsky approved

ryanofsky commented at 4:02 pm on October 24, 2019: member

Code review ACK 57fb9f6fcea89e1447f5797fd8d9f542403826e8. Only change since last review is replacing return_psbt option with opposite add_to_wallet option.

instagibbs commented at 4:16 pm on October 24, 2019: member

can I get a fresh round of concept ACKs from people in this thread? I’ll rebase if people like it.

fanquake requested review from meshcollider on Oct 24, 2019

fanquake requested review from Sjors on Oct 24, 2019

Sjors commented at 6:56 pm on October 24, 2019: member

Still concept ACK.

instagibbs force-pushed on Oct 25, 2019

instagibbs commented at 5:42 pm on October 25, 2019: member

rebased

DrahtBot removed the label Needs rebase on Oct 25, 2019

in src/wallet/rpcwallet.cpp:3321 in 9bdf420ecc outdated

3315@@ -3316,10 +3316,16 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3316             "         \"CONSERVATIVE\""},
3317                         },
3318                         "options"},
3319+                    {"add_to_wallet", RPCArg::Type::BOOL, /* default */ "true", "When false, returns the fee-bumped\n"
3320+    "transaction in PSBT format instead of submitting to the wallet and mempool.\n"
3321+    "The wallet will try to sign as much of the PSBT as possible, but may returned something\n"

Sjors commented at 8:52 am on October 26, 2019:

nit: may return

in src/wallet/rpcwallet.cpp:3327 in 9bdf420ecc outdated

3323+                    },
3324                 },
3325                 RPCResult{
3326             "{\n"
3327-            "  \"txid\":    \"value\",   (string)  The id of the new transaction\n"
3328+            "  \"psbt\":    \"psbt\",    (string) base64-encoded PSBT of that bumped transaction. Only returned when add_to_wallet is set to false.\n"

Sjors commented at 8:53 am on October 26, 2019:

nit: of the bumped transaction

in src/wallet/feebumper.cpp:50 in 9bdf420ecc outdated

46@@ -47,7 +47,7 @@ static feebumper::Result PreconditionChecks(interfaces::Chain::Lock& locked_chai
47 
48     // check that original tx consists entirely of our inputs
49     // 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)
50-    if (!wallet.IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE)) {
51+    if (!wallet.IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE|ISMINE_WATCH_ONLY)) {

Sjors commented at 9:32 am on October 26, 2019:

Light preference for only allowing this on native watch-only wallets.

in src/wallet/rpcwallet.cpp:3452 in 9bdf420ecc outdated

3457+        result.pushKV("txid", txid.GetHex());
3458+    } else {
3459+        // Return a as filled-out-as-possible PSBT
3460+        PartiallySignedTransaction psbtx(mtx);
3461+        bool complete = false;
3462+        const TransactionError err = FillPSBT(pwallet, psbtx, complete, SIGHASH_ALL, true /* sign */, true /* bip32derivs */);

Sjors commented at 9:39 am on October 26, 2019:

nit: I find it more readable to have the variable name before the value (as it would be for named params)

Sjors commented at 9:50 am on October 26, 2019:

Other psbt methods do not set bip32derivs to true by default (though I’m starting to regret recommending that). To be consistent it should be an option.

Sjors commented at 10:10 am on October 26, 2019:

See #17264

instagibbs commented at 8:07 pm on November 20, 2019:

I find it more readable to have the variable name before the value

Following convention here

To be consistent it should be an option

IMO nah, especially after I nuke all the additional arguments

Sjors changes_requested

Sjors commented at 9:52 am on October 26, 2019: member

It’s thoroughly confused about the original fee for watch-only transactions (as is the GUI, see #17263), e.g. this one had 1 satoshi / byte:

0  "origfee": 4312.66231304,
1  "fee": 0.00002820,

Other than that code at 18e9f2085d540bc54bc8a2953fae2b6bbbfaade3 looks good to me, and it actually creates a new transaction with a bumped fee.

ryanofsky approved

ryanofsky commented at 8:18 pm on November 5, 2019: member

Code review ACK 9bdf420ecc37959175882faed2d8032a737f7e07. No changes since last review other than rebase

instagibbs commented at 3:09 pm on November 18, 2019: member

after working on #17492 I think the best thing to do is remove all arguments here, and just let the “are privkeys disabled for this wallet” be the logic switch since that is the standard check now for this type of behavior.

instagibbs force-pushed on Nov 20, 2019

instagibbs renamed this:
~~Add bumpfee option to return PSBT instead of commiting to wallet~~
bumpfee: Return PSBT when wallet has privkeys disabled
on Nov 20, 2019

instagibbs commented at 8:20 pm on November 20, 2019: member

switched implementation to simply use the fact that a wallet has privkeys disabled or not to return a PSBT or attempt to make a finalized transaction. New test incoming.

instagibbs renamed this:
~~bumpfee: Return PSBT when wallet has privkeys disabled~~
WIP bumpfee: Return PSBT when wallet has privkeys disabled
on Nov 21, 2019

instagibbs force-pushed on Nov 21, 2019

instagibbs renamed this:
~~WIP bumpfee: Return PSBT when wallet has privkeys disabled~~
bumpfee: Return PSBT when wallet has privkeys disabled
on Nov 21, 2019

instagibbs commented at 4:10 pm on November 21, 2019: member

rebased on master, fixed travis, added test.

meshcollider commented at 8:28 am on November 27, 2019: contributor

utACK b2ee422be790f07a9006111e73fedb8c11d20f41

Haven’t reviewed the test yet

in src/wallet/rpcwallet.cpp:3493 in b2ee422be7 outdated

3470+    } else {
3471+        PartiallySignedTransaction psbtx(mtx);
3472+        bool complete = false;
3473+        const TransactionError err = FillPSBT(pwallet, psbtx, complete, SIGHASH_ALL, false /* sign */, true /* bip32derivs */);
3474+        CHECK_NONFATAL(!complete);
3475+        CHECK_NONFATAL(err == TransactionError::OK);

Sjors commented at 10:07 am on November 27, 2019:

Nit: check this first

instagibbs commented at 2:15 pm on November 27, 2019:

done

in test/functional/wallet_bumpfee.py:349 in 7de9adbe7e outdated

320+    assert original_txid in rbf_node.getrawmempool()
321+
322+    # And replace it once
323+    bumped_psbt = watchonly_endpoint.bumpfee(original_txid)
324+    assert "psbt" in bumped_psbt
325+    assert "txid" not in bumped_psbt

Sjors commented at 10:47 am on November 27, 2019:

If you add assert_greater_than(bumped_psbt["origfee"], 0) it will throw AssertionError: -0.00099000 <= 0

instagibbs commented at 2:16 pm on November 27, 2019:

fixed and added the test

Sjors commented at 10:47 am on November 27, 2019: member

It’s still getting the original fee wrong for me: "origfee": 0.00000000

instagibbs force-pushed on Nov 27, 2019

instagibbs commented at 2:16 pm on November 27, 2019: member

@Sjors addressed all issues

instagibbs commented at 3:16 pm on November 27, 2019: member

restarted single job which failed to fetch packages

Sjors commented at 8:13 pm on November 27, 2019: member

The test passes, but when I use the bumpfee RPC it still shows the original fee as 0. Some details that may be relevant: I used a Trezor set to testnet and imported keys into a regtest watch-only wallet, using the HWI incantation. I funded it, got that transaction confirmed, and then spent a small fraction (so there’s change). The fee was 10 sat / byte. I then bumped the fee of that last transaction to 20 sat / byte.

Sjors commented at 11:18 am on November 28, 2019: member

An easier way to write the test, and also more true to real world usage, is to import a descriptor with private keys into wallet A and then import the same descriptor with just the public keys into wallet B. You can use wallet A to sign PSBTs created with wallet B.

Here’s a test vector: https://github.com/bitcoin/bitcoin/pull/16546/commits/234f30a3524b9c2a7759ee5a1681f88cdbdde0c9#diff-579fcca4201aac25c7b61fdecdaad61fR135-R168

Sjors commented at 11:54 am on November 28, 2019: member

I reimplemented your test using a signer and an equivalent watch-only wallet: https://github.com/bitcoin/bitcoin/compare/master...Sjors:2019/11/bump_psbt

That doesn’t reproduce my issue, so for now I assume there’s a problem with my own wallet.

instagibbs commented at 3:40 pm on November 28, 2019: member

@Sjors I’m not a super fan of magically-derived priv/pubkey pairs in tests(xprv vs xpub) but I’ll take a look.

instagibbs force-pushed on Dec 2, 2019

instagibbs commented at 2:57 pm on December 2, 2019: member

took a cleaned up version of the @Sjors test linked here #16373 (comment)

I checked by hand the tpub/tprv matches ;)

Sjors commented at 8:52 am on December 3, 2019: member

Code review ACK fc7fb79

instagibbs force-pushed on Dec 3, 2019

instagibbs commented at 2:38 pm on December 3, 2019: member

un-magicified the tprv/tpub stuff h/t @Sjors for getdescriptorinfo idea to convert

instagibbs force-pushed on Dec 3, 2019

instagibbs commented at 4:22 pm on December 3, 2019: member

rebased

in src/wallet/feebumper.cpp:50 in 6acb9344eb outdated

46@@ -47,7 +47,8 @@ static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWallet
47 
48     // check that original tx consists entirely of our inputs
49     // 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)
50-    if (!wallet.IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE)) {
51+    isminefilter filter = wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) ? ISMINE_WATCH_ONLY : ISMINE_SPENDABLE;

achow101 commented at 4:21 pm on December 5, 2019:

Could you use ISMINE_ALL to avoid conflicts with future ScriptPubKeyMan?

instagibbs commented at 5:29 pm on December 6, 2019:

I’m checking for legacy spkm instead.

achow101 commented at 4:24 pm on December 5, 2019: member

Concept ACK

instagibbs force-pushed on Dec 6, 2019

achow101 commented at 6:27 pm on December 6, 2019: member

ACK 99513d717c09dd6d4dfe15d168dda7129db30e20

fanquake requested review from Sjors on Dec 6, 2019

in src/wallet/rpcwallet.cpp:3372 in 99513d717c outdated

3364@@ -3365,7 +3365,8 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3365                 },
3366                 RPCResult{
3367             "{\n"
3368-            "  \"txid\":    \"value\",   (string)  The id of the new transaction\n"
3369+            "  \"psbt\":    \"psbt\",    (string) The base64-encoded unsigned PSBT of new transaction. Only returned when private keys are disabled for the wallet.\n"
3370+            "  \"txid\":    \"value\",   (string)  The id of the new transaction. Only returned for wallets with private keys enabled.\n"
3371             "  \"origfee\":  n,         (numeric) Fee of the replaced transaction\n"
3372             "  \"fee\":      n,         (numeric) Fee of the new transaction\n"
3373             "  \"errors\":  [ str... ] (json array of strings) Errors encountered during processing (may be empty)\n"

jonatack commented at 10:39 am on December 9, 2019:

Content and spacing suggestion:

0-            "  \"psbt\":    \"psbt\",    (string) The base64-encoded unsigned PSBT of new transaction. Only returned when private keys are disabled for the wallet.\n"
1-            "  \"txid\":    \"value\",   (string)  The id of the new transaction. Only returned for wallets with private keys enabled.\n"
2-            "  \"origfee\":  n,         (numeric) Fee of the replaced transaction\n"
3-            "  \"fee\":      n,         (numeric) Fee of the new transaction\n"
4-            "  \"errors\":  [ str... ] (json array of strings) Errors encountered during processing (may be empty)\n"
5+            "  \"psbt\":    \"psbt\",    (string) The base64-encoded unsigned PSBT of the new transaction. Only returned when wallet private keys are disabled.\n"
6+            "  \"txid\":    \"value\",   (string) The id of the new transaction. Only returned when wallet private keys are enabled.\n"
7+            "  \"origfee\":  n,        (numeric) The fee of the replaced transaction.\n"
8+            "  \"fee\":      n,        (numeric) The fee of the new transaction.\n"
9+            "  \"errors\":  [ str... ] (json array of strings) Errors encountered during processing (may be empty).\n"

which returns

0Result:
1{
2  "psbt":    "psbt",    (string) The base64-encoded unsigned PSBT of the new transaction. Only returned when wallet private keys are disabled.
3  "txid":    "value",   (string) The id of the new transaction. Only returned when wallet private keys are enabled.
4  "origfee":  n,        (numeric) The fee of the replaced transaction.
5  "fee":      n,        (numeric) The fee of the new transaction.
6  "errors":  [ str... ] (json array of strings) Errors encountered during processing (may be empty).
7}

instead of

0Result:
1{
2  "psbt":    "psbt",    (string) The base64-encoded unsigned PSBT of new transaction. Only returned when private keys are disabled for the wallet.
3  "txid":    "value",   (string)  The id of the new transaction. Only returned for wallets with private keys enabled.
4  "origfee":  n,         (numeric) Fee of the replaced transaction
5  "fee":      n,         (numeric) Fee of the new transaction
6  "errors":  [ str... ] (json array of strings) Errors encountered during processing (may be empty)
7}

in src/wallet/rpcwallet.cpp:3475 in 99513d717c outdated

3478-    if (feebumper::CommitTransaction(*pwallet, hash, std::move(mtx), errors, txid) != feebumper::Result::OK) {
3479-        throw JSONRPCError(RPC_WALLET_ERROR, errors[0]);
3480-    }
3481     UniValue result(UniValue::VOBJ);
3482-    result.pushKV("txid", txid.GetHex());
3483+

jonatack commented at 10:51 am on December 9, 2019:

Perhaps add a code comment here:

0+    // If wallet private keys are enabled, return the new transaction id,
1+    // otherwise return the base64-encoded unsigned PSBT of the new transaction.

in test/functional/wallet_bumpfee.py:340 in 99513d717c outdated

335+    funding_address = watcher.getnewaddress(address_type='bech32')
336+    peer_node.sendtoaddress(funding_address, 0.001)
337+    peer_node.generate(1)
338+    test.sync_all()
339+
340+    # Create PSBT for to be bumped transaction

jonatack commented at 11:00 am on December 9, 2019:

suggestion: s/to be bumped transaction/transaction to be bumped/

jonatack commented at 11:31 am on December 9, 2019: member

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)”.

instagibbs force-pushed on Dec 9, 2019

instagibbs commented at 9:59 pm on December 9, 2019: member

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

instagibbs force-pushed on Dec 9, 2019

in test/functional/wallet_bumpfee.py:309 in 7f26d0d064 outdated

304+        "range": [0, 0],
305+        "internal": True,
306+        "keypool": False
307+    }])
308+    assert_equal(result[0], {'success': True})
309+    assert_equal(result[1], {'success': True})

jonatack commented at 2:28 pm on December 17, 2019:

can do more compact tests that are stricter

 0     pub_change_desc = rbf_node.getdescriptorinfo(priv_change_desc)["descriptor"]
 1+    success = [{'success': True}, {'success': True}]
 2+
 3     # Create a wallet with private keys that can sign PSBTs
 4     rbf_node.createwallet(wallet_name="signer", disable_private_keys=False, blank=True)
 5     signer = rbf_node.get_wallet_rpc("signer")
 6@@ -305,8 +307,7 @@ def test_watchonly_psbt(test, peer_node, rbf_node, dest_address):
 7         "internal": True,
 8         "keypool": False
 9     }])
10-    assert_equal(result[0], {'success': True})
11-    assert_equal(result[1], {'success': True})
12+    assert_equal(result, success)
13 
14     # Create another wallet with just the public keys, which creates PSBTs
15     rbf_node.createwallet(wallet_name="watcher", disable_private_keys=True, blank=True)
16@@ -329,8 +330,7 @@ def test_watchonly_psbt(test, peer_node, rbf_node, dest_address):
17         "keypool": True,
18         "watchonly": True
19     }])
20-    assert_equal(result[0], {'success': True})
21-    assert_equal(result[1], {'success': True})
22+    assert_equal(result, success)

instagibbs commented at 2:04 pm on December 18, 2019:

done

in test/functional/wallet_bumpfee.py:293 in 7f26d0d064 outdated

288+    priv_change_desc = "wpkh([00000001/84'/1'/0']tprv8ZgxMBicQKsPd7Uf69XL1XwhmjHopUGep8GuEiJDZmbQz6o58LninorQAfcKZWARbtRtfnLcJ5MQ2AtHcQJCCRUcMRvmDUjyEmNUWwx8UbK/1/*)#j6uzqvuh"
289+    pub_change_desc = rbf_node.getdescriptorinfo(priv_change_desc)["descriptor"]
290+    # Create a wallet with private keys that can sign PSBTs
291+    rbf_node.createwallet(wallet_name="signer", disable_private_keys=False, blank=True)
292+    signer = rbf_node.get_wallet_rpc("signer")
293+    assert signer.getwalletinfo()['private_keys_enabled']

jonatack commented at 5:32 pm on December 17, 2019:

nit: test failure messages here and lines 314 and 353 can be more useful using assert_equal, e.g.

0-    assert signer.getwalletinfo()['private_keys_enabled']
1+    assert_equal(True, signer.getwalletinfo()['private_keys_enabled'])

instagibbs commented at 9:17 pm on December 17, 2019:

should we make assert_true as a helper?

instagibbs commented at 2:04 pm on December 18, 2019:

I don’t think this makes it any easier to understand, it’s boolean and the line is printed on failure

in test/functional/wallet_bumpfee.py:358 in 7f26d0d064 outdated

353+    assert not watcher.finalizepsbt(bumped_psbt["psbt"])["complete"]
354+
355+    # Sign bumped transaction
356+    bumped_psbt_signed = signer.walletprocesspsbt(psbt=bumped_psbt["psbt"], sign=True, sighashtype="ALL", bip32derivs=True)
357+    bumped_psbt_final = watcher.finalizepsbt(bumped_psbt_signed["psbt"])
358+    bumped_psbt_final["complete"]

jonatack commented at 5:48 pm on December 17, 2019:

was this line intended to be

0assert_equal(True, bumped_psbt_final["complete"])

or?

instagibbs commented at 9:16 pm on December 17, 2019:

assert bumped_psbt_final["complete"], will fix!

instagibbs commented at 2:04 pm on December 18, 2019:

fixed

in src/wallet/feebumper.cpp:52 in 7f26d0d064 outdated

46@@ -47,7 +47,8 @@ static feebumper::Result PreconditionChecks(const CWallet& wallet, const CWallet
47 
48     // check that original tx consists entirely of our inputs
49     // 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)
50-    if (!wallet.IsAllFromMe(*wtx.tx, ISMINE_SPENDABLE)) {
51+    isminefilter filter = wallet.GetLegacyScriptPubKeyMan() && wallet.IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) ? ISMINE_WATCH_ONLY : ISMINE_SPENDABLE;
52+    if (!wallet.IsAllFromMe(*wtx.tx, filter)) {
53         errors.push_back("Transaction contains inputs that don't belong to this wallet");

jonatack commented at 6:56 pm on December 17, 2019:

For info, the new test when run on master ab4e6ad7629430d02d101417e010228c1099f0ae 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#pullrequestreview-328827090:

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

in test/functional/wallet_bumpfee.py:347 in 7f26d0d064 outdated

344+    psbt_final = watcher.finalizepsbt(psbt_signed["psbt"])
345+    original_txid = watcher.sendrawtransaction(psbt_final["hex"])
346+    assert_equal(len(watcher.decodepsbt(psbt)["tx"]["vin"]), 1)
347+
348+    # Bump fee, obnoxiously high to add additional watchonly input
349+    bumped_psbt = watcher.bumpfee(original_txid, {"fee_rate":0.005})

jonatack commented at 6:57 pm on December 17, 2019:

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

in src/wallet/rpcwallet.cpp:3388 in 7f26d0d064 outdated

3384@@ -3384,6 +3385,7 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3385     CAmount totalFee = 0;
3386     CCoinControl coin_control;
3387     coin_control.m_signal_bip125_rbf = true;
3388+    coin_control.fAllowWatchOnly = pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);

jonatack commented at 7:20 pm on December 17, 2019:

pico-nit: pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS) isn’t an optional rpc parameter

0-    // optional parameters
1     CAmount totalFee = 0;
2     CCoinControl coin_control;
3-    coin_control.m_signal_bip125_rbf = true;
4     coin_control.fAllowWatchOnly = pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS);
5+    coin_control.m_signal_bip125_rbf = true; // This value may be overwritten by options["replaceable"] below
6+    // Optional parameters

instagibbs commented at 2:03 pm on December 18, 2019:

fixed

jonatack commented at 7:41 pm on December 17, 2019: member

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:

 0-    funding_address = watcher.getnewaddress(address_type='bech32')
 1-    peer_node.sendtoaddress(funding_address, 0.001)
 2+    funding_address1 = watcher.getnewaddress(address_type='bech32')
 3+    funding_address2 = watcher.getnewaddress(address_type='bech32')
 4+    peer_node.sendmany("", {funding_address1: 0.001, funding_address2: 0.001})
 5     peer_node.generate(1)
 6     test.sync_all()
 7 
 8-    # Create PSBT for to be bumped transaction
 9+    # Create single-input PSBT for transaction to be bumped
10     psbt = watcher.walletcreatefundedpsbt([], {dest_address:0.0005}, 0, {"feeRate": 0.00001}, True)['psbt']
11     psbt_signed = signer.walletprocesspsbt(psbt=psbt, sign=True, sighashtype="ALL", bip32derivs=True)
12     psbt_final = watcher.finalizepsbt(psbt_signed["psbt"])
13     original_txid = watcher.sendrawtransaction(psbt_final["hex"])
14+    assert_equal(len(watcher.decodepsbt(psbt)["tx"]["vin"]), 1)
15 
16-    # Bump fee
17-    bumped_psbt = watcher.bumpfee(original_txid)
18-    assert "psbt" in bumped_psbt
19+    # Bump fee, obnoxiously high to add additional watchonly input
20+    bumped_psbt = watcher.bumpfee(original_txid, {"fee_rate":0.005})
21+    assert_greater_than(len(watcher.decodepsbt(bumped_psbt['psbt'])["tx"]["vin"]), 1)

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

jonatack commented at 7:46 pm on December 17, 2019: member

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

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

bumpfee: Return PSBT when wallet has privkeys disabled e9b4f9419c

Test watchonly wallet bumpfee with PSBT return

Co-authored-by: Sjors Provoost <sjors@sprovoost.nl>

091a876664

instagibbs force-pushed on Dec 18, 2019

instagibbs commented at 2:05 pm on December 18, 2019: member

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

achow101 commented at 11:00 pm on January 3, 2020: member

ACK 091a876664af4427db670ea8244d713b1b840048

fanquake removed review request from Sjors on Jan 4, 2020

fanquake removed review request from meshcollider on Jan 4, 2020

fanquake requested review from Sjors on Jan 4, 2020

Sjors approved

Sjors commented at 5:19 am on January 4, 2020: member

Tested ACK 091a876664af4427db670ea8244d713b1b840048

meshcollider commented at 9:40 pm on January 7, 2020: contributor

utACK 091a876664af4427db670ea8244d713b1b840048

meshcollider referenced this in commit 45f151913e on Jan 7, 2020

meshcollider merged this on Jan 7, 2020

meshcollider closed this on Jan 7, 2020

sidhujag referenced this in commit 1dc9152126 on Jan 8, 2020

fanquake referenced this in commit 742f84d0de on Jan 22, 2020

sidhujag referenced this in commit 8a43f5f9df on Jan 24, 2020

sidhujag referenced this in commit 27992b47ef on Nov 10, 2020

sidhujag referenced this in commit 0cd2c36b07 on Nov 10, 2020

DrahtBot locked this on Feb 15, 2022

bumpfee: Return PSBT when wallet has privkeys disabled #16373

Conflicts