Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: BIP324: add check for detection of missing garbage terminator #28634

Conversation

theStack
Copy link
Contributor

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

} else if (m_recv_buffer.size() == MAX_GARBAGE_LEN + BIP324Cipher::GARBAGE_TERMINATOR_LEN) {
// We've reached the maximum length for garbage + garbage terminator, and the
// terminator still does not match. Abort.
LogPrint(BCLog::NET, "V2 transport error: missing garbage terminator, peer=%d\n", m_nodeid);
return false;

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).

@DrahtBot
Copy link
Contributor

DrahtBot commented Oct 10, 2023

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 laanwj, sipa

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

@DrahtBot DrahtBot added the Tests label Oct 10, 2023
@theStack theStack force-pushed the 202310-test-bip324-check_for_missing_garbage_terminator branch from a05cf51 to 68c51db Compare October 10, 2023 20:07
@theStack theStack force-pushed the 202310-test-bip324-check_for_missing_garbage_terminator branch from 68c51db to 3bb51c2 Compare October 11, 2023 09:58
Copy link
Member

@laanwj laanwj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 3bb51c2
Test code looks correct, and similar to other test cases in this unit

@sipa
Copy link
Member

sipa commented Oct 11, 2023

utACK 3bb51c2

@DrahtBot DrahtBot removed the request for review from sipa October 11, 2023 12:59
@fanquake fanquake merged commit 4a5aae9 into bitcoin:master Oct 12, 2023
16 checks passed
@theStack theStack deleted the 202310-test-bip324-check_for_missing_garbage_terminator branch October 12, 2023 08:37
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 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants