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

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

<!--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:

• #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.

ACK a0655adcdb31c36155585f0014afd86998b28fab

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

No it's simply a reporting issue.

On Sun, Dec 1, 2019, 4:51 PM practicalswift notifications@github.com 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 https://github.com/bitcoin/bitcoin/pull/17643?email_source=notifications&email_token=ABMAFUYWFWI3Y4YRACOKHYDQWQWUJA5CNFSM4JTMA4CKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEFRWY4Y#issuecomment-560163955, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABMAFU6LGFOFK724EYW4DG3QWQWUJANCNFSM4JTMA4CA .

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()

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

ACK 02afb0c550dc8529918460c845d1da3adf236eed

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

Was backported in #17859.