Enhance `bumpfee` to include inputs when targeting a feerate #15557

instagibbs · 2019-03-07T18:12:12Z

When targeting a feerate using bumpfee, call a new function that directly uses CWallet::CreateTransaction and coin control to get the desired result. This allows us to get a superset of previous behavior, with an arbitrary RBF bump of a transaction provided it passes the preconditional checks and spare confirmed utxos are available.

Note(s):
0) The coin selection will use knapsack solver for the residual selection.

This functionality, just like knapsack coin selection in general, will hoover up negative-value inputs when given the chance.
Newly added inputs must be confirmed due to current Core policy. See error: replacement-adds-unconfirmed
Supporting this with totalFee is difficult since the "minimum total fee" option in CreateTransaction logic was (rightly)taken out in [wallet] remove minimum total fee option #10390 .

instagibbs · 2019-03-07T18:38:06Z

A slight alternative is to just nuke totalFee from orbit and delete all the redundant code.

promag

Concept ACK

promag · 2019-03-07T18:23:20Z

src/wallet/coincontrol.h

nit, int m_min_depth{0};

promag · 2019-03-07T18:38:32Z

src/interfaces/wallet.cpp

Could have this in a function:

feebumper::CreateTransaction(...) if (total_fee > 0) { return feebumper::CreateTotalBumpTransaction(...) } else { return feebumper::CreateRateBumpTransaction(...) } }

CreateTransaction always messed with my ctags anyways because of CWallet's ... :P

instagibbs · 2019-03-07T18:41:23Z

having QT unit test issues... anyone know how that's possible given the diff?

DrahtBot · 2019-03-07T20:04:19Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#15656 (wallet: Keep all outputs in bumpfee by promag)
#15157 (rpc: Bumpfee units change, satoshis to BTC by benthecarman)
#14641 (rpc: Add min_conf option to fund transaction calls by promag)
#12096 ([rpc] [wallet] Allow specifying the output index when using bumpfee by kallewoof)

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.

instagibbs · 2019-03-07T20:30:03Z

fixed QT issue, addressed nit

Sjors · 2019-03-08T17:41:42Z

Concept ACK, including on removing totalFee.

Can you split CreateRateBumpTransaction into a function that creates the transaction and one that signs it? That should make it easier to use PSBT and e.g. add signerbumpfee to #14912.

instagibbs · 2019-03-08T17:56:59Z

Can you split CreateRateBumpTransaction into a function that creates the transaction and one that signs it?

It already doesn't sign anything.

including on removing totalFee

I Looked at doing this and it removes a number of test cases that I'd have to think about harder how to test with a fee-rate only solution. Still might be worth it, but I left as-is for now since testing can be bolstered then totalFee removed at any point after.

Sjors · 2019-03-08T19:42:11Z

src/wallet/feebumper.cpp

I think you should store the change address and reuse it when present.

fixed, and added test for both bump modes.

practicalswift · 2019-03-09T07:07:12Z

src/wallet/rpcwallet.cpp

Should be "sufficient" :-)

practicalswift · 2019-03-09T07:07:55Z

test/functional/wallet_bumpfee.py

Nit: for _ in range(12): is more idiomatic when _ is unused :-)

using it now :)

stevenroose

nits; Concept ACK

stevenroose · 2019-03-14T15:11:09Z

src/wallet/feebumper.cpp

This huge indentation seems unnecessary.

stevenroose · 2019-03-14T15:11:26Z

src/wallet/feebumper.cpp

This huge indentation seems unnecessary.

promag · 2019-03-17T23:22:29Z

@instagibbs have you considered using CWallet::FundTransaction? Maybe the reason to not use it is too obvious, what am I missing?

instagibbs · 2019-03-18T13:23:56Z

@promag well for one FundTransaction doesn't have any notion of pre-existing change outputs, so I'd still have to pre-process that information. I don't see much/any possibility for code reduction?

ryanofsky

utACK 5cedf2295934637af451e977ed13d61762f41d0b. There's a little bit of redundant code added here, but it seems fine, especially if total fee option goes away in the future. I like the new code, though and the tests are very clean and direct.

I left some suggestions below, but feel free to ignore them. This change should get some release notes now that bumpfee will add new inputs and add a new change output or consolidate existing change outputs in cases where it would previously fail.

ryanofsky · 2019-03-19T15:51:48Z

src/wallet/rpcwallet.cpp

In commit "generalize bumpfee to add inputs when needed" (9240ff641006e8c3c76721232b46d99934f8431e)

