test: switch MiniWallet padding unit from weight to vsize #30718

1. 
   theStack commented at 6:11 pm on August 26, 2024: contributor

   This PR is a late follow-up for #30162, where I retrospectively consider the padding unit of choice as a mistake. The weight unit is merely a consensus rule detail and is largely irrelevant from a user’s perspective w.r.t. fee-rate calculations and mempool policy rules (e.g. for package relay and TRUC limits), so there doesn’t seem to be any value of using a granularity that we can’t even guarantee to reach exactly anyway.

   Switch to the more natural unit of vsize instead, which simplifies both the padding implementation (no “round up to the next multiple of 4” anymore) and the current tests that take use of this padding. The rather annoying multiplications by WITNESS_SCALE_FACTOR can then be removed and weird-looking magic numbers like 4004 can be replaced by numbers that are more connected to actual policy limit constants from the codebase, e.g. 1001 for exceeding TRUC_CHILD_MAX_VSIZE by one. The second commits introduces a constant for that.

2. 
   DrahtBot commented at 6:11 pm on August 26, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	instagibbs

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #30592 (Remove mempoolfullrbf by instagibbs)
   • #30322 ([test]: raise an exception in _bulk_tx_ when target_weight is too low. by ismaelsadeeq)
   • #28676 ([WIP] Cluster mempool implementation by sdaftuar)

   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.

3. DrahtBot renamed this:
   test: switch MiniWallet padding unit from weight to vsize
   test: switch MiniWallet padding unit from weight to vsize
   on Aug 26, 2024
4. DrahtBot added the label Tests on Aug 26, 2024
5. theStack force-pushed on Aug 26, 2024
6. DrahtBot added the label CI failed on Aug 26, 2024
7. 
   DrahtBot commented at 6:15 pm on August 26, 2024: contributor

   🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29268369249

   Make sure to run all tests locally, according to the documentation.

   The failure may happen due to a number of reasons, for example:

   • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

   • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

   • An intermittent issue.

   Leave a comment here, if you need help tracking down a confusing failure.

8. test: switch MiniWallet padding unit from weight to vsize

   The weight unit is merely a consensus rule detail and is largely
   irrelevant for fee-rate calculations and mempool policy rules (e.g. for
   package relay and TRUC limits), so there doesn't seem to be any value of
   using a granularity that we can't even guarantee to reach exactly
   anyway.
   
   Switch to the more natural unit of vsize instead, which simplifies both
   the padding implementation and the current tests that take use of this
   padding. The rather annoying multiplications by `WITNESS_SCALE_FACTOR`
   can then be removed and weird-looking magic numbers like `4004` can be
   replaced by numbers that are more connected to actual policy limit
   constants from the codebase, e.g. `1001` for exceeding
   `TRUC_CHILD_MAX_VSIZE` by one.

   774a3e9b2d
9. test: refactor: introduce and use `TRUC_CHILD_MAX_VSIZE` constant 7d1c8d0aa3
10. theStack force-pushed on Aug 26, 2024
11. DrahtBot removed the label CI failed on Aug 26, 2024
12. fanquake requested review from instagibbs on Sep 12, 2024
13. in test/functional/mempool_limit.py:139 in 774a3e9b2d outdated

    136+        parent_vsize = 25000
    137         num_big_parents = 3
    138-        assert_greater_than(parent_weight * num_big_parents, current_info["maxmempool"] - current_info["bytes"])
    139+        # Need to be large enough to trigger eviction
    140+        # (note that the mempool usage of a tx is about three times its vsize)
    141+        assert_greater_than(parent_vsize * num_big_parents * 3, current_info["maxmempool"] - current_info["bytes"])
    

    ---

    ---

    

    instagibbs commented at 3:54 pm on September 12, 2024:

    without this multiplier was there an issue in this PR?

    ---

    

    theStack commented at 2:59 pm on September 19, 2024:

    without this multiplier was there an issue in this PR?

    Yes, without it the assertion failed (AssertionError: 75000 <= 135680).

14. instagibbs approved
15. 
    instagibbs commented at 4:02 pm on September 12, 2024: member

    ACK 7d1c8d0aa3ebe30d7633caf9bc4765927e4d6e08

    seems straight forward to me, and all usages appear to be concerned with wallet/mempool things, not consensus related activities which may require weight-level precision.

    For weight-level consensus precision I suspect wallet-like functionality is less interesting.

Contributors
theStack DrahtBot instagibbs

Labels
Tests