test: a test to check descendant limits #22582

ritickgoenka · 2021-07-29T13:20:40Z

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)

DrahtBot · 2021-07-29T14:03:52Z

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

Conflicts

No conflicts as of last run.

tryphe · 2021-07-30T01:43:42Z

Concept ACK

glozow

Concept ACK to more testing.

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

test/functional/rpc_packages.py

glozow · 2021-08-09T06:18:51Z

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

glozow

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.

test/functional/mempool_package_limits.py

glozow · 2021-08-11T13:54:13Z

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.

test/functional/mempool_package_limits.py

glozow

utACK 5a7050c

test/functional/mempool_package_limits.py

glozow · 2021-08-16T12:11:55Z

PR description needs to be updated

jnewbery · 2021-08-16T13:33:16Z

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

ritickgoenka · 2021-08-16T17:11:56Z

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.

jnewbery · 2021-08-17T15:56:12Z

@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.

ryanofsky

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

test/functional/mempool_package_limits.py

…dants

ryanofsky

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

glozow

ACK fa7db1c

jnewbery

ACK fa7db1c

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

jnewbery · 2021-09-09T13:27:45Z

test/functional/mempool_package_limits.py

+        value = parent_value
+        txid = parent_txid
+        for i in range(23): # M2...M24
+            (tx, txhex, value, spk) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk)


Suggested change

(tx, txhex, value, spk) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk)

tx, txhex, value, spk = make_chain(node, self.address, self.privkeys, txid, value, 0, spk)

jnewbery · 2021-09-09T13:34:25Z

test/functional/mempool_package_limits.py

+        first_coin_a = self.coins.pop()
+        parent_value = (first_coin_a["amount"] - DEFAULT_FEE) / 2 # Deduct reasonable fee and make 2 outputs
+        inputs = [{"txid": first_coin_a["txid"], "vout": 0}]
+        outputs = [{self.address : parent_value}, {ADDRESS_BCRT1_P2WSH_OP_TRUE : parent_value}]


Consider using a linter like flake8 or a formatter like black to catch/fix PEP8 violations:

Suggested change

outputs = [{self.address : parent_value}, {ADDRESS_BCRT1_P2WSH_OP_TRUE : parent_value}]

outputs = [{self.address: parent_value}, {ADDRESS_BCRT1_P2WSH_OP_TRUE: parent_value}]

fa7db1c [test] checks descendants limtis for second generation Package descendants (ritickgoenka) Pull request description: This PR adds a new functional test to test the new descendant limits for packages that were proposed in bitcoin#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) ACKs for top commit: ryanofsky: Code review ACK fa7db1c. Only were suggested changes since last review: simplifying test and dropping P3 transaction as John suggested, and adding assert_equal I suggested glozow: ACK fa7db1c jnewbery: ACK fa7db1c Tree-SHA512: d1eb993550ac8ce31cbe42e17c6522a213ede66970d5d9391f31a116477ab5889fefa6ff2df6ceadd63a28c1be1ad893b0e8e449247e9ade2ca61dc636508d68

