test: BIP324: add check for detection of missing garbage terminator#28634
Merged
fanquake merged 1 commit intobitcoin:masterfrom Oct 12, 2023
Conversation
Contributor
|
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. |
theStack
force-pushed
the
202310-test-bip324-check_for_missing_garbage_terminator
branch
from
a05cf51 to
68c51db
Compare
maflcko
reviewed
Oct 11, 2023
theStack
force-pushed
the
202310-test-bip324-check_for_missing_garbage_terminator
branch
from
68c51db to
3bb51c2
Compare
Member
|
utACK 3bb51c2 |
theStack
deleted the
202310-test-bip324-check_for_missing_garbage_terminator
branch
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
that referenced
this pull request
Oct 13, 2023
…hecks 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 #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: #28625 (comment) In order to avoid bugs like this in the future, enforce that the `{un}expected_msgs` parameters are lists, as discussed in #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
… garbage terminator 3bb51c2 test: BIP324: add check for missing garbage terminator detection (Sebastian Falbesoner) Pull request description: This PR adds test coverage for the "missing garbage terminator" detection on incoming v2 transport (BIP324) connections: https://github.com/bitcoin/bitcoin/blob/04265ba9378efbd4c35b33390b1e5cf246d420a9/src/net.cpp#L1205-L1209 Note that this always happens at the same exact amount of bytes sent in (after 64 + 4095 + 16 = 4175 bytes), if at no point, the last 16 bytes of potential authentication data match the garbage, i.e. all the previous bytes after the ellswift pubkey. To keep it simple, we just send in zero-value bytes here and verify that the detection hits exactly after the last bytes is sent. AFAICT, with this PR all the v2 transport errors that can be triggered in this simple way of "just open a socket and send in a fixed byte-string" are covered. For more advanced test, we need BIP324 cryptography in the test framework in order to perform a v2 handshake etc. (PRs bitcoin#28374, bitcoin#24748). ACKs for top commit: sipa: utACK 3bb51c2 laanwj: ACK 3bb51c2 Tree-SHA512: f88275061c7c377a3d9f2608452671afc26deb6d5bd5be596de987c7e5042555153ffe681760c33bce2b921ae04e50f349ea0128a677e6443a95a079e52cdc5f
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
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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
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 "missing garbage terminator" detection on incoming v2 transport (BIP324) connections:
bitcoin/src/net.cpp
Lines 1205 to 1209 in 04265ba
Note that this always happens at the same exact amount of bytes sent in (after 64 + 4095 + 16 = 4175 bytes), if at no point, the last 16 bytes of potential authentication data match the garbage, i.e. all the previous bytes after the ellswift pubkey. To keep it simple, we just send in zero-value bytes here and verify that the detection hits exactly after the last bytes is sent.
AFAICT, with this PR all the v2 transport errors that can be triggered in this simple way of "just open a socket and send in a fixed byte-string" are covered. For more advanced test, we need BIP324 cryptography in the test framework in order to perform a v2 handshake etc. (PRs #28374, #24748).