Can you change "If totalFee is given no new inputs will be selected" to "If totalFee is given, adding new inputs is not supported"? I was confused looking at this and trying to figure out how totalFee had anything to do with adding new inputs, until I read the PR history.

Also would change "so the change output must be big out enough" to "so there must be a single change output that is big enough" since multiple change outputs in this case is still an error.

nevermind, re-read

ryanofsky · 2019-03-19T16:16:16Z

src/wallet/rpcwallet.cpp

In commit "generalize bumpfee to add inputs when needed" (9240ff641006e8c3c76721232b46d99934f8431e)

Would suggest changing "The command will pay the additional fee by decreasing (or perhaps removing) its change output or adding inputs when necessary." to "The command will pay the additional fee by reducing change outputs or adding inputs when necessary. It may add a new change output if one does not already exist." since the requirement of having a single change output is dropped and having 0 or multiple change outputs in the original transaction no longer triggers errors.

ryanofsky · 2019-03-19T16:40:36Z

test/functional/wallet_bumpfee.py

In commit "add functional tests for feerate bumpfee with adding inputs" (a14707cb44f300dbeef94b84746fba943a5c29ba)

Could remove line break since this is shorter.

ryanofsky · 2019-03-19T16:48:52Z

test/functional/wallet_bumpfee.py

In commit "add functional tests for feerate bumpfee with adding inputs" (a14707cb44f300dbeef94b84746fba943a5c29ba)

Note: New change_size parameter doesn't actually seem to be used in this PR. Seems fine to keep though, I think it makes the code more readable.

Yes I previously used it in tests but ended up not needing it with smarter looping checks.

ryanofsky · 2019-03-19T17:00:04Z

test/functional/wallet_bumpfee.py

In commit "wallet_bumpfee.py: add test for change key preservation" (5cedf2295934637af451e977ed13d61762f41d0b)

If none of the outputs are change, address will just be set to the last output address. Probably should add change_address = None before the loop, change_address = address before the break, and use change_address instead of address below.

ryanofsky · 2019-03-19T17:02:16Z

test/functional/wallet_bumpfee.py

In commit "wallet_bumpfee.py: add test for change key preservation" (5cedf2295934637af451e977ed13d61762f41d0b):

== True is usually avoided in python (https://docs.quantifiedcode.com/python-anti-patterns/readability/comparison_to_true.html). Can just drop it here and below.

instagibbs · 2019-03-19T18:27:17Z

@ryanofsky good suggestions thanks, fixed.

ryanofsky

utACK eb6bc93125723362b5f097e14671cd41b54d7919. Just contains suggested changes since last review. (Thanks!)

ryanofsky · 2019-03-19T18:35:09Z

src/wallet/rpcwallet.cpp

In commit "generalize bumpfee to add inputs when needed" (d5c0e53264a7eaf72c2691e3c19ce9fc60d3acf6)

I think this needs a comma after "given" to be grammatical (sorry to be pedantic, but I did double-check https://www.ego4u.com/en/cram-up/writing/comma?10 ).

commas are for dweebs. fixed.

ryanofsky

utACK 06155d16cbc3f96db26ecb22a35153cb87eec6b0. Changes since last review: dweebified

promag · 2019-04-14T14:17:19Z

utACK 01c1cb8.

…erate 184f878 wallet_bumpfee.py: add test for change key preservation (Gregory Sanders) d08becf add functional tests for feerate bumpfee with adding inputs (Gregory Sanders) 0ea47ba generalize bumpfee to add inputs when needed (Gregory Sanders) Pull request description: When targeting a feerate using `bumpfee`, call a new function that directly uses `CWallet::CreateTransaction` and coin control to get the desired result. This allows us to get a superset of previous behavior, with an arbitrary RBF bump of a transaction provided it passes the preconditional checks and spare confirmed utxos are available. Note(s): 0) The coin selection will use knapsack solver for the residual selection. 1) This functionality, just like knapsack coin selection in general, will hoover up negative-value inputs when given the chance. 2) Newly added inputs must be confirmed due to current Core policy. See error: `replacement-adds-unconfirmed` 3) Supporting this with `totalFee` is difficult since the "minimum total fee" option in `CreateTransaction` logic was (rightly)taken out in #10390 . ACKs for commit 184f87: jnewbery: utACK 184f878 Tree-SHA512: fb6542bdfb2c6010e328ec475cf9dcbff4eb2b1a1b27f78010214534908987a5635797196fa05edddffcbcf2987335872dc644a99261886d5cbb34a8f262ad3e

