Skip to content

test: python cryptography required for BIP 324 functional tests #28374

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

Merged
merged 8 commits into from
Nov 7, 2023

Conversation

stratospher
Copy link
Contributor

split off from #24748 to keep commits related to cryptography and functional test framework changes separate.

This PR adds python implementation and unit tests for HKDF, ChaCha20, Poly1305, ChaCha20Poly1305 AEAD, FSChaCha20 and FSChaCha20Poly1305 AEAD.

They're based on https://github.com/bitcoin/bips/blob/cc177ab7bc5abcdcdf9c956ee88afd1052053328/bip-0324/reference.py for easy review.

@DrahtBot
Copy link
Contributor

DrahtBot commented Aug 31, 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 theStack, sipa, achow101

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

Conflicts

No conflicts as of last run.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

Concept ACK

Very nice! Left some non-blocking nits below, they all can be seen as potential follow-up material.

@sipa sipa mentioned this pull request Sep 2, 2023
43 tasks
@sipa
Copy link
Member

sipa commented Sep 10, 2023

ACK fb46249. Left some nits.

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

ACK 417eaf8

left two non-blocking nits below

@DrahtBot DrahtBot requested a review from sipa September 11, 2023 10:29
@DrahtBot DrahtBot changed the title test/BIP324: python cryptography required for BIP 324 functional tests test: python cryptography required for BIP 324 functional tests Sep 11, 2023
@maflcko maflcko removed the Tests label Sep 11, 2023
@DrahtBot DrahtBot added the Tests label Sep 11, 2023
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
@stratospher stratospher force-pushed the crypto-v2tests branch 2 times, most recently from 0e6a6ec to a10a9b1 Compare September 12, 2023 14:43
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

ACK a10a9b1

(the CI failure in "CI / Win64 native" seems to be unrelated to the changes in the PR)

stratospher and others added 4 commits September 29, 2023 17:50
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
Co-authored-by: Pieter Wuille <pieter.wuille@gmail.com>
Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

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

re-ACK c534c08

@fanquake
Copy link
Member

fanquake commented Oct 2, 2023

cc @real-or-random

fanquake added a commit that referenced this pull request Oct 12, 2023
…e 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 #28374, #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 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
@sipa
Copy link
Member

sipa commented Nov 7, 2023

utACK c534c08

@DrahtBot DrahtBot removed the request for review from sipa November 7, 2023 13:27
@achow101
Copy link
Member

achow101 commented Nov 7, 2023

ACK c534c08

@achow101 achow101 merged commit 962ea5c into bitcoin:master Nov 7, 2023
kwvg added a commit to kwvg/dash that referenced this pull request Feb 20, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Feb 23, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Feb 24, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Feb 27, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Feb 27, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Feb 29, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Mar 5, 2024
kwvg added a commit to kwvg/dash that referenced this pull request Mar 5, 2024
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Mar 6, 2024
, bitcoin#28267, bitcoin#28374, bitcoin#28100 (p2p primitives)

b60c493 merge bitcoin#28100: more Span<std::byte> modernization & follow-ups (Kittywhiskers Van Gogh)
c2aa01c merge bitcoin#28374: python cryptography required for BIP 324 functional tests (Kittywhiskers Van Gogh)
7c5edf7 merge bitcoin#28267: BIP324 ciphersuite follow-up (Kittywhiskers Van Gogh)
1b1924e merge bitcoin#28008: BIP324 ciphersuite (Kittywhiskers Van Gogh)
ff54219 merge bitcoin#27993: Make poly1305 support incremental computation + modernize (Kittywhiskers Van Gogh)
d7482eb merge bitcoin#27985: Add support for RFC8439 variant of ChaCha20 (Kittywhiskers Van Gogh)

Pull request description:

  ## Additional Information

  * Dependent on #5900

  * Dependent on #5901

  * Without modifications, tests introduced in [bitcoin#28008](bitcoin#28008) will fail due to salt comprising of a fixed string (`bitcoin_v2_shared_secret`) and network bytes ([source](https://github.com/sipa/bitcoin/blob/1c7582ead6e1119899922041c1af2b4169b0bc74/src/bip324.cpp#L39-L40)). Bitcoin uses [`{0xf9, 0xbe, 0xb4, 0xd9}`](https://github.com/sipa/bitcoin/blob/1c7582ead6e1119899922041c1af2b4169b0bc74/src/kernel/chainparams.cpp#L114-L117) for mainnet while Dash uses [`{0xbf, 0x0c, 0x6b, 0xbd}`](https://github.com/dashpay/dash/blob/37f43e4e56de0d510110a7f790df34ea77748dc9/src/chainparams.cpp#L238-L241).
    * The replacement parameters are generated by:
      * Cloning https://github.com/bitcoin/bips (as of this writing, at bitcoin/bips@b3701fa)
      * Editing `bip-0324/reference.py`
        * Changing `NETWORK_MAGIC` to Dash's network magic
      * Running `gen_test_vectors.py` to get a new `packet_encoding_test_vectors.csv`
      * Using [this python script](https://github.com/bitcoin/bitcoin/blob/1c7582ead6e1119899922041c1af2b4169b0bc74/src/test/bip324_tests.cpp#L174-L196) mentioned in a comment in `src/test/bip324_tests.cpp`, generate the values that will be used to replace the ones in `bip324_tests.cpp` (it will print to `stdout` so it's recommended to pipe it to a file)
      * Paste the new values over the old ones

  ## Breaking Changes

  None. Changes are restricted to BIP324 cryptosuite, tests and associated logic.

  ## Checklist:
    _Go over all the following points, and put an `x` in all the boxes that apply._
  - [x] I have performed a self-review of my own code
  - [x] I have commented my code, particularly in hard-to-understand areas **(note: N/A)**
  - [x] I have added or updated relevant unit/integration/functional/e2e tests **(note: N/A)**
  - [x] I have made corresponding changes to the documentation **(note: N/A)**
  - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_

Top commit has no ACKs.

Tree-SHA512: bb056de8588026adae63e78d56878274ff3934a439e2eae81606fa9c0a37dab432a129315bb9c1b754400b5c883bf460eea3a0c857a3be0816c8fbf55c479843
@stratospher stratospher deleted the crypto-v2tests branch July 17, 2024 06:39
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.

9 participants