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

pull instagibbs wants to merge 3 commits into bitcoin:master from instagibbs:bumpall changing 7 files +220 −48
  1. instagibbs commented at 6:12 PM on March 7, 2019: member

    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 .
  2. instagibbs force-pushed on Mar 7, 2019
  3. in src/wallet/coincontrol.h:40 in 8697b6dba9 outdated 
      35 | @@ -36,6 +36,8 @@ class CCoinControl
  36 |      bool m_avoid_partial_spends;
  37 |      //! Fee estimation mode to control arguments to estimateSmartFee
  38 |      FeeEstimateMode m_fee_mode;
  39 | +    //! Minimum chain depth value for coin availability
  40 | +    int m_min_depth = 0;
    promag commented at 6:23 PM on March 7, 2019:

    nit, int m_min_depth{0};

  4. instagibbs commented at 6:38 PM on March 7, 2019: member

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

  5. in src/interfaces/wallet.cpp:273 in 8697b6dba9 outdated 
     273 | @@ -274,8 +274,13 @@ class WalletImpl : public Wallet
 274 |          CAmount& new_fee,
 275 |          CMutableTransaction& mtx) override
 276 |      {
 277 | -        return feebumper::CreateTransaction(m_wallet.get(), txid, coin_control, total_fee, errors, old_fee, new_fee, mtx) ==
 278 | -               feebumper::Result::OK;
 279 | +        if (total_fee > 0) {
 280 | +            return feebumper::CreateTotalBumpTransaction(m_wallet.get(), txid, coin_control, total_fee, errors, old_fee, new_fee, mtx) ==
    promag commented at 6:38 PM on March 7, 2019:

    Could have this in a function:

    feebumper::CreateTransaction(...)
    if (total_fee > 0) {
        return feebumper::CreateTotalBumpTransaction(...) 
    } else {
        return feebumper::CreateRateBumpTransaction(...)
    }
}
    instagibbs commented at 6:42 PM on March 7, 2019:

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

  6. promag commented at 6:38 PM on March 7, 2019: member

    Concept ACK

  7. instagibbs commented at 6:41 PM on March 7, 2019: member

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

  8. DrahtBot commented at 8:04 PM on March 7, 2019: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  9. instagibbs force-pushed on Mar 7, 2019
  10. instagibbs commented at 8:30 PM on March 7, 2019: member

    fixed QT issue, addressed nit

  11. fanquake added the label TX fees and policy on Mar 8, 2019
  12. instagibbs force-pushed on Mar 8, 2019
  13. instagibbs force-pushed on Mar 8, 2019
  14. Sjors commented at 5:41 PM on March 8, 2019: member

    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.

  15. instagibbs commented at 5:56 PM on March 8, 2019: member

    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.

  16. in src/wallet/feebumper.cpp:227 in 7aca983958 outdated 
     216 | +    }
 217 | +
 218 | +    // Fill in recipients(take out change outputs)
 219 | +    std::vector<CRecipient> recipients;
 220 | +    for (const auto& output : wtx.tx->vout) {
 221 | +        if (!wallet->IsChange(output)) {
    Sjors commented at 7:42 PM on March 8, 2019:

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

    instagibbs commented at 9:44 PM on March 8, 2019:

    fixed, and added test for both bump modes.

  17. instagibbs force-pushed on Mar 8, 2019
  18. instagibbs force-pushed on Mar 9, 2019
  19. in src/wallet/rpcwallet.cpp:3317 in 5a9441348e outdated 
    3311 | @@ -3313,7 +3312,14 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3312 |      CAmount old_fee;
3313 |      CAmount new_fee;
3314 |      CMutableTransaction mtx;
3315 | -    feebumper::Result res = feebumper::CreateTransaction(pwallet, hash, coin_control, totalFee, errors, old_fee, new_fee, mtx);
3316 | +    feebumper::Result res;
3317 | +    if (totalFee > 0) {
3318 | +        // Targeting total fee bump. Requires a change output of sufficent size.
    practicalswift commented at 7:07 AM on March 9, 2019:

    Should be "sufficient" :-)

    instagibbs commented at 1:53 PM on March 9, 2019:

    fixed

  20. in test/functional/wallet_bumpfee.py:193 in 5a9441348e outdated 
     188 | +    # Make sure additional inputs exist
 189 | +    rbf_node.generatetoaddress(101, rbf_node.getnewaddress())
 190 | +    rbfid = spend_one_input(rbf_node, dest_address, Decimal("0.00000700"))
 191 | +    assert_equal(len(rbf_node.getrawtransaction(rbfid, 1)["vin"]), 1)
 192 | +    # successfully bump this transaction a dozen times, even after tossing change
 193 | +    for i in range(12):
    practicalswift commented at 7:07 AM on March 9, 2019:

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

    instagibbs commented at 1:53 PM on March 9, 2019:

    using it now :)

  21. instagibbs force-pushed on Mar 9, 2019
  22. instagibbs force-pushed on Mar 9, 2019
  23. instagibbs force-pushed on Mar 9, 2019
  24. instagibbs force-pushed on Mar 9, 2019
  25. in src/wallet/feebumper.cpp:134 in 9240ff6410 outdated 
     157 | -            new_fee = nNewFeeRate.GetFee(maxNewTxSize);
 158 | -        }
 159 | +    CAmount minTotalFee = nOldFeeRate.GetFee(maxNewTxSize) + ::incrementalRelayFee.GetFee(maxNewTxSize);
 160 | +    if (total_fee < minTotalFee) {
 161 | +        errors.push_back(strprintf("Insufficient totalFee, must be at least %s (oldFee %s + incrementalFee %s)",
 162 | +                                                            FormatMoney(minTotalFee), FormatMoney(nOldFeeRate.GetFee(maxNewTxSize)), FormatMoney(::incrementalRelayFee.GetFee(maxNewTxSize))));
    stevenroose commented at 3:11 PM on March 14, 2019:

    This huge indentation seems unnecessary.

  26. in src/wallet/feebumper.cpp:140 in 9240ff6410 outdated 
     163 | +        return Result::INVALID_PARAMETER;
 164 | +    }
 165 | +    CAmount requiredFee = GetRequiredFee(*wallet, maxNewTxSize);
 166 | +    if (total_fee < requiredFee) {
 167 | +        errors.push_back(strprintf("Insufficient totalFee (cannot be less than required fee %s)",
 168 | +                                                            FormatMoney(requiredFee)));
    stevenroose commented at 3:11 PM on March 14, 2019:

    This huge indentation seems unnecessary.

  27. stevenroose commented at 3:13 PM on March 14, 2019: contributor

    nits; Concept ACK

  28. laanwj added this to the "Blockers" column in a project
  29. promag commented at 11:22 PM on March 17, 2019: member

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

  30. instagibbs commented at 1:23 PM on March 18, 2019: member

    @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?

  31. in src/wallet/rpcwallet.cpp:3218 in 9240ff6410 outdated 
    3213 | @@ -3214,9 +3214,8 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3214 |              RPCHelpMan{"bumpfee",
3215 |                  "\nBumps the fee of an opt-in-RBF transaction T, replacing it with a new transaction B.\n"
3216 |                  "An opt-in RBF transaction with the given txid must be in the wallet.\n"
3217 | -                "The command will pay the additional fee by decreasing (or perhaps removing) its change output.\n"
3218 | -                "If the change output is not big enough to cover the increased fee, the command will currently fail\n"
3219 | -                "instead of adding new inputs to compensate. (A future implementation could improve this.)\n"
3220 | +                "The command will pay the additional fee by decreasing (or perhaps removing) its change output or adding inputs when necessary.\n"
3221 | +                "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"
    ryanofsky commented at 3:51 PM on March 19, 2019:

    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.

    instagibbs commented at 6:05 PM on March 19, 2019:

    nevermind, re-read

  32. in src/wallet/rpcwallet.cpp:3217 in 9240ff6410 outdated 
    3213 | @@ -3214,9 +3214,8 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3214 |              RPCHelpMan{"bumpfee",
3215 |                  "\nBumps the fee of an opt-in-RBF transaction T, replacing it with a new transaction B.\n"
3216 |                  "An opt-in RBF transaction with the given txid must be in the wallet.\n"
3217 | -                "The command will pay the additional fee by decreasing (or perhaps removing) its change output.\n"
3218 | -                "If the change output is not big enough to cover the increased fee, the command will currently fail\n"
3219 | -                "instead of adding new inputs to compensate. (A future implementation could improve this.)\n"
3220 | +                "The command will pay the additional fee by decreasing (or perhaps removing) its change output or adding inputs when necessary.\n"
    ryanofsky commented at 4:16 PM on March 19, 2019:

    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.

  33. in test/functional/wallet_bumpfee.py:316 in a14707cb44 outdated 
     314 | +    if change_size > 0:
 315 | +        destinations[node.getrawchangeaddress()] = change_size
 316 |      rawtx = node.createrawtransaction(
 317 | -        [tx_input], {dest_address: Decimal("0.00050000"),
 318 | -                     node.getrawchangeaddress(): Decimal("0.00049000")})
 319 | +        [tx_input], destinations)
    ryanofsky commented at 4:40 PM on March 19, 2019:

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

    Could remove line break since this is shorter.

  34. in test/functional/wallet_bumpfee.py:333 in a14707cb44 outdated 
     302 | @@ -272,19 +303,21 @@ def test_locked_wallet_fails(rbf_node, dest_address):
 303 |      rbf_node.walletlock()
 304 |      assert_raises_rpc_error(-13, "Please enter the wallet passphrase with walletpassphrase first.",
 305 |                              rbf_node.bumpfee, rbfid)
 306 | +    rbf_node.walletpassphrase(WALLET_PASSPHRASE, WALLET_PASSPHRASE_TIMEOUT)
 307 |  
 308 |  
 309 | -def spend_one_input(node, dest_address):
 310 | +def spend_one_input(node, dest_address, change_size=Decimal("0.00049000")):
    ryanofsky commented at 4:48 PM on March 19, 2019:

    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.

    instagibbs commented at 5:24 PM on March 19, 2019:

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

  35. in test/functional/wallet_bumpfee.py:345 in 5cedf22959 outdated 
     317 | +    # find change from original
 318 | +    for out in unbumped_details["vout"]:
 319 | +        address = out["scriptPubKey"]["addresses"][0]
 320 | +        if rbf_node.getaddressinfo(address)["ischange"] == True:
 321 | +            break
 322 | +    assert(address is not None)
    ryanofsky commented at 5:00 PM on March 19, 2019:

    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.

  36. in test/functional/wallet_bumpfee.py:320 in 5cedf22959 outdated 
     315 | +    rate_bumped_details = rbf_node.getrawtransaction(bumped_rate_tx["txid"], 1)
 316 | +
 317 | +    # find change from original
 318 | +    for out in unbumped_details["vout"]:
 319 | +        address = out["scriptPubKey"]["addresses"][0]
 320 | +        if rbf_node.getaddressinfo(address)["ischange"] == True:
    ryanofsky commented at 5:02 PM on March 19, 2019:

    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.

  37. ryanofsky approved
  38. ryanofsky commented at 5:12 PM on March 19, 2019: member

    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.

  39. instagibbs force-pushed on Mar 19, 2019
  40. instagibbs commented at 6:27 PM on March 19, 2019: member

    @ryanofsky good suggestions thanks, fixed.

  41. in src/wallet/rpcwallet.cpp:3218 in d5c0e53264 outdated 
    3213 | @@ -3214,9 +3214,8 @@ static UniValue bumpfee(const JSONRPCRequest& request)
3214 |              RPCHelpMan{"bumpfee",
3215 |                  "\nBumps the fee of an opt-in-RBF transaction T, replacing it with a new transaction B.\n"
3216 |                  "An opt-in RBF transaction with the given txid must be in the wallet.\n"
3217 | -                "The command will pay the additional fee by decreasing (or perhaps removing) its change output.\n"
3218 | -                "If the change output is not big enough to cover the increased fee, the command will currently fail\n"
3219 | -                "instead of adding new inputs to compensate. (A future implementation could improve this.)\n"
3220 | +                "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"
3221 | +                "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"
    ryanofsky commented at 6:35 PM on March 19, 2019:

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

    instagibbs commented at 6:47 PM on March 19, 2019:

    commas are for dweebs. fixed.

  42. ryanofsky approved
  43. ryanofsky commented at 6:42 PM on March 19, 2019: member

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

  44. instagibbs force-pushed on Mar 19, 2019
  45. ryanofsky approved
  46. ryanofsky commented at 6:54 PM on March 19, 2019: member

    utACK 06155d16cbc3f96db26ecb22a35153cb87eec6b0. Changes since last review: dweebified

  47. in src/wallet/feebumper.cpp:233 in 06155d16cb outdated 
     225 | +            CRecipient recipient = {output.scriptPubKey, output.nValue, false};
 226 | +            recipients.push_back(recipient);
 227 | +        } else {
 228 | +            CTxDestination change_dest;
 229 | +            ExtractDestination(output.scriptPubKey, change_dest);
 230 | +            new_coin_control.destChange = change_dest;
    promag commented at 8:04 PM on March 20, 2019:

    If by any chance there are multiple IsChange outputs the last is used and the others are removed. Is this acceptable? A simple solution is to check:

    if (wallet->IsChange(output) && new_coin_control.destChange != CNoDestination) {
    // preserve first change
} else {
    // add recipient
}
    jnewbery commented at 3:27 PM on April 11, 2019:

    I think it's acceptable. I don't believe that Bitcoin Core wallet will produce transactions with more than one change output.

    EDIT: ... and if a user manually creates a transaction with more than one change output, I think it's fine for bumpfee to aggregate them into a single change output (since there's no way for the user to specify which change output should have the fee delta removed).

    promag commented at 2:05 PM on April 14, 2019:

    Thanks @jnewbery.

  48. DrahtBot added the label Needs rebase on Mar 21, 2019
  49. instagibbs force-pushed on Mar 21, 2019
  50. instagibbs commented at 11:30 AM on March 21, 2019: member

    rebased

  51. DrahtBot removed the label Needs rebase on Mar 21, 2019
  52. in src/wallet/feebumper.cpp:135 in ee4fc9ab67 outdated 
     158 | -            new_fee = nNewFeeRate.GetFee(maxNewTxSize);
 159 | -        }
 160 | +    CAmount minTotalFee = nOldFeeRate.GetFee(maxNewTxSize) + nodeIncrementalRelayFee.GetFee(maxNewTxSize);
 161 | +    if (total_fee < minTotalFee) {
 162 | +        errors.push_back(strprintf("Insufficient totalFee, must be at least %s (oldFee %s + incrementalFee %s)",
 163 | +            FormatMoney(minTotalFee), FormatMoney(nOldFeeRate.GetFee(maxNewTxSize)), FormatMoney(::incrementalRelayFee.GetFee(maxNewTxSize))));
    ryanofsky commented at 12:34 PM on March 21, 2019:

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

    Here and one place below, s/::incrementalRelayFee/nodeIncrementalRelayFee/

    (Sorry about this. I need to make some build changes that will cause accesses to node globals from wallet code to result in explicit link errors: #10973#pullrequestreview-213000398)

    instagibbs commented at 12:44 PM on March 21, 2019:

    argh sorry for not reading more carefully. fixed.

  53. instagibbs force-pushed on Mar 21, 2019
  54. ryanofsky approved
  55. ryanofsky commented at 1:07 PM on March 21, 2019: member

    utACK 0ad9add7531603ca50481bc20b38597ba8a47cfd. Only change since last review is rebase.

  56. jnewbery commented at 7:43 PM on March 21, 2019: member

    This is great. Strong concept ACK.

    BIP125 doesn't require all the inputs in the replaced transaction to be used in the replacement transaction, but it's very important for wallets to make sure that happens. If not, it would be possible to bump a transaction A twice to A2 and A3 where A2 and A3 don't conflict (or alternatively bump A to A2 and A2 to A3 where A and A3 don't conflict). If both later get confirmed then the sender has accidentally double paid.

    That would be a funds loss scenario, so I don't think it'd be overboard to add user documentation, code comments and testing:

  57. promag commented at 12:51 AM on March 22, 2019: member

    If both later get confirmed then the sender has accidentally double paid.

    If there's no conflict how is it double paid?

  58. instagibbs commented at 12:54 AM on March 22, 2019: member

    Conflict with respect to spent I inputs not destinations.

    On Thu, Mar 21, 2019, 8:52 PM João Barbosa notifications@github.com wrote:

    If both later get confirmed then the sender has accidentally double paid.

    If there's no conflict how is it double paid?

    — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/15557#issuecomment-475455806, or mute the thread https://github.com/notifications/unsubscribe-auth/AFgC08m_IevYIbq5sD3pVnENIsm_ISWyks5vZClqgaJpZM4bjzhN .

  59. ryanofsky referenced this in commit 4207ce958a on Mar 22, 2019
  60. ryanofsky referenced this in commit 20edb8c1a8 on Mar 22, 2019
  61. ryanofsky referenced this in commit 1a7b5e9113 on Mar 22, 2019
  62. ryanofsky referenced this in commit 36fa02eb3e on Mar 22, 2019
  63. ryanofsky referenced this in commit c5b3a3f675 on Mar 22, 2019
  64. ryanofsky referenced this in commit feaa53717b on Mar 22, 2019
  65. ryanofsky referenced this in commit cb2a6bf3b5 on Mar 22, 2019
  66. ryanofsky referenced this in commit 89061c8a41 on Mar 22, 2019
  67. ryanofsky referenced this in commit 2b6991132c on Mar 22, 2019
  68. ryanofsky referenced this in commit 1446751ad1 on Mar 22, 2019
  69. ryanofsky referenced this in commit b35ba2c161 on Mar 22, 2019
  70. ryanofsky referenced this in commit 7cae0d67b4 on Mar 22, 2019
  71. ryanofsky referenced this in commit cdc621ad1b on Mar 22, 2019
  72. ryanofsky referenced this in commit 1352a8e9fd on Mar 22, 2019
  73. ryanofsky referenced this in commit 2d0a984237 on Mar 22, 2019
  74. promag commented at 8:52 AM on March 22, 2019: member

    @instagibbs right, double paid, not double spent.

  75. instagibbs force-pushed on Mar 22, 2019
  76. instagibbs commented at 1:49 PM on March 22, 2019: member

    @jnewbery Comments addressed, and paragraphed poached if that's ok :)

  77. instagibbs commented at 1:50 PM on March 22, 2019: member

    Also added a small note on the need for confirmed new inputs as not a bip125 requirement, but of current mempool relay policy.

  78. jnewbery commented at 2:10 PM on March 22, 2019: member

    Thanks @instagibbs . Looks good.

    Also added a small note on the need for confirmed new inputs as not a bip125 requirement, but of current mempool relay policy.

    Maybe I'm misunderstanding this comment, but I think this is a BIP 125 requirement (rule 2).

  79. instagibbs commented at 2:14 PM on March 22, 2019: member

    @jnewbery Oh I'm misremembering: It's not an RBF rule, just bip125!

  80. instagibbs force-pushed on Mar 22, 2019
  81. instagibbs commented at 2:15 PM on March 22, 2019: member

    @jnewbery fixed comment

  82. ryanofsky approved
  83. ryanofsky commented at 4:14 PM on March 22, 2019: member

    utACK 8e9027bd9864c8a2c0be314bd1f799b9ed8c4085. Changes since last review: new comments and test changes to check for missing inputs

  84. Empact referenced this in commit d59868f135 on Mar 25, 2019
  85. Empact referenced this in commit e329ad3cfe on Mar 25, 2019
  86. Empact referenced this in commit 4356711361 on Mar 25, 2019
  87. Empact referenced this in commit c1968914ef on Mar 25, 2019
  88. in src/wallet/feebumper.cpp:77 in 8e9027bd98 outdated 
      72 | @@ -73,7 +73,7 @@ bool TransactionCanBeBumped(const CWallet* wallet, const uint256& txid)
  73 |      return res == feebumper::Result::OK;
  74 |  }
  75 |  
  76 | -Result CreateTransaction(const CWallet* wallet, const uint256& txid, const CCoinControl& coin_control, CAmount total_fee, std::vector<std::string>& errors,
  77 | +Result CreateTotalBumpTransaction(const CWallet* wallet, const uint256& txid, const CCoinControl& coin_control, CAmount total_fee, std::vector<std::string>& errors,
  78 |                           CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx)
    jnewbery commented at 4:42 PM on April 2, 2019:

    nit: increase indentation to match movement of open-parens in line above.

  89. in src/wallet/feebumper.cpp:149 in 8e9027bd98 outdated 
     142 | -                                                                FormatMoney(requiredFee)));
 143 | -            return Result::INVALID_PARAMETER;
 144 | -        }
 145 | -        new_fee = total_fee;
 146 | -        nNewFeeRate = CFeeRate(total_fee, maxNewTxSize);
 147 | -    } else {
    jnewbery commented at 4:48 PM on April 2, 2019:

    See the error text below: the totalFee value should be at least %s or the settxfee value should be at least %s to add transaction. Please remove the reference to settxfee since it's no longer used in this function (it was used in the call to GetMinimumFee().

  90. in src/wallet/feebumper.cpp:145 in 8e9027bd98 outdated 
     168 | +        errors.push_back(strprintf("Insufficient totalFee (cannot be less than required fee %s)",
 169 | +            FormatMoney(requiredFee)));
 170 | +        return Result::INVALID_PARAMETER;
 171 |      }
 172 | +    new_fee = total_fee;
 173 | +    nNewFeeRate = CFeeRate(total_fee, maxNewTxSize);
    jnewbery commented at 4:51 PM on April 2, 2019:

    No need anymore to declare nNewFeeRate at line 122 above. You can move the declaration and assignment down to below the // check that fee rate is higher than mempool's ... comment below.

  91. in src/wallet/feebumper.cpp:144 in 8e9027bd98 outdated 
     167 | +    if (total_fee < requiredFee) {
 168 | +        errors.push_back(strprintf("Insufficient totalFee (cannot be less than required fee %s)",
 169 | +            FormatMoney(requiredFee)));
 170 | +        return Result::INVALID_PARAMETER;
 171 |      }
 172 | +    new_fee = total_fee;
    jnewbery commented at 4:58 PM on April 2, 2019:

    This could potentially be moved to the top of the function.

  92. in src/wallet/feebumper.cpp:203 in 8e9027bd98 outdated 
     194 | @@ -210,6 +195,110 @@ Result CreateTransaction(const CWallet* wallet, const uint256& txid, const CCoin
 195 |          }
 196 |      }
 197 |  
 198 | +    return Result::OK;
 199 | +}
 200 | +
 201 | +
 202 | +Result CreateRateBumpTransaction(CWallet* wallet, const uint256& txid, const CCoinControl& coin_control, std::vector<std::string>& errors,
 203 | +                         CAmount& old_fee, CAmount& new_fee, CMutableTransaction& mtx)
    jnewbery commented at 4:59 PM on April 2, 2019:

    nit: please align with open-parens.

    instagibbs commented at 1:49 PM on April 3, 2019:

    is there a style guide entry for this? I don't really know the convention.

    jnewbery commented at 2:03 PM on April 3, 2019:

    No style guide entry I don't think. I just saw that the old function had aligned arguments, and when you copied it and changed the length of the function name it unaligned it.

  93. in src/wallet/feebumper.cpp:242 in 8e9027bd98 outdated 
     237 | +    // return old fee first
 238 | +    old_fee = wtx.GetDebit(ISMINE_SPENDABLE) - wtx.tx->GetValueOut();
 239 | +    int64_t txSize = GetVirtualTransactionSize(*(wtx.tx));
 240 | +    // Feerate of thing we are bumping
 241 | +    CFeeRate old_feerate(old_fee, txSize);
 242 | +    CFeeRate nodeIncrementalRelayFee = wallet->chain().relayIncrementalFee();
    jnewbery commented at 5:04 PM on April 2, 2019:

    I think this line should go below the comment below, since it's logically linked to the relay fee, not the feerate of the tx.

  94. in src/wallet/feebumper.cpp:259 in 8e9027bd98 outdated 
     254 | +    CFeeRate min_new_feerate(old_feerate.GetFeePerK() + 1 + walletIncrementalRelayFee.GetFeePerK());
 255 | +
 256 | +    // Feerate calculated by GetMinimumFeeRate may not actually hit bip125 requirements
 257 | +    // so pick the larger of the two
 258 | +    FeeCalculation fee_calc;
 259 | +    CFeeRate block_feerate(GetMinimumFeeRate(*wallet, new_coin_control, &fee_calc));
    jnewbery commented at 5:09 PM on April 2, 2019:

    Any reason for changing this from using nullptr as the feeCalc argument? Instantiating the unused variable fee_calc seems unnecessary and confusing.

  95. in src/wallet/wallet.cpp:2739 in 8e9027bd98 outdated 
    2814 | @@ -2815,7 +2815,7 @@ bool CWallet::CreateTransaction(interfaces::Chain::Lock& locked_chain, const std
2815 |          LOCK(cs_wallet);
2816 |          {
2817 |              std::vector<COutput> vAvailableCoins;
2818 | -            AvailableCoins(*locked_chain, vAvailableCoins, true, &coin_control);
2819 | +            AvailableCoins(*locked_chain, vAvailableCoins, true, &coin_control, 1, MAX_MONEY, MAX_MONEY, 0, coin_control.m_min_depth);
    jnewbery commented at 5:18 PM on April 2, 2019:

    AvailableCoins() could be refactored to not take an nMinDepth argument now that coin control has a m_min_depth member. Could be done in a follow-up PR.

    instagibbs commented at 1:49 PM on April 3, 2019:

    as a larger discussion(for another time), what should be in CCoinControl and what shouldn't be? It's nice to have a compact struct that describes the intent of each payment, including all known constraints by the caller.

  96. jnewbery commented at 5:22 PM on April 2, 2019: member

    I've only reviewed the bitcoind code, not the test code. A few comments inline.

  97. instagibbs force-pushed on Apr 3, 2019
  98. instagibbs commented at 1:53 PM on April 3, 2019: member

    @jnewbery comments addressed

  99. instagibbs force-pushed on Apr 3, 2019
  100. in src/wallet/feebumper.cpp:279 in e9f1a9f71b outdated 
     275 | +
 276 | +    // We cannot source new unconfirmed inputs(bip125 rule 2)
 277 | +    new_coin_control.m_min_depth = 1;
 278 | +
 279 | +    CTransactionRef tx_new = MakeTransactionRef();
 280 | +    CReserveKey reservekey(wallet);
    jnewbery commented at 10:25 PM on April 3, 2019:

    I haven't been able to dig into this fully, but do you need to call CReserveKey::KeepKey() on this reserve key to remove it from the key pool after using it?

    instagibbs commented at 6:41 PM on April 4, 2019:

    good catch, fixed.

    ryanofsky commented at 3:35 PM on April 11, 2019:

    #15557 (review)

    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.

    instagibbs commented at 4:07 PM on April 11, 2019:

    I think this would happen if you bumped the transaction, created a new change output, and then make another change output before the next block is seen. Otherwise the keypool-scanning we do would have taken care of it as soon as it's seen in a block.

    A more conservative change that doesn't involve asserting would be to replace the ReturnKey call in the destructor to KeepKey, the idea being it's ok to burn a couple keys accidentally rather than repeat a script.

    jnewbery commented at 4:38 PM on April 11, 2019:

    The worst possible outcome would have been reusing a keypool address for change in multiple transactions. It's a marginal privacy leak but not disastrous.

    The whole CReserveKey GetReservedKey()-ReturnKey()-KeepKey() cycle seems pretty brittle to me, and I think should be refactored. I haven't figured out exactly how yet, but I have a PR here: #15777 that documents how it currently works.

  101. instagibbs force-pushed on Apr 4, 2019
  102. in src/wallet/feebumper.cpp:235 in 5164305655 outdated 
     230 | +            ExtractDestination(output.scriptPubKey, change_dest);
 231 | +            new_coin_control.destChange = change_dest;
 232 | +        }
 233 | +    }
 234 | +
 235 | +    // Old transaction stuff
    jnewbery commented at 6:26 PM on April 8, 2019:

    I know this code is mostly inherited from the existing feebumper::CreateTransaction(), but I think it can be made a lot more readable. CFeeRate overrides the < operator (so std::max can be used) and the += operator. Do you think the following code is clearer:

        // Get the fee rate of the original transaction. This is calculated from
    // the tx fee/vsize, so it may have been rounded down. Add 1 satoshi to the
    // result.
    old_fee = wtx.GetDebit(ISMINE_SPENDABLE) - wtx.tx->GetValueOut();
    int64_t txSize = GetVirtualTransactionSize(*(wtx.tx));
    CFeeRate feerate(old_fee, txSize);
    feerate += CFeeRate(1);

    // The node has a configurable incremental relay fee. Increment the fee by
    // the minimum of that and the wallet's conservative
    // WALLET_INCREMENTAL_RELAY_FEE value to future proof against changes to
    // network wide policy for incremental relay fee that our node may not be
    // aware of. This ensures we're over the over the required relay fee rate
    // (BIP 125 rule 4).  The replacement tx will be at least as large as the
    // original tx, so the total fee will be greater (BIP 125 rule 3)
    CFeeRate node_incremental_relay_fee = wallet->chain().relayIncrementalFee();
    CFeeRate wallet_incremental_relay_fee = CFeeRate(WALLET_INCREMENTAL_RELAY_FEE);
    feerate += std::max(node_incremental_relay_fee, wallet_incremental_relay_fee);

    // Fee rate must also be at least the wallet's GetMinimumFeeRate
    CFeeRate min_feerate(GetMinimumFeeRate(*wallet, new_coin_control, /* feeCalc */ nullptr));

    // Set the required fee rate for the replacement transaction in coin control.
    new_coin_control.m_feerate = std::max(feerate, min_feerate);

    (I also re-added the comment about adding 1 to the fee rate in case the calculated old fee rate was rounded down).

    instagibbs commented at 7:29 PM on April 10, 2019:

    good suggestions, will use

  103. in src/wallet/feebumper.cpp:222 in 5164305655 outdated 
     217 | +    Result result = PreconditionChecks(*locked_chain, wallet, wtx, errors);
 218 | +    if (result != Result::OK) {
 219 | +        return result;
 220 | +    }
 221 | +
 222 | +    // Fill in recipients(and preserve a change key)
    jnewbery commented at 6:26 PM on April 8, 2019:

    This could be slightly more accurate if it said "preserve a change destination if there is one"

    instagibbs commented at 7:29 PM on April 10, 2019:

    sure

  104. in test/functional/wallet_bumpfee.py:184 in 5164305655 outdated 
     176 | @@ -173,6 +177,55 @@ def test_small_output_fails(rbf_node, dest_address):
 177 |      rbfid = spend_one_input(rbf_node, dest_address)
 178 |      assert_raises_rpc_error(-4, "Change output is too small", rbf_node.bumpfee, rbfid, {"totalFee": 50001})
 179 |  
 180 | +def test_no_more_inputs_fails(rbf_node, dest_address):
 181 | +    # feerate rbf requires confirmed outputs when change output doesn't exist or is insufficient
 182 | +    rbf_node.generatetoaddress(1, dest_address)
 183 | +    # spend all funds, no change output
 184 | +    rbfid = rbf_node.sendtoaddress(rbf_node.getnewaddress(), rbf_node.getbalance(), "", "", True)
    jnewbery commented at 6:41 PM on April 8, 2019:

    nit: prefer using named arguments when using RPCs:

    • makes it clear what the argument is (with positional arguments I have to guess what True refers to here)
    • you don't have to pass the empty strings for the unused arguments.
  105. in test/functional/wallet_bumpfee.py:180 in 5164305655 outdated 
     176 | @@ -173,6 +177,55 @@ def test_small_output_fails(rbf_node, dest_address):
 177 |      rbfid = spend_one_input(rbf_node, dest_address)
 178 |      assert_raises_rpc_error(-4, "Change output is too small", rbf_node.bumpfee, rbfid, {"totalFee": 50001})
 179 |  
 180 | +def test_no_more_inputs_fails(rbf_node, dest_address):
    jnewbery commented at 6:43 PM on April 8, 2019:

    micronit: prefer defining these subtest methods in the order they're called in the run_test() method.

    instagibbs commented at 7:30 PM on April 10, 2019:

    done

  106. in test/functional/wallet_bumpfee.py:206 in 5164305655 outdated 
     201 | +    # Keep bumping until we out-spend change output
 202 | +    orig_fee = 0
 203 | +    while orig_fee < Decimal("0.0005"):
 204 | +        new_input_set = input_set(rbf_node.getrawtransaction(rbfid))
 205 | +        new_item = list(new_input_set)[0]
 206 | +        assert_equal(len(original_input_set), 1)
    jnewbery commented at 6:52 PM on April 8, 2019:

    Redundant line. original_input_set doesn't change in the loop and has already been tested.

    instagibbs commented at 7:31 PM on April 10, 2019:

    not immediately apparent to the reader(me) imo. leaving this in.

  107. in test/functional/wallet_bumpfee.py:212 in 5164305655 outdated 
     207 | +        assert_equal(original_item.hash, new_item.hash)
 208 | +        assert_equal(original_item.n, new_item.n)
 209 | +        rbfid_new_det = rbf_node.bumpfee(rbfid)
 210 | +        rbfid_new = rbfid_new_det["txid"]
 211 | +        raw_pool = rbf_node.getrawmempool()
 212 | +        assert(rbfid not in raw_pool)
    jnewbery commented at 6:53 PM on April 8, 2019:

    assert is not a function. Remove parens

  108. in test/functional/wallet_bumpfee.py:209 in 5164305655 outdated 
     204 | +        new_input_set = input_set(rbf_node.getrawtransaction(rbfid))
 205 | +        new_item = list(new_input_set)[0]
 206 | +        assert_equal(len(original_input_set), 1)
 207 | +        assert_equal(original_item.hash, new_item.hash)
 208 | +        assert_equal(original_item.n, new_item.n)
 209 | +        rbfid_new_det = rbf_node.bumpfee(rbfid)
    jnewbery commented at 6:53 PM on April 8, 2019:

    What does det mean here?

    instagibbs commented at 7:20 PM on April 10, 2019:

    details :) ill expand

  109. in test/functional/wallet_bumpfee.py:189 in 5164305655 outdated 
     184 | +    rbfid = rbf_node.sendtoaddress(rbf_node.getnewaddress(), rbf_node.getbalance(), "", "", True)
 185 | +    assert_raises_rpc_error(-4, "Unable to create transaction: Insufficient funds", rbf_node.bumpfee, rbfid)
 186 | +
 187 | +def test_small_output_with_feerate_succeeds(rbf_node, dest_address):
 188 | +    def input_set(tx):
 189 | +        tx_struct = FromHex(CTransaction(), tx)
    jnewbery commented at 6:56 PM on April 8, 2019:

    Why not use the verbose output of getrawtransaction and get the inputs directly instead of deserializing the hex tx?

    instagibbs commented at 7:37 PM on April 10, 2019:

    I think I had a reason but can't recall, will adapt

  110. in test/functional/wallet_bumpfee.py:202 in 5164305655 outdated 
     197 | +    rbfid = spend_one_input(rbf_node, dest_address)
 198 | +    original_input_set = input_set(rbf_node.getrawtransaction(rbfid))
 199 | +    assert_equal(len(original_input_set), 1)
 200 | +    original_item = list(original_input_set)[0]
 201 | +    # Keep bumping until we out-spend change output
 202 | +    orig_fee = 0
    jnewbery commented at 6:57 PM on April 8, 2019:

    nit: This variable gets updated, so I think tx_fee would be a more appropriate name.

    instagibbs commented at 7:37 PM on April 10, 2019:

    sure

  111. in test/functional/wallet_bumpfee.py:200 in 5164305655 outdated 
     195 | +    # Make sure additional inputs exist
 196 | +    rbf_node.generatetoaddress(101, rbf_node.getnewaddress())
 197 | +    rbfid = spend_one_input(rbf_node, dest_address)
 198 | +    original_input_set = input_set(rbf_node.getrawtransaction(rbfid))
 199 | +    assert_equal(len(original_input_set), 1)
 200 | +    original_item = list(original_input_set)[0]
    jnewbery commented at 6:57 PM on April 8, 2019:

    nit: item is vague here. Why not txin?

  112. in test/functional/wallet_bumpfee.py:190 in 5164305655 outdated 
     185 | +    assert_raises_rpc_error(-4, "Unable to create transaction: Insufficient funds", rbf_node.bumpfee, rbfid)
 186 | +
 187 | +def test_small_output_with_feerate_succeeds(rbf_node, dest_address):
 188 | +    def input_set(tx):
 189 | +        tx_struct = FromHex(CTransaction(), tx)
 190 | +        input_set = set()
    jnewbery commented at 7:04 PM on April 8, 2019:

    Why a set rather than a list?

  113. in test/functional/wallet_bumpfee.py:221 in 5164305655 outdated 
     216 | +
 217 | +    # input(s) have been added
 218 | +    final_input_set = input_set(rbf_node.getrawtransaction(rbfid))
 219 | +    assert_greater_than(len(final_input_set), 1)
 220 | +    # Original input is in final set
 221 | +    found_original = False
    jnewbery commented at 7:08 PM on April 8, 2019:

    More concise:

        assert [txin for txin in final_input_set
            if txin.hash == original_item.hash
            and txin.n == original_item.n]
  114. in test/functional/wallet_bumpfee.py:77 in 5164305655 outdated 
      72 | @@ -73,6 +73,10 @@ def run_test(self):
  73 |          test_unconfirmed_not_spendable(rbf_node, rbf_node_address)
  74 |          test_bumpfee_metadata(rbf_node, dest_address)
  75 |          test_locked_wallet_fails(rbf_node, dest_address)
  76 | +        test_change_script_match(rbf_node, dest_address)
  77 | +        # These tests wipe out a number of utxos that are expected in other tests
    jnewbery commented at 8:19 PM on April 8, 2019:

    This is a shame. At the moment I think the subtests can be run in any order. It'd be nice to keep that property (so, for example, anyone adding new subtests doesn't need to worry about state carried over from previous tests). Is it possible to rewrite your subtests in such a way to only use the existing outputs and not generate new blocks? For the test_no_more_inputs_fails() test you could lock all outputs and then unlock them, for example.

  115. in test/functional/wallet_bumpfee.py:315 in 5164305655 outdated 
     324 | @@ -272,19 +325,43 @@ def test_locked_wallet_fails(rbf_node, dest_address):
 325 |      rbf_node.walletlock()
 326 |      assert_raises_rpc_error(-13, "Please enter the wallet passphrase with walletpassphrase first.",
 327 |                              rbf_node.bumpfee, rbfid)
 328 | +    rbf_node.walletpassphrase(WALLET_PASSPHRASE, WALLET_PASSPHRASE_TIMEOUT)
 329 |  
 330 | -
 331 | -def spend_one_input(node, dest_address):
 332 | +def test_change_script_match(rbf_node, dest_address):
    jnewbery commented at 8:45 PM on April 8, 2019:

    I think this could do with a doc string explaining what this subtest is testing.

  116. in test/functional/wallet_bumpfee.py:331 in 5164305655 outdated 
     324 | @@ -272,19 +325,43 @@ def test_locked_wallet_fails(rbf_node, dest_address):
 325 |      rbf_node.walletlock()
 326 |      assert_raises_rpc_error(-13, "Please enter the wallet passphrase with walletpassphrase first.",
 327 |                              rbf_node.bumpfee, rbfid)
 328 | +    rbf_node.walletpassphrase(WALLET_PASSPHRASE, WALLET_PASSPHRASE_TIMEOUT)
 329 |  
 330 | -
 331 | -def spend_one_input(node, dest_address):
 332 | +def test_change_script_match(rbf_node, dest_address):
 333 | +    rbfid = spend_one_input(rbf_node, dest_address)
    jnewbery commented at 8:46 PM on April 8, 2019:

    A bit more concise:

    def test_change_script_match(rbf_node, dest_address):
    """Test that the same change addresses is used for the replacement transaction when possible."""
    def get_change_address(tx):
        tx_details = rbf_node.getrawtransaction(tx, 1)
        txout_addresses = [txout['scriptPubKey']['addresses'][0] for txout in tx_details["vout"]]
        return [address for address in txout_addresses if rbf_node.getaddressinfo(address)["ischange"]]

    # Check that there is only one change output
    rbfid = spend_one_input(rbf_node, dest_address)
    change_addresses = get_change_address(rbfid)
    assert_equal(len(change_addresses), 1)

    # Now find that address in each subsequent tx, and no other change
    bumped_total_tx = rbf_node.bumpfee(rbfid, {"totalFee": 2000})
    assert_equal(change_addresses, get_change_address(bumped_total_tx['txid']))
    bumped_rate_tx = rbf_node.bumpfee(bumped_total_tx["txid"])
    assert_equal(change_addresses, get_change_address(bumped_rate_tx['txid']))
  117. jnewbery commented at 8:46 PM on April 8, 2019: member

    Looks good. I left some nit-tastic comments.

  118. jnewbery commented at 8:50 PM on April 8, 2019: member

    I left a bunch of nits, mostly on Python code style, which you should feel free to ignore.

    I think functionally this is fine. My main concern is the introduction of a lot of duplicate code between CreateTotalBumpTransaction() and CreateRateBumpTransaction(), but I don't think that should prevent merge.

  119. ryanofsky referenced this in commit f839b1355f on Apr 8, 2019
  120. ryanofsky referenced this in commit 155f318f2f on Apr 8, 2019
  121. ryanofsky referenced this in commit 756f7440e7 on Apr 10, 2019
  122. ryanofsky referenced this in commit ca0ec7f044 on Apr 10, 2019
  123. ryanofsky referenced this in commit b874747b51 on Apr 10, 2019
  124. ryanofsky referenced this in commit 78a2fb55c9 on Apr 10, 2019
  125. instagibbs commented at 7:22 PM on April 10, 2019: member

    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.

  126. instagibbs force-pushed on Apr 10, 2019
  127. instagibbs commented at 8:01 PM on April 10, 2019: member

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

  128. in test/functional/wallet_bumpfee.py:20 in b6dcb6a214 outdated 
      16 | @@ -17,7 +17,7 @@
  17 |  import io
  18 |  
  19 |  from test_framework.blocktools import add_witness_commitment, create_block, create_coinbase, send_to_witness
  20 | -from test_framework.messages import BIP125_SEQUENCE_NUMBER, CTransaction
  21 | +from test_framework.messages import BIP125_SEQUENCE_NUMBER, CTransaction, FromHex
    jnewbery commented at 10:07 PM on April 10, 2019:

    Remove this new import of FromHex to satisfy the linter.

    instagibbs commented at 10:25 PM on April 10, 2019:

    fixed.

  129. jnewbery commented at 10:10 PM on April 10, 2019: member

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

    utACK b6dcb6a2141160c2c6ca015478cc74c9d803ed83

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

  130. instagibbs force-pushed on Apr 10, 2019
  131. instagibbs commented at 10:26 PM on April 10, 2019: member

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

  132. DrahtBot added the label Needs rebase on Apr 11, 2019
  133. generalize bumpfee to add inputs when needed 0ea47ba7b3
  134. add functional tests for feerate bumpfee with adding inputs d08becff85
  135. wallet_bumpfee.py: add test for change key preservation 184f8785f7
  136. instagibbs force-pushed on Apr 11, 2019
  137. DrahtBot removed the label Needs rebase on Apr 11, 2019
  138. jnewbery commented at 2:31 PM on April 11, 2019: member

    utACK 184f8785f710d58d9ef82e611591c9cbff5ab89d @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.

  139. promag commented at 2:58 PM on April 11, 2019: member

    Someone can reply to this #15557 (review)?

  140. instagibbs commented at 3:37 PM on April 11, 2019: member

    If you intentionally send to a raw change address it may but that's not really an intended work flow.

    On Thu, Apr 11, 2019, 11:30 AM John Newbery notifications@github.com wrote:

    @jnewbery commented on this pull request.

    In src/wallet/feebumper.cpp https://github.com/bitcoin/bitcoin/pull/15557#discussion_r274483048:

    • Result result = PreconditionChecks(*locked_chain, wallet, wtx, errors);
    • if (result != Result::OK) {
    •    return result;
    • }
    • // Fill in recipients(and preserve a change key)
    • std::vector<CRecipient> recipients;
    • for (const auto& output : wtx.tx->vout) {
    •    if (!wallet->IsChange(output)) {
    •        CRecipient recipient = {output.scriptPubKey, output.nValue, false};
    •        recipients.push_back(recipient);
    •    } else {
    •        CTxDestination change_dest;
    •        ExtractDestination(output.scriptPubKey, change_dest);
    •        new_coin_control.destChange = change_dest;

    I think it's acceptable. I don't believe that Bitcoin Core wallet will produce transactions with more than one change output.

    — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/bitcoin/bitcoin/pull/15557#discussion_r274483048, or mute the thread https://github.com/notifications/unsubscribe-auth/AFgC01LBorTrP3srT4XrUdUuXq0FVtekks5vf1UagaJpZM4bjzhN .

  141. ryanofsky approved
  142. ryanofsky commented at 3:46 PM on April 11, 2019: member

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

  143. promag commented at 2:17 PM on April 14, 2019: member

    utACK 01c1cb8.

  144. meshcollider merged this on Apr 14, 2019
  145. meshcollider closed this on Apr 14, 2019
  146. meshcollider referenced this in commit 4f4ef3138b on Apr 14, 2019
  147. meshcollider removed this from the "Blockers" column in a project
  148. MarcoFalke referenced this in commit 65526fc866 on May 14, 2019
  149. sidhujag referenced this in commit ef84484a6a on May 15, 2019
  150. meshcollider referenced this in commit 459baa1756 on Jul 17, 2019
  151. sidhujag referenced this in commit 1c2372623e on Jul 29, 2019
  152. HashUnlimited referenced this in commit f5845d2db7 on Aug 30, 2019
  153. HashUnlimited referenced this in commit cb7e65c19f on Aug 30, 2019
  154. barrystyle referenced this in commit 02edf5acf3 on Nov 11, 2019
  155. barrystyle referenced this in commit d9cb43f622 on Nov 11, 2019
  156. deadalnix referenced this in commit 103cee9a7f on May 27, 2020
  157. deadalnix referenced this in commit a7a55fb3ac on May 27, 2020
  158. deadalnix referenced this in commit 8790173c76 on Jul 1, 2020
  159. monstrobishi referenced this in commit 56f5cbbba2 on Sep 6, 2020
  160. Munkybooty referenced this in commit e7e1fefdf7 on Oct 16, 2021
  161. Munkybooty referenced this in commit 6e9649fc17 on Dec 21, 2021
  162. Munkybooty referenced this in commit 2c1eb4480b on Dec 21, 2021
  163. Munkybooty referenced this in commit d01f373563 on Dec 23, 2021
  164. Munkybooty referenced this in commit 57ea4e5826 on Dec 24, 2021
  165. Munkybooty referenced this in commit 7975686b0e on Dec 27, 2021
  166. vijaydasmp referenced this in commit 65e9a868f1 on Dec 28, 2021
  167. vijaydasmp referenced this in commit 4f356f98e1 on Dec 28, 2021
  168. Munkybooty referenced this in commit 95618246cc on Feb 6, 2022
  169. DrahtBot locked this on Feb 15, 2022
Contributors
instagibbspromagDrahtBotSjorspracticalswiftstevenrooseryanofskyjnewbery
Labels
TX fees and policy
Linked (view graph)
#10390 [wallet] remove minimum total fee option#15639 bitcoin-wallet tool: Drop libbitcoin_server.a dependency#15656 wallet: Keep all outputs in bumpfee#15777 [docs] Add doxygen comments for keypool classes#15796 CReserveKey should not allow passive key re-use, debug assert in destructor#15816 scripts: GitHub merge script is missing review comments#16208 wallet: Consume ReserveDestination on successful CreateTransaction
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-30 12:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me