test: check that loading snapshot not matching AssumeUTXO parameters fails #28625
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
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. |
|
ACK 2e31250 |
|
utACK 2e31250 |
|
ACK 2e31250 |
maflcko
reviewed
Oct 12, 2023
| f.write(bytes.fromhex(bad_snapshot_block_hash)[::-1] + valid_snapshot_contents[32:]) | ||
|
|
||
| expected_log = f"assumeutxo height in snapshot metadata not recognized ({bad_snapshot_height}) - refusing to load snapshot" | ||
| with self.nodes[1].assert_debug_log(expected_log): |
There was a problem hiding this comment.
This is wrong. assert_debug_log accepts an array of strings, not a string. Otherwise it will search for any single character in the string, which will always pass.
There was a problem hiding this comment.
Good catch. Unfortunately, I just noticed that the exact same mistake also happened in #28634 😱 . Given that this is really hard to catch (in total five out of six reviewers didn't see it), it may make sense to assert the type in assert_debug_log to avoid bugs like that in the future.
There was a problem hiding this comment.
Also, this isn't the first issue. It keeps coming back in variants. Longer term a type safe language can be considered. However, in the short term a hack on top of python could make sense (mypy or manual asserts)?
theStack
deleted the
202310-test-assumeutxo_failed_load_param_mismatch
branch
theStack
added a commit
to theStack/bitcoin
that referenced
this pull request
Oct 12, 2023
Two recently added tests (PR bitcoin#28625 / commit 2e31250 and PR bitcoin#28634 / commit 3bb51c2) introduced a bug by wrongly using the `assert_debug_log` helper. Instead of passing the expected debug string in a list as expected, it was passed as bare string, which is then interpretered as a list of characters, very likely leading the debug log assertion pass even if the intended message is not appearing. In order to avoid bugs like this in the future, enforce that the `{un}expected_msgs` parameters are lists.
theStack
added a commit
to theStack/bitcoin
that referenced
this pull request
Oct 13, 2023
Two recently added tests (PR bitcoin#28625 / commit 2e31250 and PR bitcoin#28634 / commit 3bb51c2) introduced a bug by wrongly using the `assert_debug_log` helper. Instead of passing the expected debug string in a list as expected, it was passed as bare string, which is then interpretered as a list of characters, very likely leading the debug log assertion pass even if the intended message is not appearing. In order to avoid bugs like this in the future, enforce that the `{un}expected_msgs` parameters are lists.
achow101
added a commit
to bitcoin-core/gui
that referenced
this pull request
Oct 13, 2023
…ugs, add type checks ac4caf3 test: fix `assert_debug_log` call-site bugs, add type checks (Sebastian Falbesoner) Pull request description: Two recently added tests (PR #28625 / commit 2e31250 and PR #28634 / commit 3bb51c2) introduced bugs by wrongly using the `assert_debug_log` helper: https://github.com/bitcoin/bitcoin/blob/5ea4fc05edde66c5c90383bc054590dfbdb2b645/test/functional/feature_assumeutxo.py#L84-L85 (already fixed in bitcoin/bitcoin#28639) https://github.com/bitcoin/bitcoin/blob/5ea4fc05edde66c5c90383bc054590dfbdb2b645/test/functional/p2p_v2_transport.py#L148 https://github.com/bitcoin/bitcoin/blob/5ea4fc05edde66c5c90383bc054590dfbdb2b645/test/functional/p2p_v2_transport.py#L159 Instead of passing the expected debug string in a list as expected, it was passed as bare string, which is then interpretered as a list of characters, very likely leading the debug log assertion pass even if the intended message is not appearing. Thanks to maflcko for discovering: bitcoin/bitcoin#28625 (comment) In order to avoid bugs like this in the future, enforce that the `{un}expected_msgs` parameters are lists, as discussed in bitcoin/bitcoin#28625 (comment). Using mypy might be an alternative, but I guess it takes quite a bit of effort to properly integrate this into CI for the whole functional test suite (including taking care of false-positives), so I decided to go with the simpler "manual asserts" hack. Suggestions are very welcome of course. ACKs for top commit: achow101: ACK ac4caf3 maflcko: lgtm ACK ac4caf3 dergoegge: ACK ac4caf3 Tree-SHA512: a9677af76a0c370e71f0411339807b1dc6b2a81763db4ec049cd6d766404b916e2bdd002883db5a79c9c388d7d8ebfcbd5f31d43d50be868eeb928e3c906a746
Frank-GER
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 13, 2023
…ssumeUTXO parameters fails
Frank-GER
pushed a commit
to syscoin/syscoin
that referenced
this pull request
Oct 21, 2023
… type checks ac4caf3 test: fix `assert_debug_log` call-site bugs, add type checks (Sebastian Falbesoner) Pull request description: Two recently added tests (PR bitcoin#28625 / commit 2e31250 and PR bitcoin#28634 / commit 3bb51c2) introduced bugs by wrongly using the `assert_debug_log` helper: https://github.com/bitcoin/bitcoin/blob/5ea4fc05edde66c5c90383bc054590dfbdb2b645/test/functional/feature_assumeutxo.py#L84-L85 (already fixed in bitcoin#28639) https://github.com/bitcoin/bitcoin/blob/5ea4fc05edde66c5c90383bc054590dfbdb2b645/test/functional/p2p_v2_transport.py#L148 https://github.com/bitcoin/bitcoin/blob/5ea4fc05edde66c5c90383bc054590dfbdb2b645/test/functional/p2p_v2_transport.py#L159 Instead of passing the expected debug string in a list as expected, it was passed as bare string, which is then interpretered as a list of characters, very likely leading the debug log assertion pass even if the intended message is not appearing. Thanks to maflcko for discovering: bitcoin#28625 (comment) In order to avoid bugs like this in the future, enforce that the `{un}expected_msgs` parameters are lists, as discussed in bitcoin#28625 (comment). Using mypy might be an alternative, but I guess it takes quite a bit of effort to properly integrate this into CI for the whole functional test suite (including taking care of false-positives), so I decided to go with the simpler "manual asserts" hack. Suggestions are very welcome of course. ACKs for top commit: achow101: ACK ac4caf3 maflcko: lgtm ACK ac4caf3 dergoegge: ACK ac4caf3 Tree-SHA512: a9677af76a0c370e71f0411339807b1dc6b2a81763db4ec049cd6d766404b916e2bdd002883db5a79c9c388d7d8ebfcbd5f31d43d50be868eeb928e3c906a746
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds test coverage for the failed loading of an AssumeUTXO snapshot in case the referenced block hash doesn't match the parameters in the chainparams. Right now, I expect this would be the most common error-case for
loadtxoutsetout in the wild, as for mainnet them_assumeutxo_datamap is empty and this error condition would obviously always be triggered for any (otherwise valid, correctly encoded) snapshot. Note that this test-case is the simplest scenario and doesn't cover any of the TODO ideas mentioned at the top of the functional test yet.