test: add conflicting topology test case #30066
pull instagibbs wants to merge 1 commits into bitcoin:master from instagibbs:2024-05-submit-conflicts changing 1 files +32 −0-
instagibbs commented at 9:04 pm on May 8, 2024: memberWe 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.
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: membercc @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 forpackage-not-child-with-unconfirmed-parents
, which only occurs once, semi-incidentally, intest/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 throughtest/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 anassert 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: memberconcept ACK, 1 suggestion
-
instagibbs force-pushed on May 9, 2024
-
glozow commented at 11:21 am on May 9, 2024: memberACK 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 approved
-
sdaftuar approved
-
sdaftuar commented at 12:20 pm on May 14, 2024: memberutACK, 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.
-
instagibbs force-pushed on May 14, 2024
-
sdaftuar commented at 1:43 pm on May 14, 2024: memberre-utACK
-
glozow commented at 12:50 pm on May 15, 2024: memberreACK 9365baa489
-
DrahtBot requested review from sdaftuar on May 15, 2024
-
DrahtBot requested review from rkrux on May 15, 2024
-
sdaftuar approved
-
rkrux approved
-
DrahtBot requested review from sdaftuar on May 17, 2024
-
glozow merged this on May 20, 2024
-
glozow closed this on May 20, 2024
-
bitcoin locked this on May 20, 2025