f1a77b0 [docs] Add doxygen comment for CReserveKey (John Newbery) 37796b2 [docs] Add doxygen comment for CKeyPool (John Newbery) ef2d515 [wallet] move-only: move CReserveKey to be next to CKeyPool (John Newbery) Pull request description: Docs/move-only Adds doxygen comments for the CKeyPool and CReserveKey objects. The way these work is pretty confusing and it's easy to overlook details (eg #15557 (comment)). These are on the verbose side, but I think too much commenting is better than not enough. Happy to take feedback on what's an appropriate level. ACKs for commit f1a77b: jonatack: Thanks, John. Re-ACK f1a77b0, doc-only changes with respect to previous review. jb55: ACK f1a77b0 Tree-SHA512: 8bc97c7029cd2e8d9bfd2d2144eeff73474c71eda5a9d10817e1578ca0b70da677252037d83143faaff1808e2193408a21a8a89d36049eac77fd313990f0b67b

f1a77b0 [docs] Add doxygen comment for CReserveKey (John Newbery) 37796b2 [docs] Add doxygen comment for CKeyPool (John Newbery) ef2d515 [wallet] move-only: move CReserveKey to be next to CKeyPool (John Newbery) Pull request description: Docs/move-only Adds doxygen comments for the CKeyPool and CReserveKey objects. The way these work is pretty confusing and it's easy to overlook details (eg bitcoin#15557 (comment)). These are on the verbose side, but I think too much commenting is better than not enough. Happy to take feedback on what's an appropriate level. ACKs for commit f1a77b: jonatack: Thanks, John. Re-ACK f1a77b0, doc-only changes with respect to previous review. jb55: ACK f1a77b0 Tree-SHA512: 8bc97c7029cd2e8d9bfd2d2144eeff73474c71eda5a9d10817e1578ca0b70da677252037d83143faaff1808e2193408a21a8a89d36049eac77fd313990f0b67b

…Transaction e10e1e8 Restrict lifetime of ReserveDestination to CWallet::CreateTransaction (Gregory Sanders) d9ff862 CreateTransaction calls KeepDestination on ReserveDestination before success (Gregory Sanders) Pull request description: The typical usage pattern of `ReserveDestination` is to explicitly `KeepDestination`, or `ReturnDestination` when it's detected it will not be used. Implementers such as myself may fail to complete this pattern, and could result in key re-use: #15557 (comment) Since ReserveDestination is currently only used directly in the `CreateTransaction`/`CommitTransaction` flow(or fee bumping where it's just used in `CreateTransaction`), I instead make the assumption that if a transaction is returned by `CreateTransaction` it's highly likely that it will be accepted by the caller, and the `ReserveDestination` kept. This simplifies the API as well. There are very few cases where this would not be the case which may result in keys being burned. Those failure cases appear to be: `CommitTransaction` failing to get the transaction into the mempool Belt and suspenders check in `WalletModel::prepareTransaction` Alternative to #15796 ACKs for top commit: achow101: ACK e10e1e8 Reviewed the diff stevenroose: utACK e10e1e8 meshcollider: utACK e10e1e8 Tree-SHA512: 78d047a00f39ab41cfa297052cc1e9c224d5f47d3d2299face650d71827635de077ac33fb4ab9f7dc6fc5a27f4a68415a1bc9ca33a3cb09a78f4f15b2a48411b

… CreateTransaction e10e1e8 Restrict lifetime of ReserveDestination to CWallet::CreateTransaction (Gregory Sanders) d9ff862 CreateTransaction calls KeepDestination on ReserveDestination before success (Gregory Sanders) Pull request description: The typical usage pattern of `ReserveDestination` is to explicitly `KeepDestination`, or `ReturnDestination` when it's detected it will not be used. Implementers such as myself may fail to complete this pattern, and could result in key re-use: bitcoin#15557 (comment) Since ReserveDestination is currently only used directly in the `CreateTransaction`/`CommitTransaction` flow(or fee bumping where it's just used in `CreateTransaction`), I instead make the assumption that if a transaction is returned by `CreateTransaction` it's highly likely that it will be accepted by the caller, and the `ReserveDestination` kept. This simplifies the API as well. There are very few cases where this would not be the case which may result in keys being burned. Those failure cases appear to be: `CommitTransaction` failing to get the transaction into the mempool Belt and suspenders check in `WalletModel::prepareTransaction` Alternative to bitcoin#15796 ACKs for top commit: achow101: ACK e10e1e8 Reviewed the diff stevenroose: utACK e10e1e8 meshcollider: utACK e10e1e8 Tree-SHA512: 78d047a00f39ab41cfa297052cc1e9c224d5f47d3d2299face650d71827635de077ac33fb4ab9f7dc6fc5a27f4a68415a1bc9ca33a3cb09a78f4f15b2a48411b

