test, refactor: fund tx deterministically in `feature_coinstatindex` #24570

pull josibake wants to merge 1 commits into bitcoin:master from josibake:josibake-make-coinstatindex-test-deterministic changing 1 files +15 −2
  1. josibake commented at 4:37 PM on March 15, 2022: member

    While working on some changes to coin selection in a separate branch, I noticed this test was failing. This is because the test expects exact values from gettxoutsetinfo to compare against, which relies on coin selection to be deterministic.

    If coin selection is changed in any way, this test will fail and need to be updated with new values.

    Instead, we can make the test deterministic by filtering to a UTXO of a specific amount as our first input and then using fundrawtransaction to grab a second UTXO. This works because the remaining large UTXOs in the wallet are coinbase outputs. Creating a transaction in this way does not affect any of the logic in the test.

    <!-- *** Please remove the following help text before submitting: *** Pull requests without a rationale and clear improvement may be closed immediately. GUI-related pull requests should be opened against https://github.com/bitcoin-core/gui first. See CONTRIBUTING.md -->

    <!-- Please provide clear motivation for your patch and explain how it improves Bitcoin Core user experience or Bitcoin Core developer experience significantly: * Any test improvements or new tests that improve coverage are always welcome. * All other changes should have accompanying unit tests (see `src/test/`) or functional tests (see `test/`). Contributors should note which tests cover modified code. If no tests exist for a region of modified code, new tests should accompany the change. * Bug fixes are most welcome when they come with steps to reproduce or an explanation of the potential issue as well as reasoning for the way the bug was fixed. * Features are welcome, but might be rejected due to design or scope issues. If a feature is based on a lot of dependencies, contributors should first consider building the system outside of Bitcoin Core, if possible. * Refactoring changes are only accepted if they are required for a feature or bug fix or otherwise improve developer experience significantly. For example, most "code style" refactoring changes require a thorough explanation why they are useful, what downsides they have and why they *significantly* improve developer experience or avoid serious programming bugs. Note that code style is often a subjective matter. Unless they are explicitly mentioned to be preferred in the [developer notes](/doc/developer-notes.md), stylistic code changes are usually rejected. -->

    <!-- Bitcoin Core has a thorough review process and even the most trivial change needs to pass a lot of eyes and requires non-zero or even substantial time effort to review. There is a huge lack of active reviewers on the project, so patches often sit for a long time. -->

  2. fanquake added the label Tests on Mar 15, 2022
  3. maflcko commented at 4:43 PM on March 15, 2022: member

    Would it be harder to use MiniWallet, which is deterministic by design?

  4. josibake renamed this:
    test, refactor: fund tx deterministically in `test/functional/feature_coinstatindex.py`
    test, refactor: fund tx deterministically in `feature_coinstatindex`
    on Mar 15, 2022
  5. josibake commented at 4:54 PM on March 15, 2022: member

    Would it be harder to use MiniWallet, which is deterministic by design?

    Good point! Looking through the full test, it seems like you could use MiniWallet, but it's definitely a bigger change than the few lines I did here. Selfishly, I'd prefer making the small change now to unblock myself on a bigger PR I'm working on for coin selection, and then come back to this one later and re-write it to use MiniWallet in a follow-up PR.

  6. josibake force-pushed on Mar 16, 2022
  7. josibake commented at 12:34 PM on March 16, 2022: member

    force push to fix the commit message

  8. in test/functional/feature_coinstatsindex.py:164 in 9a8b21f0da outdated 
     160 | +        # we select this as the first input and then use `fundrawtransaction`
 161 | +        # to select a 50 BTC coinbase UTXO from our wallet
 162 |          tx1_inputs = []
 163 | +        for utxo in self.nodes[0].listunspent():
 164 | +            if utxo['amount'] == 40:
 165 | +                tx1_inputs.append({"txid":utxo['txid'], "vout": utxo['vout']})
    willcl-ark commented at 11:57 AM on March 17, 2022:

    nit: missing space after ':'

    tx1_inputs.append({"txid": utxo['txid'], "vout": utxo['vout']})

    josibake commented at 2:18 PM on March 17, 2022:

    good catch!

  9. in test/functional/feature_coinstatsindex.py:150 in 9a8b21f0da outdated 
     146 | @@ -147,11 +147,24 @@ def _test_coin_stats_index(self):
 147 |              })
 148 |              self.block_sanity_check(res5['block_info'])
 149 |  
 150 | -        # Generate and send a normal tx with two outputs
 151 | +        # Generate and send a normal tx with two outputs.
    willcl-ark commented at 11:57 AM on March 17, 2022:

    nit: These lines are > 79 chars

    josibake commented at 2:18 PM on March 17, 2022:

    fixed!

  10. willcl-ark approved
  11. willcl-ark commented at 12:03 PM on March 17, 2022: contributor

    tACK. Although I didn't have @josibake's branch to test against (where the tests were failing), the test passes with this patch, and making the test more deterministic here is an improvement.

  12. test: make `feature_coinstatindex` deterministic
    select a specific input from listunspent
and use fundrawtransaction to add the remaining amount
this ensures the same values are output from gettxoutsetinfo
regardless of the coin selection algorithm used
    6980d74cbb
  13. josibake force-pushed on Mar 17, 2022
  14. DrahtBot commented at 8:40 PM on March 17, 2022: contributor

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

    • #24584 ([RFC] wallet: avoid mixing different OutputTypes during coin selection by josibake)

    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.

  15. maflcko commented at 8:42 PM on March 17, 2022: member

    See #24605 as alternative

  16. josibake commented at 10:15 AM on March 18, 2022: member

    closing in favor of https://github.com/bitcoin/bitcoin/pull/24605

  17. josibake closed this on Mar 18, 2022
  18. josibake deleted the branch on Mar 18, 2022
  19. bitcoin locked this on Oct 11, 2023
Contributors
josibakemaflckowillcl-arkDrahtBot
Labels
Tests
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-17 09:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me