wallet: Fix origfee return for bumpfee with feerate arg #17643

instagibbs · 2019-12-01T14:48:00Z

fixes #17642 and adds a simple test that would have caught it

DrahtBot · 2019-12-01T17:42:16Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#17566 (Switch to weight units for all feerates computation by darosior)
#16373 (bumpfee: Return PSBT when wallet has privkeys disabled by instagibbs)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

darosior

ACK a0655ad

darosior · 2019-12-01T21:30:46Z

src/wallet/feebumper.cpp

@@ -108,12 +108,11 @@ static feebumper::Result CheckFeeRate(const CWallet& wallet, const CWalletTx& wt
    return feebumper::Result::OK;
 }

-static CFeeRate EstimateFeeRate(const CWallet& wallet, const CWalletTx& wtx, CCoinControl& coin_control, CAmount& old_fee)
+static CFeeRate EstimateFeeRate(const CWallet& wallet, const CWalletTx& wtx, CCoinControl& coin_control, CAmount old_fee)


Could be const, I think ?

done, and re-ordered the arguments to be all const then non-const.

practicalswift · 2019-12-01T21:50:58Z

@instagibbs

Thanks for tackling this quickly.

I'm not too familiar with this part of the code: is there any scenario where the wrong fee could have been selected due to the uninitialized read? What is the likely user impact?

instagibbs · 2019-12-01T22:57:38Z

No it's simply a reporting issue.

…

On Sun, Dec 1, 2019, 4:51 PM practicalswift ***@***.***> wrote: @instagibbs <https://github.com/instagibbs> Thanks for tackling this quickly. I'm not too familiar with this part of the code: is there any scenario where the wrong fee could have been selected due to the uninitialized read? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#17643?email_source=notifications&email_token=ABMAFUYWFWI3Y4YRACOKHYDQWQWUJA5CNFSM4JTMA4CKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFRWY4Y#issuecomment-560163955>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABMAFU6LGFOFK724EYW4DG3QWQWUJANCNFSM4JTMA4CA> .

practicalswift · 2019-12-02T07:16:00Z

A demo of the unitialized read (prior to the fix):

$ test/functional/uninitialized_orig_fee.py -l WARNING
origfee: 1401472.87914520
$ test/functional/uninitialized_orig_fee.py -l WARNING
origfee: 1400508.05266608
$ test/functional/uninitialized_orig_fee.py -l WARNING
origfee: 1404964.24912920
$ test/functional/uninitialized_orig_fee.py -l WARNING
origfee: 1E-8
$ test/functional/uninitialized_orig_fee.py -l WARNING
origfee: 1398604.42601496

Code:

#!/usr/bin/env python3

from decimal import Decimal

from test_framework.messages import BIP125_SEQUENCE_NUMBER
from test_framework.test_framework import BitcoinTestFramework
from test_framework.util import connect_nodes

MIN_RELAY_FEE = Decimal("0.00000141")
FEE_RATE = 0.0015

SEND_AMOUNT_1 = Decimal("49")
SEND_AMOUNT_2 = Decimal("48")


class UninitializedOrigFeeTest(BitcoinTestFramework):
    def set_test_params(self):
        self.num_nodes = 2
        self.setup_clean_chain = True
        self.extra_args = [
            ["-walletrbf={}".format(i), "-mintxfee=0.00002", "-addresstype=bech32"]
            for i in range(self.num_nodes)
        ]

    def run_test(self):
        connect_nodes(self.nodes[0], 1)
        self.sync_all()
        peer_node, rbf_node = self.nodes
        rbf_node_address = rbf_node.getnewaddress()
        peer_node.generate(101)
        self.sync_all()
        peer_node.sendtoaddress(rbf_node_address, SEND_AMOUNT_1)
        self.sync_all()
        peer_node.generate(1)
        self.sync_all()
        dest_address = peer_node.getnewaddress()
        tx_input = dict(
            sequence=BIP125_SEQUENCE_NUMBER,
            **next(u for u in rbf_node.listunspent() if u["amount"] == SEND_AMOUNT_1)
        )
        destinations = {dest_address: SEND_AMOUNT_2}
        destinations[rbf_node.getrawchangeaddress()] = (
            SEND_AMOUNT_1 - SEND_AMOUNT_2 - MIN_RELAY_FEE
        )
        rawtx = rbf_node.createrawtransaction([tx_input], destinations)
        signedtx = rbf_node.signrawtransactionwithwallet(rawtx)
        rbfid = rbf_node.sendrawtransaction(signedtx["hex"])
        self.sync_mempools((rbf_node, peer_node))
        bumped_tx = rbf_node.bumpfee(rbfid, {"fee_rate": FEE_RATE})
        print("origfee: {}".format(bumped_tx["origfee"]))


