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

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

Merged
merged 3 commits into from Apr 14, 2019

Conversation

Projects
None yet
10 participants
@instagibbs
Copy link
Member

commented Mar 7, 2019

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 .

@instagibbs instagibbs force-pushed the instagibbs:bumpall branch from b138547 to 8697b6d Mar 7, 2019

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

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

@promag
Copy link
Member

left a comment

Concept ACK

@@ -36,6 +36,8 @@ class CCoinControl
bool m_avoid_partial_spends;
//! Fee estimation mode to control arguments to estimateSmartFee
FeeEstimateMode m_fee_mode;
//! Minimum chain depth value for coin availability
int m_min_depth = 0;

This comment has been minimized.

Copy link
@promag

promag Mar 7, 2019

Member

nit, int m_min_depth{0};

return feebumper::CreateTransaction(m_wallet.get(), txid, coin_control, total_fee, errors, old_fee, new_fee, mtx) ==
feebumper::Result::OK;
if (total_fee > 0) {
return feebumper::CreateTotalBumpTransaction(m_wallet.get(), txid, coin_control, total_fee, errors, old_fee, new_fee, mtx) ==

This comment has been minimized.

Copy link
@promag

promag Mar 7, 2019

Member

Could have this in a function:

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

This comment has been minimized.

Copy link
@instagibbs

instagibbs Mar 7, 2019

Author Member

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

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

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

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Mar 7, 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:

  • #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 instagibbs force-pushed the instagibbs:bumpall branch from 8697b6d to 8b1e615 Mar 7, 2019

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Mar 7, 2019

fixed QT issue, addressed nit

@instagibbs instagibbs force-pushed the instagibbs:bumpall branch 2 times, most recently from 9365da5 to 7aca983 Mar 8, 2019

@Sjors

This comment has been minimized.

Copy link
Member

commented Mar 8, 2019

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

This comment has been minimized.

Copy link
Member Author

commented Mar 8, 2019

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.

// Fill in recipients(take out change outputs)
std::vector<CRecipient> recipients;
for (const auto& output : wtx.tx->vout) {
if (!wallet->IsChange(output)) {

This comment has been minimized.

Copy link
@Sjors

Sjors Mar 8, 2019

Member

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

This comment has been minimized.

Copy link
@instagibbs

instagibbs Mar 8, 2019

Author Member

fixed, and added test for both bump modes.

@instagibbs instagibbs force-pushed the instagibbs:bumpall branch 2 times, most recently from 80181c9 to 5a94413 Mar 8, 2019

feebumper::Result res = feebumper::CreateTransaction(pwallet, hash, coin_control, totalFee, errors, old_fee, new_fee, mtx);
feebumper::Result res;
if (totalFee > 0) {
// Targeting total fee bump. Requires a change output of sufficent size.

This comment has been minimized.

Copy link
@practicalswift

practicalswift Mar 9, 2019

Member

Should be "sufficient" :-)

This comment has been minimized.

Copy link
@instagibbs

instagibbs Mar 9, 2019

Author Member

fixed

rbfid = spend_one_input(rbf_node, dest_address, Decimal("0.00000700"))
assert_equal(len(rbf_node.getrawtransaction(rbfid, 1)["vin"]), 1)
# successfully bump this transaction a dozen times, even after tossing change
for i in range(12):

This comment has been minimized.

Copy link
@practicalswift

practicalswift Mar 9, 2019

Member

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

This comment has been minimized.

Copy link
@instagibbs

instagibbs Mar 9, 2019

Author Member

using it now :)

@instagibbs instagibbs force-pushed the instagibbs:bumpall branch 4 times, most recently from a296975 to 5cedf22 Mar 9, 2019

@stevenroose
Copy link
Contributor

left a comment

nits; Concept ACK

CAmount minTotalFee = nOldFeeRate.GetFee(maxNewTxSize) + ::incrementalRelayFee.GetFee(maxNewTxSize);
if (total_fee < minTotalFee) {
errors.push_back(strprintf("Insufficient totalFee, must be at least %s (oldFee %s + incrementalFee %s)",
FormatMoney(minTotalFee), FormatMoney(nOldFeeRate.GetFee(maxNewTxSize)), FormatMoney(::incrementalRelayFee.GetFee(maxNewTxSize))));

This comment has been minimized.

Copy link
@stevenroose

stevenroose Mar 14, 2019

Contributor

This huge indentation seems unnecessary.

CAmount requiredFee = GetRequiredFee(*wallet, maxNewTxSize);
if (total_fee < requiredFee) {
errors.push_back(strprintf("Insufficient totalFee (cannot be less than required fee %s)",
FormatMoney(requiredFee)));

This comment has been minimized.

Copy link
@stevenroose

stevenroose Mar 14, 2019

Contributor

This huge indentation seems unnecessary.

@laanwj laanwj added this to Blockers in High-priority for review Mar 14, 2019

@promag

This comment has been minimized.

Copy link
Member

commented Mar 17, 2019

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

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Mar 18, 2019

@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
Copy link
Contributor

left a comment

utACK 5cedf22. 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.

"If the change output is not big enough to cover the increased fee, the command will currently fail\n"
"instead of adding new inputs to compensate. (A future implementation could improve this.)\n"
"The command will pay the additional fee by decreasing (or perhaps removing) its change output or adding inputs when necessary.\n"
"If `totalFee` is given no new inputs will be selected, so the change output must be big enough to cover the increased fee, or it will fail.\n"

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 19, 2019

Contributor

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

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.

This comment has been minimized.

Copy link
@instagibbs

instagibbs Mar 19, 2019

Author Member

nevermind, re-read

"The command will pay the additional fee by decreasing (or perhaps removing) its change output.\n"
"If the change output is not big enough to cover the increased fee, the command will currently fail\n"
"instead of adding new inputs to compensate. (A future implementation could improve this.)\n"
"The command will pay the additional fee by decreasing (or perhaps removing) its change output or adding inputs when necessary.\n"

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 19, 2019

Contributor

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

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.

rawtx = node.createrawtransaction(
[tx_input], {dest_address: Decimal("0.00050000"),
node.getrawchangeaddress(): Decimal("0.00049000")})
[tx_input], destinations)

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 19, 2019

Contributor

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

Could remove line break since this is shorter.



def spend_one_input(node, dest_address):
def spend_one_input(node, dest_address, change_size=Decimal("0.00049000")):

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 19, 2019

Contributor

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

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.

This comment has been minimized.

Copy link
@instagibbs

instagibbs Mar 19, 2019

Author Member

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

address = out["scriptPubKey"]["addresses"][0]
if rbf_node.getaddressinfo(address)["ischange"] == True:
break
assert(address is not None)

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 19, 2019

Contributor

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

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.

# find change from original
for out in unbumped_details["vout"]:
address = out["scriptPubKey"]["addresses"][0]
if rbf_node.getaddressinfo(address)["ischange"] == True:

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 19, 2019

Contributor

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

== 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 instagibbs force-pushed the instagibbs:bumpall branch from 5cedf22 to eb6bc93 Mar 19, 2019

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Mar 19, 2019

@ryanofsky good suggestions thanks, fixed.

@ryanofsky
Copy link
Contributor

left a comment

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

"If the change output is not big enough to cover the increased fee, the command will currently fail\n"
"instead of adding new inputs to compensate. (A future implementation could improve this.)\n"
"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.\n"
"If `totalFee` is given adding inputs is not supported, so there must be a single change output that is big enough or it will fail.\n"

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Mar 19, 2019

Contributor

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

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 :bowtie:).

This comment has been minimized.

Copy link
@instagibbs

instagibbs Mar 19, 2019

Author Member

commas are for dweebs. fixed.

@instagibbs instagibbs force-pushed the instagibbs:bumpall branch from eb6bc93 to 06155d1 Mar 19, 2019

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 10, 2019

Remove access to node globals from wallet-linked code
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)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Apr 10, 2019

bitcoin-wallet tool: Drop libbitcoin_server.a dependency
This ensures wallet code doesn't access node global state, avoiding bugs like
bitcoin#15557 (comment)
@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

My main concern is the introduction of a lot of duplicate code between CreateTotalBumpTransaction() and CreateRateBumpTransaction()

If I have proper concept ACKs my next step would likely be removing the total bump altogether, which will mostly involve re-jiggering test coverage.

@instagibbs instagibbs force-pushed the instagibbs:bumpall branch from 5164305 to b6dcb6a Apr 10, 2019

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

Addressed most comments with more concise/clear code and tests.

@jnewbery
Copy link
Member

left a comment

Looks good. Thanks for being so responsive to my feedback.

utACK b6dcb6a

I'd be happier if you could address this: #15557 (comment), but I don't think it's worth holding up this PR any longer. It can always be improved in a future PR.

@@ -17,7 +17,7 @@
import io

from test_framework.blocktools import add_witness_commitment, create_block, create_coinbase, send_to_witness
from test_framework.messages import BIP125_SEQUENCE_NUMBER, CTransaction
from test_framework.messages import BIP125_SEQUENCE_NUMBER, CTransaction, FromHex

This comment has been minimized.

Copy link
@jnewbery

jnewbery Apr 10, 2019

Member

Remove this new import of FromHex to satisfy the linter.

This comment has been minimized.

Copy link
@instagibbs

instagibbs Apr 10, 2019

Author Member

fixed.

@instagibbs instagibbs force-pushed the instagibbs:bumpall branch from b6dcb6a to 01c1cb8 Apr 10, 2019

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Apr 10, 2019

I don't have as much time as I'd like to perfect my tests sadly, thanks for the robust feedback.

meshcollider added a commit to meshcollider/bitcoin that referenced this pull request Apr 11, 2019

Merge bitcoin#15639: bitcoin-wallet tool: Drop libbitcoin_server.a de…
…pendency

78a2fb5 bitcoin-wallet tool: Drop libbitcoin_server.a dependency (Russell Yanofsky)
b874747 Remove access to node globals from wallet-linked code (Russell Yanofsky)
fbc6bb8 bitcoin-wallet tool: Drop MakeChain calls (Russell Yanofsky)

Pull request description:

  Dropping the `bitcoin-wallet` dependency on `libbitcoin_server.a` ensures wallet code can't access node global state, avoiding bugs like bitcoin#15557 (comment)

ACKs for commit 78a2fb:
  jnewbery:
    utACK 78a2fb5. Nice work, Russ.
  MarcoFalke:
    utACK 78a2fb5
  MeshCollider:
    utACK bitcoin@78a2fb5

Tree-SHA512: ee6ea774f683b936bea66638211dd53c42b8316e1ef03dd58d12fb7ee3891432a43c5c149944173c1e2436aa756b672e1679c39fc10043792ac55cd4d8af2823

@instagibbs instagibbs force-pushed the instagibbs:bumpall branch from 01c1cb8 to 184f878 Apr 11, 2019

@DrahtBot DrahtBot removed the Needs rebase label Apr 11, 2019

@jnewbery

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

utACK 184f878

@ryanofsky - are you able to reACK? Should be minor changes since your last ACK.

@Sjors @promag @stevenroose - you've all concept ACKed this. Are you able to review? Code changes are very clear so this should be a fairly easy review.

@promag

This comment has been minimized.

Copy link
Member

commented Apr 11, 2019

Someone can reply to this #15557 (comment)?

@instagibbs

This comment has been minimized.

Copy link
Member Author

commented Apr 11, 2019

@ryanofsky
Copy link
Contributor

left a comment

utACK 01c1cb8. Changes since last review: ReserveKey fix, tweaks to fee calculation code (mostly for clarification), simplifications to python tests.

new_coin_control.m_min_depth = 1;

CTransactionRef tx_new = MakeTransactionRef();
CReserveKey reservekey(wallet);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 11, 2019

Contributor

#15557 (comment)

good catch, fixed.

This was a pretty serious bug right? At least a privacy leak? Or am I mistaken?

Assuming this was serious, it seems like we should do something to prevent this happening in the future, like asserting in the CReserveKey destructor that either KeepKey or Reservekey was previously called.

@promag

This comment has been minimized.

Copy link
Member

commented Apr 14, 2019

utACK 01c1cb8.

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

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

meshcollider added a commit that referenced this pull request Apr 14, 2019

Merge #15557: Enhance `bumpfee` to include inputs when targeting a fe…
…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

@meshcollider meshcollider removed this from Blockers in High-priority for review Apr 14, 2019

MarcoFalke added a commit that referenced this pull request May 14, 2019

Merge #15777: [docs] Add doxygen comments for keypool classes
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

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request May 15, 2019

Merge bitcoin#15777: [docs] Add doxygen comments for keypool classes
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

Kiku-git added a commit to Kiku-git/bitcoin that referenced this pull request May 16, 2019

Remove access to node globals from wallet-linked code
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)

Kiku-git added a commit to Kiku-git/bitcoin that referenced this pull request May 16, 2019

bitcoin-wallet tool: Drop libbitcoin_server.a dependency
This ensures wallet code doesn't access node global state, avoiding bugs like
bitcoin#15557 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.