test: add coverage for sigop limit policy (-bytespersigop setting)
#27171
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
7ac70fa to
4ff0a26
Compare
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah I see where that comes from, thanks for clarifying! Comment does look better to me now.
4ff0a26 to
89cd20c
Compare
maflcko
left a comment
There was a problem hiding this comment.
nice ACK 89cd20c 📁
Show signature
Signature:
untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: nice ACK 89cd20cbedbba344bab92dd1d71dac9c84320a70 📁
B5cwOnNBzAlwwLnU0zkxGtximOvhWc70b8b8cIBDwmGXl+th6o29WfVzP4QXUU+NEdze0Yl8uHyok7mzdst1AQ==
| num_singlesigops = num_sigops % 20 | ||
| witness_script = CScript( | ||
| [OP_FALSE, OP_IF] + | ||
| [OP_CHECKMULTISIG]*num_multisigops + |
There was a problem hiding this comment.
for reference, usually fAccurate is set for witness scripts sigop counts, but omitting the k and n in the CMS imitates fAccurate=false.
|
@theStack could open a followup addressing anything here?
|
|
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. |
…espersigop` setting) 89cd20c test: add coverage for sigop limit policy (`-bytespersigop` setting) (Sebastian Falbesoner) Pull request description: 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 bitcoin#7081 for mitigating the [sigop spam attack](https://bitcointalk.org/index.php?topic=1166928.0); the initial implementation rejected txs exceeding the limit, but was changed in bitcoin#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. ACKs for top commit: glozow: light review ACK 89cd20c MarcoFalke: nice ACK 89cd20c 📁 Tree-SHA512: 06998ce93bf9d5ce6143db2996a43f13990c415f97afe684227ad469349e73952bf4f6c871c1e6349e07606f4d45db64408848873a86a89481cdca5a134e5e60
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"). |
…s ancestor/descendant size (27171 follow-up) 6d24d1e test: check that sigop limit also affects ancestor/descendant size (Sebastian Falbesoner) Pull request description: This is a follow-up to #27171, adding a check that the sigop-limit vsize logic is also respected for {ancestor,descendant}size calculation (as suggested in bitcoin/bitcoin#27171 (review)). For simplicity, we use a one-parent-one-child cluster here and only check for the case that the sigop-limit equivalent size is larger than the serialized vsize. ACKs for top commit: glozow: code review ACK 6d24d1e, thanks for taking! Tree-SHA512: dc65e455d06cfef1f1d6a53b959f99ec1ca3fe51c98dc1ed5826614b5619773d34aff0171c43a0ede4fd45605b2eb7a9278e027196128bb7ad8586b859f1cf70
…or/descendant size (27171 follow-up) 6d24d1e test: check that sigop limit also affects ancestor/descendant size (Sebastian Falbesoner) Pull request description: This is a follow-up to bitcoin#27171, adding a check that the sigop-limit vsize logic is also respected for {ancestor,descendant}size calculation (as suggested in bitcoin#27171 (review)). For simplicity, we use a one-parent-one-child cluster here and only check for the case that the sigop-limit equivalent size is larger than the serialized vsize. ACKs for top commit: glozow: code review ACK 6d24d1e, thanks for taking! Tree-SHA512: dc65e455d06cfef1f1d6a53b959f99ec1ca3fe51c98dc1ed5826614b5619773d34aff0171c43a0ede4fd45605b2eb7a9278e027196128bb7ad8586b859f1cf70
This PR adds missing test coverage for the
-bytespersigopoption, 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
-bytespersigopsetting 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.