Remove last few instances of accesses to node global variables from wallet code. Also remove accesses to node globals from code in policy/policy.cpp that isn't actually called by wallet code, but does get linked into wallet code. This is the last change needed to allow bitcoin-wallet tool to be linked without depending on libbitcoin_server.a, to ensure wallet code doesn't access node global state and avoid bugs like bitcoin#15557 (comment)

This ensures wallet code doesn't access node global state, avoiding bugs like bitcoin#15557 (comment)

Remove last few instances of accesses to node global variables from wallet code. Also remove accesses to node globals from code in policy/policy.cpp that isn't actually called by wallet code, but does get linked into wallet code. This is the last change needed to allow bitcoin-wallet tool to be linked without depending on libbitcoin_server.a, to ensure wallet code doesn't access node global state and avoid bugs like bitcoin/bitcoin#15557 (comment)

This ensures wallet code doesn't access node global state, avoiding bugs like bitcoin/bitcoin#15557 (comment)

Summary: Remove last few instances of accesses to node global variables from wallet code. Also remove accesses to node globals from code in policy/policy.cpp that isn't actually called by wallet code, but does get linked into wallet code. This is the last change needed to allow bitcoin-wallet tool to be linked without depending on libbitcoin_server.a, to ensure wallet code doesn't access node global state and avoid bugs like bitcoin/bitcoin#15557 (comment) --- bitcoin/bitcoin@b874747 --- Depends on D6233 Partial backport of Core [[bitcoin/bitcoin#15639 | PR15639]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6255

…ency Summary: This ensures wallet code doesn't access node global state, avoiding bugs like bitcoin/bitcoin#15557 (comment) --- bitcoin/bitcoin@78a2fb5 --- Depends on D6255 Concludes backport of Core [[bitcoin/bitcoin#15639 | PR15639]] Test Plan: ninja check-all Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6256

…pendency Summary: This ensures wallet code doesn't access node global state, avoiding bugs like bitcoin/bitcoin#15557 (comment) --- bitcoin/bitcoin@78a2fb5 Test Plan: ninja check-all [also with -DBUILD_BITCOIN_WALLET=OFF] make check-recursive ./test/functional/test_runner.py [also with --disable-wallet] Reviewers: #bitcoin_abc, deadalnix Reviewed By: #bitcoin_abc, deadalnix Subscribers: deadalnix Differential Revision: https://reviews.bitcoinabc.org/D6795

…Transaction e10e1e8 Restrict lifetime of ReserveDestination to CWallet::CreateTransaction (Gregory Sanders) d9ff862 CreateTransaction calls KeepDestination on ReserveDestination before success (Gregory Sanders) Pull request description: The typical usage pattern of `ReserveDestination` is to explicitly `KeepDestination`, or `ReturnDestination` when it's detected it will not be used. Implementers such as myself may fail to complete this pattern, and could result in key re-use: bitcoin/bitcoin#15557 (comment) Since ReserveDestination is currently only used directly in the `CreateTransaction`/`CommitTransaction` flow(or fee bumping where it's just used in `CreateTransaction`), I instead make the assumption that if a transaction is returned by `CreateTransaction` it's highly likely that it will be accepted by the caller, and the `ReserveDestination` kept. This simplifies the API as well. There are very few cases where this would not be the case which may result in keys being burned. Those failure cases appear to be: `CommitTransaction` failing to get the transaction into the mempool Belt and suspenders check in `WalletModel::prepareTransaction` Alternative to bitcoin/bitcoin#15796 ACKs for top commit: achow101: ACK e10e1e8 Reviewed the diff stevenroose: utACK e10e1e8 meshcollider: utACK e10e1e8 Tree-SHA512: 78d047a00f39ab41cfa297052cc1e9c224d5f47d3d2299face650d71827635de077ac33fb4ab9f7dc6fc5a27f4a68415a1bc9ca33a3cb09a78f4f15b2a48411b

