test: a test to check descendant limits #22582

This PR adds a new functional test to test the new descendant limits for packages that were proposed in #21800.

+----------------------+
|                      |
|         M1           |
|        ^  ^          |
|       M2   ^         |
|      .      ^        |
|     .        ^       |
|    .          ^      |
|   .            ^     |
|  M24            ^    |
|                  ^   |
|                  P1  |
|                  ^   |
|                  P2  |
|                      |
+----------------------+

This test is for checking a transaction to fail its descendant count limits because of a combination of mempool descendants, package direct descendants, and package indirect descendants.

In this test, P1 has M1 as a mempool ancestor, P2 has no in-mempool ancestors, but when combined P2 has M1 as an ancestor and M1 exceeds descendant_limits (23 in-mempool descendants + 2 in-package descendants, a total of 26 including itself)

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

Concept ACK

Concept ACK to more testing.

The commit message for c18188e96204f9264e2ed64ab5e53f3abdd137e5 seems to contain some squash comments? Would be better to describe exactly what edge case you're testing / coverage you're adding.

Will review again after rebase - note that the mempool tests have been moved to mempool_package_limits.py

Thanks for rebasing, the test is well-written and does what the comment says it does.

Suggestion: label your commit message with [test] and add a short description about why this test is an improvement.

Another suggestion (probably for a different PR): This doesn't matter that much, but we have quite a few of the same lines repeated in this test. Given how many OP_TRUE spends there are, I think it'd be good to either add a OP_TRUE variation of make_chain to wallet.py or, better yet, make all those functions methods of MiniWallet.

utACK 5a7050c63c1a574abf336a47b18e154554a4020a

PR description needs to be updated

I still don't understand what logic this is testing that isn't already covered by the test_desc_count_limits() subtest.

I still don't understand what logic this is testing that isn't already covered by the test_desc_count_limits() subtest.

It is testing if the second generation in Package transactions are considered while checking Descendant count limits. It is analogous to the 'test_anc_count_limits_2()' subtest for Ancestor count limits.

@glozow has pointed out that this scenario is for a transaction to fail its descendant limit count due to a combination of mempool descendants, package direct descendants, and package indirect descendants, which seems like a reasonable thing to test.

I think the transaction P3 is unnecessary, since the package would fail with just P2. It's good for a test to fail at the edge case (i.e. the package would be accepted if you removed just one transaction).

I'd also recommend you remove the image from the PR description and replace it with an ascii drawing. The PR description is included in the merge commit, so it's best if it's plain text without attachments.

Code review ACK e0f92f30954438540e6a676af77d17fbb963ca5d. Seems like a useful and straightforward test case, assuming it doesn't duplicate existing coverage.

Code review ACK fa7db1cbf7e8400b625fccd5757f8e1b200796bd. Only were suggested changes since last review: simplifying test and dropping P3 transaction as John suggested, and adding assert_equal I suggested

ACK fa7db1cbf7e8400b625fccd5757f8e1b200796bd

ACK fa7db1cbf7

I've left a couple of comments inline, but they can be picked up in #22901.

If you have to touch this again, I'd suggest changing the commit log:

-[test] checks descendants limtis for second generation Package descendants
+[test] check descendant limits for second generation package descendants