-
Notifications
You must be signed in to change notification settings - Fork 37.7k
crypto: more Span<std::byte>
modernization & follow-ups
#28100
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
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. ConflictsNo conflicts as of last run. |
0e53ce9
to
b1faef7
Compare
Span<std::byte>
modernization & follow-upsSpan<std::byte>
modernization & follow-ups
60583fc
to
153a674
Compare
Concept ACK |
There was a problem hiding this 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==
153a674
to
aafafde
Compare
Rebased after merge of #28008, and addresses feedback. I've dropped the |
29fb4ce
to
baf93fb
Compare
@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? |
There was a problem hiding this 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++!
ACK baf93fb 🙉 Show signatureSignature:
|
There was a problem hiding this 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
.
@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. |
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. |
I will address them. |
baf93fb
to
57cc136
Compare
Addressed review comments by @stratospher. Note: new last commit. |
There was a problem hiding this 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)
ACK 57cc136. |
…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
, 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
This modernizes the ChaCha20 and ChaCha20Aligned interfaces to be
Span<std::byte>
based, and other improvements.ChaCha20
andChaCha20Aligned
to beSpan<std::byte>
based (aligning them withFSChaCha20
,AEADChaCha20Poly1305
, andFSChaCha20Poly1305
)HexStr
on byte vectors in tests (suggested in Make poly1305 support incremental computation + modernize #27993 (comment))std::byte
vectors inConsumeRandomLengthByteVector
andConsumeFixedLengthByteVector
, and use it (suggested in Make poly1305 support incremental computation + modernize #27993 (comment))While related, I don't see this as a necessary for BIP324.