…ng a feerate 184f878 wallet_bumpfee.py: add test for change key preservation (Gregory Sanders) d08becf add functional tests for feerate bumpfee with adding inputs (Gregory Sanders) 0ea47ba generalize bumpfee to add inputs when needed (Gregory Sanders) Pull request description: When targeting a feerate using `bumpfee`, call a new function that directly uses `CWallet::CreateTransaction` and coin control to get the desired result. This allows us to get a superset of previous behavior, with an arbitrary RBF bump of a transaction provided it passes the preconditional checks and spare confirmed utxos are available. Note(s): 0) The coin selection will use knapsack solver for the residual selection. 1) This functionality, just like knapsack coin selection in general, will hoover up negative-value inputs when given the chance. 2) Newly added inputs must be confirmed due to current Core policy. See error: `replacement-adds-unconfirmed` 3) Supporting this with `totalFee` is difficult since the "minimum total fee" option in `CreateTransaction` logic was (rightly)taken out in bitcoin#10390 . ACKs for commit 184f87: jnewbery: utACK 184f878 Tree-SHA512: fb6542bdfb2c6010e328ec475cf9dcbff4eb2b1a1b27f78010214534908987a5635797196fa05edddffcbcf2987335872dc644a99261886d5cbb34a8f262ad3e

instagibbs force-pushed the bumpall branch from b138547 to 8697b6d Compare March 7, 2019 18:13

promag reviewed Mar 7, 2019

View reviewed changes

instagibbs force-pushed the bumpall branch from 8697b6d to 8b1e615 Compare March 7, 2019 20:27

fanquake added the TX fees and policy label Mar 8, 2019

instagibbs force-pushed the bumpall branch 2 times, most recently from 9365da5 to 7aca983 Compare March 8, 2019 16:33

Sjors reviewed Mar 8, 2019

View reviewed changes

instagibbs force-pushed the bumpall branch 2 times, most recently from 80181c9 to 5a94413 Compare March 9, 2019 00:58

practicalswift reviewed Mar 9, 2019

View reviewed changes

instagibbs force-pushed the bumpall branch 4 times, most recently from a296975 to 5cedf22 Compare March 9, 2019 14:17

stevenroose reviewed Mar 14, 2019

View reviewed changes

ryanofsky approved these changes Mar 19, 2019

View reviewed changes

instagibbs force-pushed the bumpall branch from 5cedf22 to eb6bc93 Compare March 19, 2019 18:27

ryanofsky approved these changes Mar 19, 2019

View reviewed changes

instagibbs force-pushed the bumpall branch from eb6bc93 to 06155d1 Compare March 19, 2019 18:46

ryanofsky approved these changes Mar 19, 2019

View reviewed changes

instagibbs mentioned this pull request Apr 11, 2019

CReserveKey should not allow passive key re-use, debug assert in destructor #15796

Closed

meshcollider merged commit 184f878 into bitcoin:master Apr 14, 2019

fanquake mentioned this pull request Apr 15, 2019

scripts: GitHub merge script is missing review comments #15816

Closed

jnewbery mentioned this pull request Apr 22, 2019

wallet: Remove AvailableCoins nMinDepth argument #15823

Closed

instagibbs mentioned this pull request Jun 13, 2019

wallet: Consume ReserveDestination on successful CreateTransaction #16208

Merged

HashUnlimited pushed a commit to HashUnlimited/chaincoin that referenced this pull request Aug 30, 2019

bitcoin-wallet tool: Drop libbitcoin_server.a dependency

cb7e65c

This ensures wallet code doesn't access node global state, avoiding bugs like bitcoin#15557 (comment)

barrystyle pushed a commit to zentoshi/zentoshi that referenced this pull request Nov 11, 2019

bitcoin-wallet tool: Drop libbitcoin_server.a dependency

d9cb43f

This ensures wallet code doesn't access node global state, avoiding bugs like bitcoin/bitcoin#15557 (comment)

jonatack mentioned this pull request May 23, 2020

Rationalize components bitcoin-core-review-club/website#192

Merged

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

Enhance bumpfee to include inputs when targeting a feerate #15557

Enhance bumpfee to include inputs when targeting a feerate #15557

Uh oh!

Conversation

instagibbs commented Mar 7, 2019

Uh oh!

instagibbs commented Mar 7, 2019

Uh oh!

promag left a comment

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!

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

Uh oh!

Uh oh!

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

Uh oh!

Conflicts

Uh oh!

instagibbs commented Mar 7, 2019

Uh oh!

Sjors commented Mar 8, 2019

Uh oh!

instagibbs commented Mar 8, 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!

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!

stevenroose left a comment

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 commented Mar 17, 2019

Uh oh!

instagibbs commented Mar 18, 2019

Uh oh!

ryanofsky left a comment

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!

ryanofsky Mar 19, 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!

instagibbs commented Mar 19, 2019

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Enhance `bumpfee` to include inputs when targeting a feerate #15557

Enhance `bumpfee` to include inputs when targeting a feerate #15557

instagibbs commented Mar 7, 2019 •

edited

Loading

DrahtBot commented Mar 7, 2019 •

edited

Loading

ryanofsky Mar 19, 2019 •

edited

Loading