test: add conflicting topology test case #30066
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
cc @glozow |
| # ... and without the in-mempool ancestor tx1 included in the call | ||
| submitres = node.submitpackage([tx2["hex"], tx_child["hex"]]) | ||
|
|
||
| assert_equal(submitres, {'package_msg': 'package-not-child-with-unconfirmed-parents', 'tx-results': {}, 'replaced-transactions': []}) |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
and specifically, the conflict is happening between a transaction in the mempool already, and a transaction in the package
There was a problem hiding this comment.
that even if topologies
that are acceptable are relaxed, like
removing package-not-child-with-unconfirmed-parents,
Where exactly here is this topology relaxed?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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'
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
instagibbs
force-pushed
the
2024-05-submit-conflicts
branch
from
d612e8c
to
fba1565
Compare
| # ... and without the in-mempool ancestor tx1 included in the call | ||
| submitres = node.submitpackage([tx2["hex"], tx_child["hex"]]) | ||
|
|
||
| assert_equal(submitres, {'package_msg': 'package-not-child-with-unconfirmed-parents', 'tx-results': {}, 'replaced-transactions': []}) |
There was a problem hiding this comment.
that even if topologies
that are acceptable are relaxed, like
removing package-not-child-with-unconfirmed-parents,
Where exactly here is this topology relaxed?
| # ... and without the in-mempool ancestor tx1 included in the call | ||
| submitres = node.submitpackage([tx2["hex"], tx_child["hex"]]) | ||
|
|
||
| assert_equal(submitres, {'package_msg': 'package-not-child-with-unconfirmed-parents', 'tx-results': {}, 'replaced-transactions': []}) |
There was a problem hiding this comment.
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.
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.
instagibbs
force-pushed
the
2024-05-submit-conflicts
branch
from
fba1565
to
9365baa
Compare
|
re-utACK |
|
reACK 9365baa |
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.