rpc: Bumpfee units change, satoshis to BTC #15157

benthecarman commented at 11:39 PM on January 12, 2019: contributor

Fixes #14815

fanquake added the label Wallet on Jan 12, 2019

fanquake added the label RPC/REST/ZMQ on Jan 12, 2019

benthecarman force-pushed on Jan 13, 2019

DrahtBot commented at 1:01 AM on January 13, 2019: contributor

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)
#15557 (Enhance bumpfee to include inputs when targeting a feerate by instagibbs)
#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.

promag commented at 2:26 AM on January 13, 2019: member

Current implementation is breaking change. You should support the old option under the depreciation condition. It also needs release notes.

gmaxwell commented at 8:47 AM on January 13, 2019: contributor

Operating on fee instead of feerate seems wrong to me in any case-- in particular bumping might change the transaction size in rather arbitrary ways.

in test/functional/wallet_bumpfee.py:218 in c7a64da0ca outdated

 217 |  
 218 |  def test_rebumping_not_replaceable(rbf_node, dest_address):
 219 |      # check that re-bumping a non-replaceable bump tx fails
 220 |      rbfid = spend_one_input(rbf_node, dest_address)
 221 | -    bumped = rbf_node.bumpfee(rbfid, {"totalFee": 10000, "replaceable": False})
 222 | +    bumped = rbf_node.bumpfee(rbfid, {"total_fee": 0.0001 , "replaceable": False})  # 10000 sats

practicalswift commented at 1:34 PM on January 14, 2019:

Drop space before comment :-)

DrahtBot added the label Needs rebase on Jan 14, 2019

laanwj commented at 5:41 PM on January 16, 2019: member

Current implementation is breaking change. You should support the old option under the depreciation condition. It also needs release notes.

Concept ACK, but yes.

in src/wallet/rpcwallet.cpp:3184 in c7a64da0ca outdated

3181 |                      {"txid", RPCArg::Type::STR_HEX, /* opt */ false, /* default_val */ "", "The txid to be bumped"},
3182 |                      {"options", RPCArg::Type::OBJ, /* opt */ true, /* default_val */ "null", "",
3183 |                          {
3184 |                              {"confTarget", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "fallback to wallet's default", "Confirmation target (in blocks)"},
3185 | -                            {"totalFee", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "fallback to 'confTarget'", "Total fee (NOT feerate) to pay, in satoshis.\n"
3186 | +                            {"total_fee", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "fallback to 'confTarget'", "Total fee (NOT feerate) to pay, in bitcoins.\n"

maflcko commented at 6:39 PM on January 16, 2019:

Type should be AMOUNT and not NUM

benthecarman force-pushed on Jan 17, 2019

DrahtBot removed the label Needs rebase on Jan 17, 2019

fanquake added the label Needs rebase on Jan 29, 2019

benthecarman force-pushed on Feb 4, 2019

benthecarman commented at 12:58 PM on February 4, 2019: contributor

Rebased and fixed release notes

DrahtBot removed the label Needs rebase on Feb 4, 2019

practicalswift commented at 1:44 PM on February 4, 2019: contributor

How can we make sure that an end-user who doesn't pay attention to the unit change isn't hurt financially?

benthecarman commented at 3:38 PM on February 5, 2019: contributor

How can we make sure that an end-user who doesn't pay attention to the unit change isn't hurt financially?

I could add a sanity check to make sure the fee isn't over 1

benthecarman force-pushed on Feb 7, 2019

benthecarman commented at 7:02 PM on February 7, 2019: contributor

Sanity check added

in src/wallet/rpcwallet.cpp:3325 in 8d3d737647 outdated

3339 | +            } else if (options.exists("confTarget")) { // TODO: alias this to conf_target
3340 | +                coin_control.m_confirm_target = ParseConfirmTarget(options["confTarget"]);
3341 | +            } else if (options.exists("total_fee")) {
3342 | +                total_fee = AmountFromValue(options["total_fee"]); // Convert to satoshis
3343 | +                if (total_fee >= 100000000) { // User entered satoshis
3344 | +                    throw JSONRPCError(RPC_INVALID_PARAMETER, "Invalid total_fee, enter a value in bitcoins.");

practicalswift commented at 9:37 PM on February 7, 2019:

The error should include why the total_fee is invalid (as done below).

Also this error condition should be tested :-)

benthecarman commented at 10:14 PM on February 7, 2019:

The error should include why the total_fee is invalid (as done below)

Do you mean saying something like total_fee can't be >= 1?

luke-jr commented at 10:43 PM on February 7, 2019:

Wouldn't this only trigger if the user intended to pay a >=1 BTC fee?

benthecarman force-pushed on Feb 8, 2019

benthecarman commented at 12:49 AM on February 8, 2019: contributor

Updated error message and added test for sanity check

luke-jr commented at 2:37 AM on February 23, 2019: member

Does this really need to duplicate all the code?

benthecarman commented at 3:09 AM on February 23, 2019: contributor

Does this really need to duplicate all the code?

It doesn't, but I did that so it'd be easier to remove the depreciated version in the future.

luke-jr commented at 3:16 AM on February 23, 2019: member

I don't think it makes it any easier, just harder to review the PR... :/

benthecarman commented at 3:18 AM on February 23, 2019: contributor

Alright I'll clean it up then

benthecarman force-pushed on Feb 24, 2019

benthecarman commented at 9:35 AM on February 24, 2019: contributor

Should be ready for a re-review

DrahtBot added the label Needs rebase on Mar 4, 2019

benthecarman force-pushed on Mar 4, 2019

DrahtBot removed the label Needs rebase on Mar 4, 2019

DrahtBot added the label Needs rebase on Mar 21, 2019

benthecarman force-pushed on Mar 22, 2019

DrahtBot removed the label Needs rebase on Mar 22, 2019

rpc: Bumpfee units change, satoshis to BTC 5087ab3bea

in src/wallet/rpcwallet.cpp:3290 in 165bc95bae outdated

3294 | -        } else if (options.exists("totalFee")) {
3295 | -            totalFee = options["totalFee"].get_int64();
3296 | -            if (totalFee <= 0) {
3297 | -                throw JSONRPCError(RPC_INVALID_PARAMETER, strprintf("Invalid totalFee %s (must be greater than 0)", FormatMoney(totalFee)));
3298 | +        } else if (options.exists("total_fee") && !IsDeprecatedRPCEnabled("bumpfee")) {
3299 | +            total_fee = AmountFromValue(options["total_fee"]); // Convert to satohis

practicalswift commented at 7:58 PM on April 4, 2019:

Should be "satoshis" :-)

benthecarman force-pushed on Apr 4, 2019

DrahtBot added the label Needs rebase on Apr 15, 2019

DrahtBot commented at 2:11 PM on April 15, 2019: contributor

Needs rebase

luke-jr commented at 6:19 AM on April 17, 2019: member

It should probably remain possible to use the new API even when the deprecated one is enabled...

benthecarman commented at 8:52 PM on May 16, 2019: contributor

Going to close this issue, seems the totalFee argument is being deprecated in #15996

benthecarman closed this on May 16, 2019

laanwj removed the label Needs rebase on Oct 24, 2019

bitcoin locked this on Dec 16, 2021

benthecarman deleted the branch on Feb 17, 2024