test: refactor: dedup mempool_package_limits.py subtests via decorator #27350

theStack commented at 11:20 PM on March 27, 2023: contributor

The subtests in the functional test mempool_package_limits.py all follow the same pattern:

first, check that the mempool is currently empty
create and submit certain single txs to the mempool, prepare list of hex transactions
check that testmempoolaccept on the package hex fails with a "package-mempool-limits" error on each tx result
after mining a block, check that submitting the package succeeds

Note that steps 1,3,4 are identical for each of the subtests and only step 2 varies, so this might be a nice opportunity to deduplicate code by using a newly introduced decorator which executes the necessary before and after the essential part of the subtest. This also makes it easier to add new subtests without having to copy-paste those parts once again.

In addition, the first commit switches the fee unit from BTC to Satoshis, which allows to get rid of some imports (COIN and Decimal) and a comment for the test_desc_size_limits subtest is fixed (s/25KvB/21KvB/).

DrahtBot commented at 11:20 PM on March 27, 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, ismaelsadeeq

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 Mar 27, 2023

glozow assigned glozow on Mar 28, 2023

in test/functional/mempool_package_limits.py:21 in 0ed2f030d0 outdated

  21 | +
  22 | +# Decorator to check that
  23 | +# 1) mempool is empty at the start of a subtest
  24 | +# 2) after running the subtest, submitting the created package
  25 | +#    fails with the error "package-mempool-limits" for each tx
  26 | +# 3) after mining a block, submitting the created package succeeds

glozow commented at 10:36 AM on March 28, 2023:

(1) I think it could be useful to clarify that the subtest modifies mempool and creates the txns (2) I prefer to reserve the term "submit" for sendraw and submitpackage

# 2) run the subtest, which may submit some transaction(s) to the mempool and create a list of hex transactions
# 3) testmempoolaccept the package hex and check that it fails with the error "package-mempool-limits" for each tx
# 4) after mining a block, clearing the pre-submitted transactions from mempool, submitting the created package succeeds

theStack commented at 8:17 PM on March 28, 2023:

Thanks, updated the description! (Makes sense to use "submit" only for sendrawtransaction and submitpackage RPCs; neat idea of using "testmempoolaccept" directly as a verb :)).

in test/functional/mempool_package_limits.py:321 in 0ed2f030d0 outdated

 316 | @@ -339,12 +317,11 @@ def test_desc_size_limits(self):
 317 |          and in-package descendants are all considered together.
 318 |          """
 319 |          node = self.nodes[0]
 320 | -        assert_equal(0, node.getmempoolinfo()["size"])
 321 |          target_weight = 21 * 1000 * WITNESS_SCALE_FACTOR
 322 | -        high_fee = Decimal("0.0021") # 10 sats/vB
 323 | +        high_fee = 210_000  # 10 sats/vB

glozow commented at 10:39 AM on March 28, 2023:

Ah nice. But might as well just make it this?

        high_fee = target_weight * 10

theStack commented at 8:10 PM on March 28, 2023:

To make this work, we need to convert back from weight to vsize first. Since fee = (target_weight / WITNESS_SCALE_FACTOR) * 10 looks rather weird, I decided to specify the target size directly in vsize and deduce both fee and weight from that. One more variable, but (hopefully) more readable and less indirect magic number magic involved.

test: refactor: use Satoshis for fees in mempool_package_limits.py

This avoids having to convert from BTC to Sats and needs less imports.
Also specify the tx's target size in vsize rather than in weight, which
allows us to specify the fee-rate by a simple multiplication, rather
than having another magic number for it.

72f25e238c

test: dedup package limit checks via decorator in mempool_package_limits.py e669833943

theStack force-pushed on Mar 28, 2023

glozow commented at 4:33 PM on March 29, 2023: member

utACK e669833943bda13b2840a174dc8e45194187fc8e

fanquake requested review from josibake on Mar 30, 2023

in test/functional/mempool_package_limits.py:24 in e669833943

  24 | +#    create a list of hex transactions
  25 | +# 3) testmempoolaccept the package hex and check that it fails with the error
  26 | +#    "package-mempool-limits" for each tx
  27 | +# 4) after mining a block, clearing the pre-submitted transactions from mempool,
  28 | +#    check that submitting the created package succeeds
  29 | +def check_package_limits(func):

ismaelsadeeq commented at 12:09 PM on March 30, 2023:

Nice :100: Just a couple of nit minor suggestions to improve the readability of the code:

Add a blank line after the first assertion assert_equal(0, node.getmempoolinfo()["size"])
Add a blank line after the line where package_hex is assigned to func(self, *args, **kwargs)

Separating the operations as written in the decorator comment, also goes well with PEP9 guidelines.

ismaelsadeeq commented at 12:13 PM on March 30, 2023: member

ACK e669833943bda13b2840a174dc8e45194187fc8e

glozow added the label Refactoring on Mar 30, 2023

glozow removed the label Tests on Mar 30, 2023

glozow merged this on Mar 30, 2023

glozow closed this on Mar 30, 2023

theStack deleted the branch on Mar 30, 2023

sidhujag referenced this in commit 93f8659f26 on Apr 1, 2023

bitcoin locked this on Mar 29, 2024