if __name__ == "__main__":
    UninitializedOrigFeeTest().main()

practicalswift · 2019-12-02T07:22:41Z

With the patch in this PR applied:

$ test/functional/uninitialized_orig_fee.py -l WARNING
origfee: 0.00000141
$ test/functional/uninitialized_orig_fee.py -l WARNING
origfee: 0.00000141
$ test/functional/uninitialized_orig_fee.py -l WARNING
origfee: 0.00000141
$ test/functional/uninitialized_orig_fee.py -l WARNING
origfee: 0.00000141
$ test/functional/uninitialized_orig_fee.py -l WARNING
origfee: 0.00000141

achow101

ACK 02afb0c

maflcko · 2019-12-03T15:27:00Z

ACK. Tested by running the test without re-compiling bitcoind

02afb0c Fix origfee return for bumpfee with feerate arg (Gregory Sanders) Pull request description: fixes #17642 and adds a simple test that would have caught it ACKs for top commit: achow101: ACK 02afb0c Tree-SHA512: 303e392e05407f204dffe360689b5bb5dc77fd462dd0e489bc0b6c8f94f89ab7fe2bd8cb47e4dc6dc5c23a619826d15f3bf6b02b2c8e96402fbb51953c462e2d

…ate arg 02afb0c Fix origfee return for bumpfee with feerate arg (Gregory Sanders) Pull request description: fixes bitcoin#17642 and adds a simple test that would have caught it ACKs for top commit: achow101: ACK 02afb0c Tree-SHA512: 303e392e05407f204dffe360689b5bb5dc77fd462dd0e489bc0b6c8f94f89ab7fe2bd8cb47e4dc6dc5c23a619826d15f3bf6b02b2c8e96402fbb51953c462e2d

Github-Pull: bitcoin#17643 Rebased-From: 02afb0c

bd8c6f1 Fix origfee return for bumpfee with feerate arg (Gregory Sanders) Pull request description: Backport of Github-Pull: #17643 Rebased-From: 02afb0c ACKs for top commit: fanquake: ACK bd8c6f1 - the appveyor failure is unrelated. instagibbs: utACK bd8c6f1 Tree-SHA512: 7e420a3fe02503194b6fc8eae5277c46289cd6abe131b2513ad80422819e6bafbc7768e7be344d4132ebdbc24846d459ba2a271be184725d818dff77510fa4de

fanquake · 2020-01-06T00:00:47Z

Was backported in #17859.

[0.19] Backports bitcoin#17858 Unbreak build with Boost 1.72.0 bitcoin#17654 cli: fix Fatal LevelDB error when specifying -blockfilterindex=basic twice bitcoin#17687 rpc: require second argument only for scantxoutset start action bitcoin#17728 wallet: Fix origfee return for bumpfee with feerate arg bitcoin#17643 test: fix "bitcoind already running" warnings on macOS bitcoin#17488 net: Log to net category for exceptions in ProcessMessages bitcoin#17762 Updates to appveyor config for VS2019 and Qt5.9.8 + msvc project fixes bitcoin#17364 Appveyor improvement - text file for vcpkg package list bitcoin#17416 Update msvc build for Visual Studio 2019 v16.4 bitcoin#17736 scripts: fix symbol-check & security-check argument passing bitcoin#17857 qt: Periodic translations update for 0.19 branch IsUsedDestination should count any known single-key address bitcoin#17621 init: Stop indexes on shutdown after ChainStateFlushed callback. bitcoin#17897 qt: Translations update pre-rc1 wallet: Reset reused transactions cache bitcoin#17843 Squashed 'src/univalue/' changes from 7890db9..98261b1 0.19: Update univalue subtree bitcoin#18100 qt: Pre-rc2 translations update

[0.19] Backports bitcoin#17858 Unbreak build with Boost 1.72.0 bitcoin#17654 cli: fix Fatal LevelDB error when specifying -blockfilterindex=basic twice bitcoin#17687 rpc: require second argument only for scantxoutset start action bitcoin#17728 wallet: Fix origfee return for bumpfee with feerate arg bitcoin#17643 test: fix "bitcoind already running" warnings on macOS bitcoin#17488 net: Log to net category for exceptions in ProcessMessages bitcoin#17762 Updates to appveyor config for VS2019 and Qt5.9.8 + msvc project fixes bitcoin#17364 Appveyor improvement - text file for vcpkg package list bitcoin#17416 Update msvc build for Visual Studio 2019 v16.4 bitcoin#17736 scripts: fix symbol-check & security-check argument passing bitcoin#17857 qt: Periodic translations update for 0.19 branch IsUsedDestination should count any known single-key address bitcoin#17621 init: Stop indexes on shutdown after ChainStateFlushed callback. bitcoin#17897 qt: Translations update pre-rc1 wallet: Reset reused transactions cache bitcoin#17843 Squashed 'src/univalue/' changes from 7890db9..98261b1 0.19: Update univalue subtree bitcoin#18100 qt: Pre-rc2 translations update [0.19] Further 0.19 backports bitcoin#18218 build: don't embed a build-id when building libdmg-hfsplus bitcoin#18004

