Skip to content

Conversation

sipa
Copy link
Member

@sipa sipa commented Jul 18, 2023

This modernizes the ChaCha20 and ChaCha20Aligned interfaces to be Span<std::byte> based, and other improvements.

While related, I don't see this as a necessary for BIP324.

@DrahtBot
Copy link
Contributor

DrahtBot commented Jul 18, 2023

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK theStack, stratospher
Stale ACK MarcoFalke

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.

@sipa sipa force-pushed the 202307_crypto_modern branch from 0e53ce9 to b1faef7 Compare July 18, 2023 19:16
@sipa sipa changed the title crypto: ChaCha20 Span<std::byte> modernization & follow-ups crypto: more Span<std::byte> modernization & follow-ups Jul 18, 2023
@sipa sipa force-pushed the 202307_crypto_modern branch from 60583fc to 153a674 Compare July 18, 2023 21:15
@DrahtBot DrahtBot mentioned this pull request Jul 18, 2023
@fanquake fanquake requested a review from theStack August 2, 2023 08:37
@theStack
Copy link
Contributor

theStack commented Aug 8, 2023

Concept ACK

Copy link
Member

@maflcko maflcko left a comment

Choose a reason for hiding this comment

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

nice ACK 153a674 🔖

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: nice ACK 153a6745fa523a3fd0ccce1304ceb75d4511df20 🔖
pMD3fmJquqJLFEdbollqeq+L8nBymi3uLOeQKTb65R/+y65NKCnTWqnk7Flu+t57wNz+Avhshe+x1CU260NoDA==

@sipa
Copy link
Member Author

sipa commented Aug 14, 2023

Rebased after merge of #28008, and addresses feedback. I've dropped the Crypt -> Encrypt / Decrypt change as I felt the duplication that resulted wasn't worth it anymore.

@fanquake
Copy link
Member

Some changes here are also in #28008, but either or both can go in.

@sipa could you update the PR dscription in regards to this, now that #28008 has been merged.

cc @stratospher you might be interested in reviewing here?

@sipa
Copy link
Member Author

sipa commented Aug 14, 2023

@sipa could you update the PR dscription in regards to this, now that #28008 has been merged.

@fanquake Done.

Copy link
Contributor

@stratospher stratospher left a comment

Choose a reason for hiding this comment

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

ACK baf93fb. neat c++!

@DrahtBot DrahtBot requested a review from maflcko August 15, 2023 05:54
@maflcko
Copy link
Member

maflcko commented Aug 15, 2023

ACK baf93fb 🙉

Show signature

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK baf93fbd47058b182643c5a11fe9a8928276b05f  🙉
bVwXQLzRdyPR0w0spuTcbS3DX/txQkFYl41x1X2FfdLOU1CyGgw5vele1a0LWd0FeAIO4nLjR7ZBleZwl4c0Dw==

@DrahtBot DrahtBot removed the request for review from maflcko August 15, 2023 08:24
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.

Code-review ACK baf93fb

Potential follow-up nit: there might be some more instances where using BOOST_CHECK_EQUAL over BOOST_CHECK could make sense, see git grep "BOOST_CHECK(.*==" ./src/test/crypto_tests.cpp.

@fanquake
Copy link
Member

@sipa when you get a chance to respond to review comments here, we can decide to either merge as-is, and address in a followup, or touch up here.

@maflcko
Copy link
Member

maflcko commented Aug 17, 2023

I think they should either be fixed up here or not at all. I don't think it makes sense to create a follow-up to a follow-up.

@sipa
Copy link
Member Author

sipa commented Aug 17, 2023

I will address them.

@sipa sipa force-pushed the 202307_crypto_modern branch from baf93fb to 57cc136 Compare August 17, 2023 19:43
@sipa
Copy link
Member Author

sipa commented Aug 17, 2023

Addressed review comments by @stratospher. Note: new last commit.

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 57cc136

(CI failure is unrelated)

@stratospher
Copy link
Contributor

ACK 57cc136.

@DrahtBot DrahtBot removed the request for review from stratospher August 18, 2023 03:51
@fanquake fanquake merged commit 5eb6690 into bitcoin:master Aug 18, 2023
Frank-GER pushed a commit to syscoin/syscoin that referenced this pull request Sep 8, 2023
…ollow-ups

57cc136 crypto: make ChaCha20::SetKey wipe buffer (Pieter Wuille)
da0ec62 tests: miscellaneous hex / std::byte improvements (Pieter Wuille)
bdcbc85 fuzz: support std::byte in Consume{Fixed,Variable}LengthByteVector (Pieter Wuille)
7d1cd93 crypto: require key on ChaCha20 initialization (Pieter Wuille)
44c1176 random: simplify FastRandomContext::randbytes using fillrand (Pieter Wuille)
3da636e crypto: refactor ChaCha20 classes to use Span<std::byte> interface (Pieter Wuille)

Pull request description:

  This modernizes the ChaCha20 and ChaCha20Aligned interfaces to be `Span<std::byte>` based, and other improvements.

  * Modifies all functions and constructors of `ChaCha20` and `ChaCha20Aligned` to be `Span<std::byte>` based (aligning them with `FSChaCha20`, `AEADChaCha20Poly1305`, and `FSChaCha20Poly1305`)
  * Remove default constructors, to make sure all call sites provide a key (suggested in bitcoin#26153 (comment))
  * Wipe key material on rekey for security (suggested in bitcoin#26153 (comment))
  * Use `HexStr` on byte vectors in tests (suggested in bitcoin#27993 (comment))
  * Support `std::byte` vectors in `ConsumeRandomLengthByteVector` and `ConsumeFixedLengthByteVector`, and use it (suggested in bitcoin#27993 (comment))
  * And a few more.

  While related, I don't see this as a necessary for BIP324.

ACKs for top commit:
  stratospher:
    ACK 57cc136.
  theStack:
    re-ACK 57cc136

Tree-SHA512: 361da4ff003c8465a32eeac0983a8a6f047dbbf5b400168b409c8e3234e79d577fc854e0764389446585da3e12b964c94dd67fc0c9c1d1d092cec296121e05d4
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
@bitcoin bitcoin locked and limited conversation to collaborators Aug 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants