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

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.

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.

DrahtBot added the label Tests on Feb 28, 2023

theStack force-pushed on Feb 28, 2023

fanquake requested review from glozow on Feb 28, 2023

glozow commented at 9:30 am on February 28, 2023: member

Concept ACK

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.

test: add coverage for sigop limit policy (`-bytespersigop` setting) 89cd20cbed

theStack force-pushed on Mar 7, 2023

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?

fanquake requested review from maflcko on Mar 8, 2023

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.

maflcko approved

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==

fanquake merged this on Mar 10, 2023

fanquake closed this on Mar 10, 2023

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.

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.

theStack deleted the branch on Mar 10, 2023

sidhujag referenced this in commit aa4696f8d7 on Mar 10, 2023

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”).

fanquake referenced this in commit 0973018067 on Mar 19, 2023

sidhujag referenced this in commit 11890d713a on Mar 19, 2023

bitcoin locked this on Mar 10, 2024

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

Reviews

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