Summary: ``` 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. ``` Backport of [[bitcoin/bitcoin#22582 | core#22582]]. Also remove a debug print leftover from D12162. Test Plan: ./test/functional/test_runner.py mempool_package_limits Reviewers: #bitcoin_abc, sdulfari Reviewed By: #bitcoin_abc, sdulfari Subscribers: sdulfari Differential Revision: https://reviews.bitcoinabc.org/D12365

fanquake added the Tests label Jul 29, 2021

DrahtBot mentioned this pull request Jul 29, 2021

Package Mempool Submission with Package Fee-Bumping #22290

Closed

10 tasks

ritickgoenka force-pushed the test_descendant_limit_bushy branch from 68b3c7a to c18188e Compare July 29, 2021 18:40

fanquake requested a review from glozow July 30, 2021 06:01

glozow reviewed Jul 30, 2021

View reviewed changes

test/functional/rpc_packages.py Outdated Show resolved Hide resolved

test/functional/rpc_packages.py Outdated Show resolved Hide resolved

ritickgoenka force-pushed the test_descendant_limit_bushy branch from c18188e to 0dab0ac Compare July 30, 2021 13:12

ritickgoenka marked this pull request as ready for review July 30, 2021 15:25

DrahtBot mentioned this pull request Jul 31, 2021

mempool/validation: mempool ancestor/descendant limits for packages #21800

Merged

DrahtBot added the Needs rebase label Aug 9, 2021

ritickgoenka force-pushed the test_descendant_limit_bushy branch from 0dab0ac to fc42d4f Compare August 10, 2021 16:46

DrahtBot removed the Needs rebase label Aug 10, 2021

ritickgoenka marked this pull request as draft August 10, 2021 17:31

ritickgoenka force-pushed the test_descendant_limit_bushy branch from fc42d4f to 2bc8b5a Compare August 10, 2021 18:35

ritickgoenka marked this pull request as ready for review August 11, 2021 04:10

glozow reviewed Aug 11, 2021

View reviewed changes

test/functional/mempool_package_limits.py Outdated Show resolved Hide resolved

test/functional/mempool_package_limits.py Outdated Show resolved Hide resolved

test/functional/mempool_package_limits.py Outdated Show resolved Hide resolved

ritickgoenka requested a review from glozow August 11, 2021 16:31

ritickgoenka force-pushed the test_descendant_limit_bushy branch from 2bc8b5a to 4fd7979 Compare August 11, 2021 17:25

glozow reviewed Aug 12, 2021

View reviewed changes

test/functional/mempool_package_limits.py Show resolved Hide resolved

test/functional/mempool_package_limits.py Outdated Show resolved Hide resolved

ritickgoenka force-pushed the test_descendant_limit_bushy branch 2 times, most recently from c03c6d3 to 5a7050c Compare August 12, 2021 18:10

glozow reviewed Aug 16, 2021

View reviewed changes

test/functional/mempool_package_limits.py Outdated Show resolved Hide resolved

ritickgoenka force-pushed the test_descendant_limit_bushy branch from 5a7050c to e0f92f3 Compare August 17, 2021 13:00

ritickgoenka force-pushed the test_descendant_limit_bushy branch 2 times, most recently from 97cf2d3 to 2ef27b5 Compare August 19, 2021 16:08

ryanofsky reviewed Aug 19, 2021

View reviewed changes

test/functional/mempool_package_limits.py Show resolved Hide resolved

[test] checks descendants limtis for second generation Package descen…

fa7db1c

…dants

ritickgoenka force-pushed the test_descendant_limit_bushy branch from 2ef27b5 to fa7db1c Compare August 26, 2021 18:11

ritickgoenka requested a review from jnewbery September 5, 2021 16:38

ryanofsky approved these changes Sep 7, 2021

View reviewed changes

fanquake requested a review from glozow September 8, 2021 00:57

glozow reviewed Sep 9, 2021

View reviewed changes

jnewbery reviewed Sep 9, 2021

View reviewed changes

maflcko merged commit fac7181 into bitcoin:master Sep 9, 2021

bitcoin locked and limited conversation to collaborators Oct 30, 2022

	(tx, txhex, value, spk) = make_chain(node, self.address, self.privkeys, txid, value, 0, spk)
	tx, txhex, value, spk = make_chain(node, self.address, self.privkeys, txid, value, 0, spk)

	outputs = [{self.address : parent_value}, {ADDRESS_BCRT1_P2WSH_OP_TRUE : parent_value}]
	outputs = [{self.address: parent_value}, {ADDRESS_BCRT1_P2WSH_OP_TRUE: parent_value}]

test: a test to check descendant limits #22582

test: a test to check descendant limits #22582

Uh oh!

Conversation

ritickgoenka commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Jul 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

tryphe commented Jul 30, 2021

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

glozow commented Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

glozow commented Aug 11, 2021

Uh oh!

Uh oh!

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

glozow commented Aug 16, 2021

Uh oh!

jnewbery commented Aug 16, 2021

Uh oh!

ritickgoenka commented Aug 16, 2021

Uh oh!

jnewbery commented Aug 17, 2021

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

glozow left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Sep 9, 2021

Choose a reason for hiding this comment

Uh oh!

jnewbery Sep 9, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ritickgoenka commented Jul 29, 2021 •

edited

Loading

DrahtBot commented Jul 29, 2021 •

edited

Loading

glozow commented Aug 9, 2021 •

edited

Loading