…ate arg 02afb0c Fix origfee return for bumpfee with feerate arg (Gregory Sanders) Pull request description: fixes bitcoin#17642 and adds a simple test that would have caught it ACKs for top commit: achow101: ACK 02afb0c Tree-SHA512: 303e392e05407f204dffe360689b5bb5dc77fd462dd0e489bc0b6c8f94f89ab7fe2bd8cb47e4dc6dc5c23a619826d15f3bf6b02b2c8e96402fbb51953c462e2d

fanquake added Wallet Needs backport (0.19) labels Dec 1, 2019

darosior approved these changes Dec 1, 2019

View reviewed changes

fanquake requested a review from achow101 December 1, 2019 22:58

Fix origfee return for bumpfee with feerate arg

02afb0c

instagibbs force-pushed the bf_expl_fix branch from a0655ad to 02afb0c Compare December 2, 2019 01:55

achow101 approved these changes Dec 2, 2019

View reviewed changes

maflcko changed the title ~~Fix origfee return for bumpfee with feerate arg~~ wallet: Fix origfee return for bumpfee with feerate arg Dec 3, 2019

maflcko merged commit 02afb0c into bitcoin:master Dec 3, 2019

luke-jr pushed a commit to luke-jr/bitcoin that referenced this pull request Jan 3, 2020

Fix origfee return for bumpfee with feerate arg

bd8c6f1

Github-Pull: bitcoin#17643 Rebased-From: 02afb0c

luke-jr mentioned this pull request Jan 3, 2020

[0.19] Fix origfee return for bumpfee with feerate arg #17859

Merged

fanquake removed the Needs backport (0.19) label Jan 6, 2020

Sjors mentioned this pull request Mar 2, 2020

bumpfee RPC not working as expected since 0.19.0.1 #18240

Closed

MarkLTZ mentioned this pull request Apr 4, 2020

Bitcoin PR tracking litecoinz-core/litecoinz#14

Closed

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

wallet: Fix origfee return for bumpfee with feerate arg #17643

wallet: Fix origfee return for bumpfee with feerate arg #17643

Uh oh!

instagibbs commented Dec 1, 2019

Uh oh!

DrahtBot commented Dec 1, 2019

Uh oh!

darosior left a comment

Uh oh!

darosior Dec 1, 2019

Uh oh!

instagibbs Dec 2, 2019

Uh oh!

practicalswift commented Dec 1, 2019 •

edited

Loading

Uh oh!

instagibbs commented Dec 1, 2019 via email

Uh oh!

practicalswift commented Dec 2, 2019 •

edited

Loading

Uh oh!

practicalswift commented Dec 2, 2019

Uh oh!

achow101 left a comment

Uh oh!

maflcko commented Dec 3, 2019

Uh oh!

fanquake commented Jan 6, 2020

Uh oh!

Uh oh!

wallet: Fix origfee return for bumpfee with feerate arg #17643

wallet: Fix origfee return for bumpfee with feerate arg #17643

Uh oh!

Conversation

instagibbs commented Dec 1, 2019

Uh oh!

DrahtBot commented Dec 1, 2019

Conflicts

Uh oh!

darosior left a comment

Choose a reason for hiding this comment

Uh oh!

darosior Dec 1, 2019

Choose a reason for hiding this comment

Uh oh!

instagibbs Dec 2, 2019

Choose a reason for hiding this comment

Uh oh!

practicalswift commented Dec 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

instagibbs commented Dec 1, 2019 via email

Uh oh!

practicalswift commented Dec 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Dec 2, 2019

Uh oh!

achow101 left a comment

Choose a reason for hiding this comment

Uh oh!

maflcko commented Dec 3, 2019

Uh oh!

fanquake commented Jan 6, 2020

Uh oh!

Uh oh!

practicalswift commented Dec 1, 2019 •

edited

Loading

practicalswift commented Dec 2, 2019 •

edited

Loading