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	glozow, instagibbs, maflcko, achow101

   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. theStack force-pushed on Aug 26, 2024
9. DrahtBot removed the label CI failed on Aug 26, 2024
10. fanquake requested review from instagibbs on Sep 12, 2024
11. 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).

12. instagibbs approved
13. 
    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.

14. 
    maflcko commented at 2:30 pm on September 23, 2024: member

    lgtm ACK 7d1c8d0aa3ebe30d7633caf9bc4765927e4d6e08 🔋

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: lgtm ACK 7d1c8d0aa3ebe30d7633caf9bc4765927e4d6e08 🔋
    3AilT3VJn41NLLR5ibt4CojkeNEaMt0u6r0xXZBXwvXh439nSfzdvg0vFZrEZ9qrjT+yLJRj7qba+exi7Uk3gAg==
    

15. in test/functional/mempool_truc.py:94 in 7d1c8d0aa3 outdated

    87@@ -87,10 +88,10 @@ def test_truc_acceptance(self):
    88             from_node=node,
    89             fee_rate=DEFAULT_FEE,
    90             utxo_to_spend=tx_v3_parent_normal["new_utxo"],
    91-            target_vsize=997,
    92+            target_vsize=TRUC_CHILD_MAX_VSIZE - 3,
    93             version=3
    94         )
    95-        assert_greater_than_or_equal(1000, tx_v3_child_almost_heavy["tx"].get_vsize())
    96+        assert_greater_than_or_equal(TRUC_CHILD_MAX_VSIZE, tx_v3_child_almost_heavy["tx"].get_vsize())
    

    ---

    ---

    

    glozow commented at 6:03 pm on September 28, 2024:

    a few more 1000s not changed:

    L104 assert_greater_than_or_equal(tx_v3_child_almost_heavy["tx"].get_vsize() + tx_v3_child_almost_heavy_rbf["tx"].get_vsize(), 1000)

    L194 assert_greater_than(node.getmempoolentry(tx_v3_child_large["txid"])["vsize"], 1000)

    ---

    

    theStack commented at 8:49 pm on September 28, 2024:

    Thanks, updated!
16. 
    glozow commented at 6:05 pm on September 28, 2024: member

    ACK 7d1c8d0aa3ebe30d7633caf9bc4765927e4d6e08, lmk if you want to update the second commit
17. theStack force-pushed on Sep 28, 2024
18. 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.

    c16ae71768
19. test: refactor: introduce and use `TRUC_CHILD_MAX_VSIZE` constant 940edd6ac2
20. theStack force-pushed on Sep 28, 2024
21. 
    theStack commented at 8:52 pm on September 28, 2024: contributor

    Thanks for the reviews! Tackled #30718 (review) and also had to rebase on master (as otherwise the CI would complain, probably because the PR didn’t have the switch to CMake included yet).
22. 
    glozow commented at 11:25 pm on September 29, 2024: member

    reACK 940edd6 via range-diff
23. DrahtBot requested review from maflcko on Sep 29, 2024
24. DrahtBot requested review from instagibbs on Sep 29, 2024
25. 
    instagibbs commented at 2:20 pm on September 30, 2024: member

    reACK 940edd6ac24e921f639b1376fe7d35dc14c1887d
26. 
    maflcko commented at 3:25 pm on September 30, 2024: member

    re-ACK 940edd6ac24e921f639b1376fe7d35dc14c1887d 🍷

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
    1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
    2trusted comment: re-ACK 940edd6ac24e921f639b1376fe7d35dc14c1887d 🍷
    3oPob44VYgX9b7OllQsFX35CgTqctQv2rD7E3l4Ofl1Gkfel22T7kAZEZtIZ7Jr2OPKVx/l6IhUVKuF/T6JF2DA==
    

27. 
    achow101 commented at 9:00 pm on September 30, 2024: member

    ACK 940edd6ac24e921f639b1376fe7d35dc14c1887d
28. achow101 merged this on Sep 30, 2024
29. achow101 closed this on Sep 30, 2024

    ---

30. theStack deleted the branch on Sep 30, 2024
31. 
    ismaelsadeeq commented at 10:09 pm on October 1, 2024: member

    post-merge ACK 940edd6ac24e921f639b1376fe7d35dc14c1887d

    This PR simplify _bulk_tx a lot.

Contributors
theStack DrahtBot instagibbs maflcko glozow achow101 ismaelsadeeq

Review Requested
instagibbs maflcko

Labels
Tests