test: add conflicting topology test case

instagibbs commented at 9:04 pm on May 8, 2024: member

We want to ensure that even if topologies that are acceptable are relaxed, like removing package-not-child-with-unconfirmed-parents, that we don’t end up accepting packages we shouldn’t.

DrahtBot commented at 9:05 pm on May 8, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	glozow, rkrux
Concept ACK	sdaftuar

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 May 8, 2024

instagibbs commented at 9:07 pm on May 8, 2024: member

cc @glozow

in test/functional/rpc_packages.py:267 in d612e8c07c outdated

262+        assert_equal(submitres, {'package_msg': 'conflict-in-package', 'tx-results': {}, 'replaced-transactions': []})
263+
264+        # ... and without the in-mempool ancestor tx1 included in the call
265+        submitres = node.submitpackage([tx2["hex"], tx_child["hex"]])
266+
267+        assert_equal(submitres, {'package_msg': 'package-not-child-with-unconfirmed-parents', 'tx-results': {}, 'replaced-transactions': []})

glozow commented at 9:29 am on May 9, 2024:

i.e. if we relaxed the package-not-child-with-unconfirmed-parents rule, we should still fail because

package parent is conflicting with ancestor of child (what the rule would be)
the child spends 2 conflicting transactions, which makes no sense (what’s actually wrong)

instagibbs commented at 1:52 pm on May 9, 2024:

and specifically, the conflict is happening between a transaction in the mempool already, and a transaction in the package

rkrux commented at 2:20 pm on May 9, 2024:

that even if topologies that are acceptable are relaxed, like removing package-not-child-with-unconfirmed-parents,

Where exactly here is this topology relaxed?

instagibbs commented at 2:27 pm on May 9, 2024:

It hasn’t been, but has been discussed (having trouble finding the discussion on github sorry).

specifically: If we drop package-not-child-with-unconfirmed-parents, then we would allow in-mempool ancestors for packages we submit

rkrux commented at 8:47 am on May 10, 2024:

I see, the part I’m a little confused about is that how can we test for something that’s not done yet. Is the intention to have this test written before the topology is relaxed so that we have more robustness in the tests?

Also, am I correct to assume that this error message would need to be updated when the topology is relaxed later? 'package_msg': 'package-not-child-with-unconfirmed-parents'

instagibbs commented at 1:39 pm on May 10, 2024:

It’s coverage for package-not-child-with-unconfirmed-parents, which only occurs once, semi-incidentally, in test/functional/p2p_opportunistic_1p1c.py. If the restriction is taken out, we know there’s already coverage and can adapt the test to cover the new expected failure, which would be the actual conflict underlying the scenario.

rkrux commented at 7:42 am on May 12, 2024:

Oh I see, it makes sense to me now. I’ll take this as an opportunity to go through test/functional/p2p_opportunistic_1p1c.py as well!

rkrux commented at 7:44 am on May 12, 2024:

However, I do feel that the description can be updated to let others know that this topology will be relaxed in the future and not now.

sdaftuar commented at 12:18 pm on May 14, 2024:

This may be just a matter of personal preference, so feel free to take it or leave it – but if I were writing this I’d drop this line which tests the reason for failure, and also drop line 269 which tests that RBF doesn’t work, in favor of just having the assertion at line 270 below that the child doesn’t make it in. That way, someone changing the behavior in the future is less likely to rip the whole test out (eg if “not child with unconfirmed parents” stops being a rejection reason, or if we were to process a subpackage for validation) and overlook the fact that tx_child should never make it into the mempool, no matter what our policies for processing packages are.

instagibbs commented at 12:38 pm on May 14, 2024:

I’m a little conflicted since without the string checking it’s a little harder to tell what the two cases being checked are there? I do agree that dropping the check for the parent RBF is better.

I pushed a small update to make it a bit harder to have the whole case ripped out by someone reading the test, let me know what you think.

sdaftuar commented at 1:40 pm on May 14, 2024:

Looks good, thanks!

in test/functional/rpc_packages.py:268 in d612e8c07c outdated

263+
264+        # ... and without the in-mempool ancestor tx1 included in the call
265+        submitres = node.submitpackage([tx2["hex"], tx_child["hex"]])
266+
267+        assert_equal(submitres, {'package_msg': 'package-not-child-with-unconfirmed-parents', 'tx-results': {}, 'replaced-transactions': []})
268+

glozow commented at 9:32 am on May 9, 2024:

Perhaps add an assert tx2["txid"] not in node.getrawmempool(). Depending on where the rule is enforced (or how we received them if it’s p2p), tx2 could replace tx1 before we fail :laughing: either way it’d be good for the test to show us what happens.

instagibbs commented at 10:44 am on May 9, 2024:

done

glozow commented at 9:34 am on May 9, 2024: member

concept ACK, 1 suggestion

instagibbs force-pushed on May 9, 2024

glozow commented at 11:21 am on May 9, 2024: member

ACK fba1565f4b6bafbf2516f03184cf58aa80d9843f

in test/functional/rpc_packages.py:266 in fba1565f4b outdated

261+        submitres = node.submitpackage([tx1["hex"], tx2["hex"], tx_child["hex"]])
262+        assert_equal(submitres, {'package_msg': 'conflict-in-package', 'tx-results': {}, 'replaced-transactions': []})
263+
264+        # ... and without the in-mempool ancestor tx1 included in the call
265+        submitres = node.submitpackage([tx2["hex"], tx_child["hex"]])
266+

rkrux commented at 2:22 pm on May 9, 2024:

Super nit: There’s a blank line here b/w the RPC and the assertion & also on line 248, but not b/w 261-262. Consistency here would be nice.

instagibbs commented at 2:29 pm on May 9, 2024:

can do if I touch test again

instagibbs commented at 12:39 pm on May 14, 2024:

done

rkrux commented at 2:23 pm on May 9, 2024: none

I’m still reviewing the PR and gaining some context. For now, make is successful and all the functional tests pass on commit fba1565.

rkrux approved

rkrux commented at 7:45 am on May 12, 2024: none

tACK fba1565

sdaftuar approved

sdaftuar commented at 12:20 pm on May 14, 2024: member

utACK, one style preference/nit for you to consider

test: add conflicting topology test case

We want to ensure that even if topologies
that are acceptable are relaxed, like
removing package-not-child-with-unconfirmed-parents,
that we don't end up accepting packages we shouldn't.

9365baa489

instagibbs force-pushed on May 14, 2024

sdaftuar commented at 1:43 pm on May 14, 2024: member

re-utACK

glozow commented at 12:50 pm on May 15, 2024: member

reACK 9365baa489

DrahtBot requested review from sdaftuar on May 15, 2024

DrahtBot requested review from rkrux on May 15, 2024

sdaftuar approved

rkrux approved

rkrux commented at 1:48 pm on May 17, 2024: none

reACK 9365baa

DrahtBot requested review from sdaftuar on May 17, 2024

glozow merged this on May 20, 2024

glozow closed this on May 20, 2024

test: add conflicting topology test case #30066

Code Coverage

Reviews