test: add coverage for sigop limit policy (-bytespersigop setting) #27171

pull theStack wants to merge 1 commits into bitcoin:master from theStack:202302-add_bytespersigop_test changing 2 files +126 −0
  1. theStack commented at 0:44 am on February 28, 2023: contributor

    This PR adds missing test coverage for the -bytespersigop option, which determines how pre-taproot signature operations (OP_CHECKSIG{VERIFY}, OP_CHECKMULTIGSIG{VERIFY}) affect fee handling calculations. The setting was introduced in PR #7081 for mitigating the sigop spam attack; the initial implementation rejected txs exceeding the limit, but was changed in #8365 later to account for higher sizes in the mempool (i.e. exceeding the sigop limit is possible, but has to be compensated by higher fees).

    For each combination of -bytespersigop setting and sigops count, the test first creates a P2WSH spending transaction with a witness script that puts sigops in a non-executing branch (OP_FALSE OP_IF OP_CHECKMULTISIG … OP_CHECKSIG … OP_ENDIF). This tx is then bumped up to reach exactly the sig-op limit equivalent vsize by padding its datacarrier output. Based on that, increasing the tx’s vsize should still reflect a vsize increase in the mempool, while a decrease of the tx’s vsize should lead to the mempool treating the tx’s vsize to be the sig-op limit equivalent vsize, since the limit was exceeded.

    I assume that this parameter is almost never set explicitly by users (also it is not relevant for taproot spends), but it doesn’t hurt to have a test for it. See also https://bitcoin.stackexchange.com/a/87958 for another explanation.

  2. DrahtBot commented at 0:44 am on February 28, 2023: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK glozow, MarcoFalke

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

  3. DrahtBot added the label Tests on Feb 28, 2023
  4. theStack force-pushed on Feb 28, 2023
  5. fanquake requested review from glozow on Feb 28, 2023
  6. glozow commented at 9:30 am on February 28, 2023: member
    Concept ACK
  7. in test/functional/mempool_sigoplimit.py:103 in 4ff0a260e8 outdated
     98+        assert_equal(res['vsize'], sigop_equivalent_vsize+1)
     99+
    100+        # decrease the tx's vsize to be right below the sigop-limit equivalent size
    101+        # => tx's vsize in mempool should stick at the sigop-limit equivalent
    102+        # bytes level (i.e. exceeding the sigop limit is possible, but has to
    103+        # be compensated by higher fees)
    


    glozow commented at 5:34 pm on March 6, 2023:
    Not sure if I’m misunderstanding this comment but afaik the reason is that txsize is max(serialized size, “sigop” size), not that fees have any impact?

    theStack commented at 3:25 am on March 7, 2023:

    The intention was to express the implication of exceeding the sig-op limit from a user’s perspective (tx is treated as larger than it’s serialized vsize => more fees needed to reach the same fee-rate; or as PR desc of #8365 put it “high-sigop transactions […] need to pay a fee corresponding to the maximally-used resource.”). Agree that the comment is rather misleading and not very helpful for the test, replaced it.

    Generally struggled a bit with terminology for this PR (not even sure if talking about a “limit” is proper, usually we reject txs from mempool if limits are exceeded), suggestions of course very welcome.


    glozow commented at 5:24 pm on March 8, 2023:
    Ah I see where that comes from, thanks for clarifying! Comment does look better to me now.
  8. test: add coverage for sigop limit policy (`-bytespersigop` setting) 89cd20cbed
  9. theStack force-pushed on Mar 7, 2023
  10. glozow commented at 5:38 pm on March 8, 2023: member

    light review ACK 89cd20cbed

    (Doesn’t need to be in this PR) it could be worth checking that this vsize calculation is used for anc/desc limits as well?

  11. fanquake requested review from maflcko on Mar 8, 2023
  12. in test/functional/mempool_sigoplimit.py:74 in 89cd20cbed
    69+        # (note that the sigops count even though being in a branch that's not executed)
    70+        num_multisigops = num_sigops // 20
    71+        num_singlesigops = num_sigops % 20
    72+        witness_script = CScript(
    73+            [OP_FALSE, OP_IF] +
    74+            [OP_CHECKMULTISIG]*num_multisigops +
    


    maflcko commented at 12:24 pm on March 10, 2023:
    for reference, usually fAccurate is set for witness scripts sigop counts, but omitting the k and n in the CMS imitates fAccurate=false.
  13. maflcko approved
  14. maflcko commented at 12:26 pm on March 10, 2023: member

    nice ACK 89cd20cbedbba344bab92dd1d71dac9c84320a70 📁

    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: nice ACK 89cd20cbedbba344bab92dd1d71dac9c84320a70  📁
    3B5cwOnNBzAlwwLnU0zkxGtximOvhWc70b8b8cIBDwmGXl+th6o29WfVzP4QXUU+NEdze0Yl8uHyok7mzdst1AQ==
    
  15. fanquake merged this on Mar 10, 2023
  16. fanquake closed this on Mar 10, 2023

  17. fanquake commented at 1:58 pm on March 10, 2023: member

    @theStack could open a followup addressing anything here?

    (Doesn’t need to be in this PR) it could be worth checking that this vsize calculation is used for anc/desc limits as well?

    for reference, usually fAccurate is set for witness scripts sigop counts, but omitting the k and n in the CMS imitates fAccurate=false.

  18. maflcko commented at 2:01 pm on March 10, 2023: member
    My comment was only meant for reviewers. A unit test may be more appropriate if someone wants to check the behavior of the sigop counting function itself.
  19. theStack deleted the branch on Mar 10, 2023
  20. sidhujag referenced this in commit aa4696f8d7 on Mar 10, 2023
  21. theStack commented at 12:49 pm on March 11, 2023: contributor

    @theStack could open a followup addressing anything here?

    (Doesn’t need to be in this PR) it could be worth checking that this vsize calculation is used for anc/desc limits as well?

    for reference, usually fAccurate is set for witness scripts sigop counts, but omitting the k and n in the CMS imitates fAccurate=false.

    Yes, I’m planning to do a follow-up next week, extending the checks on ancestor/descendant limits is definitely a good idea! Regarding Marco’s explanation w.r.t. fAccurate, will consider adding a small explanation comment (something like e.g. “if k and n are omitted, the sig-ops costs of OP_CMS are 20 even in witness spends”).

  22. fanquake referenced this in commit 0973018067 on Mar 19, 2023
  23. sidhujag referenced this in commit 11890d713a on Mar 19, 2023
  24. bitcoin locked this on Mar 10, 2024

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: 2024-10-31 06:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me