BIP324: Cipher suite #25361

pull dhruv wants to merge 5 commits into bitcoin:master from dhruv:bip324-cipher-suite changing 19 files +1012 −597
  1. dhruv commented at 3:40 PM on June 13, 2022: contributor

    This PR supersedes #20962 and introduces a two-layered cipher suite used in the latest draft of BIP324.

    • Inner layer uses RFC8439 which comes with a formal security analysis so any novelty introduced by our cipher suite still offers a baseline confidence in confidentiality and authenticity. The RFC8439 instance is re-keyed every 256 messages for forward secrecy.
    • Outer layer uses a forward secure version of ChaCha20, FSChaCha20 which re-keys itself every 256 messages for forward secrecy. It is used to encrypt the message length resulting in a pseudorandom byte stream.

    The dependency tree for BIP324 PRs is here.

  2. dhruv cross-referenced this on Jun 13, 2022 from issue Alter the ChaCha20Poly1305@Bitcoin AEAD to the new specification by jonasschnelli
  3. fanquake added the label P2P on Jun 13, 2022
  4. dhruv added this to the "Needs review" column in a project

  5. DrahtBot added the label Needs rebase on Jun 14, 2022
  6. dhruv force-pushed on Jun 25, 2022
  7. dhruv commented at 3:51 AM on June 25, 2022: contributor

    Rebased

  8. dhruv force-pushed on Jun 25, 2022
  9. DrahtBot removed the label Needs rebase on Jun 25, 2022
  10. DrahtBot commented at 4:16 PM on June 25, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process. A summary of reviews will appear here.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #27479 (BIP324: ElligatorSwift integrations by sipa)
    • #26684 (bench: add readblock benchmark by andrewtoth)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  11. DrahtBot cross-referenced this on Jun 25, 2022 from issue refactor: use std:: prefix for std lib funcs by fanquake
  12. DrahtBot cross-referenced this on Jun 25, 2022 from issue [Fuzz] Poly1305 differential fuzzing against Floodyberry's implementation by prakash1512
  13. in src/test/crypto_tests.cpp:250 in 340c1ca331 outdated
     173 | +        c20.SetRFC8439Nonce(nonce);
     174 | +        c20.SeekRFC8439(seek);
     175 | +        std::vector<unsigned char> keystream;
     176 | +        keystream.resize(CHACHA20_ROUND_OUTPUT);
     177 | +        c20.Keystream(keystream.data(), CHACHA20_ROUND_OUTPUT);
     178 | +        BOOST_CHECK_EQUAL(HexStr(keystream).substr(0, hex_expected_keystream.size()), hex_expected_keystream);
    


    stratospher commented at 8:13 AM on June 28, 2022:

    is there any particular reason why the entire CHACHA20_ROUND_OUTPUT(64 bytes of keystream) is not being compared? for example, the test vectors in L571-L576 compare only first 32 bytes of keystream.


    dhruv commented at 2:44 AM on July 7, 2022:

    No reason other than I'm using the test vectors as-is from the hyperlink in the comment before those vectors.

  14. dhruv force-pushed on Jun 30, 2022
  15. dhruv commented at 4:15 AM on June 30, 2022: contributor

    Found necessary fixes from sanitizers, fuzz tests and unit tests in downstream branches. Pushed those changes. Ready for further review.

  16. dhruv cross-referenced this on Jun 30, 2022 from issue BIP324: Add encrypted p2p transport {de}serializer by dhruv
  17. DrahtBot cross-referenced this on Jun 30, 2022 from issue [crypto] Reduce wasted pseudorandom bytes in ChaCha20 by dhruv
  18. dhruv force-pushed on Jul 6, 2022
  19. dhruv commented at 7:44 PM on July 6, 2022: contributor

    Updated to correctly rekey using CSHA256 instead of CHash256. Thanks for the catch, @stratospher !

  20. dhruv cross-referenced this on Jul 18, 2022 from issue BIP324: Handshake prerequisites by dhruv
  21. DrahtBot cross-referenced this on Jul 25, 2022 from issue crypto: avoid potential buffer overread in `ChaCha20::SetKey` by theStack
  22. DrahtBot cross-referenced this on Jul 25, 2022 from issue tidy: add modernize-use-using by fanquake
  23. DrahtBot cross-referenced this on Jul 26, 2022 from issue crypto: drop 16 byte key support for ChaCha20 by theStack
  24. dhruv cross-referenced this on Aug 5, 2022 from issue Update ChaCha20 to RFC 8439? by Sjors
  25. DrahtBot cross-referenced this on Aug 8, 2022 from issue refactor: Extract MIB_BYTES constant for init.cpp by Empact
  26. DrahtBot cross-referenced this on Aug 23, 2022 from issue Use static member functions from class instead of instances by aureleoules
  27. dhruv force-pushed on Sep 11, 2022
  28. dhruv commented at 8:17 PM on September 11, 2022: contributor

    Updated to:

    • Expose the RFC8439 AAD via the BIP324CipherSuite interface.
    • Remove multiple plaintext interface from RFC8439 -- it ended up not being super helpful to get to the optimization I was trying
    • No longer expose the mac tag from RFC8439 - the tag is a detail internal to the AEAD and the ciphertext + mac is now treated as a blob

    Ready for further review.

  29. dhruv cross-referenced this on Sep 12, 2022 from issue BIP324: Enable v2 P2P encrypted transport by dhruv
  30. in src/crypto/chacha20.h:53 in 9d9ffc4a59 outdated
      44 | @@ -39,4 +45,48 @@ class ChaCha20
      45 |      void Crypt(const unsigned char* input, unsigned char* output, size_t bytes);
      46 |  };
      47 |  
      48 | +class FSChaCha20
      49 | +{
      50 | +private:
      51 | +    ChaCha20 c20;
      52 | +    size_t rekey_interval;
      53 | +    uint32_t messages_with_key;
    


    stratospher commented at 8:08 AM on September 18, 2022:

    9d9ffc4: naming it as a counter would be more intuitive.

        uint32_t message_counter;
    

    dhruv commented at 4:32 AM on September 29, 2022:

    Fixed.

  31. in src/crypto/bip324_suite.cpp:62 in 361e971893 outdated
      57 | +bool BIP324CipherSuite::Crypt(const Span<const std::byte> input, Span<std::byte> output,
      58 | +                              BIP324HeaderFlags& flags, bool encrypt)
      59 | +{
      60 | +    // check buffer boundaries
      61 | +    if (
      62 | +        // if we encrypt, make sure the destination has the space for the length field, header, ciphertext and MAC
    


    stratospher commented at 8:11 AM on September 18, 2022:

    361e9718:

            // if we encrypt, make sure the destination has the space for the length field, header, payload ciphertext and MAC
    

    dhruv commented at 4:32 AM on September 29, 2022:

    Updated.

  32. in src/crypto/bip324_suite.cpp:76 in 361e971893 outdated
      71 | +        // output will be encrypted length + encrypted (header and payload) + mac tag
      72 | +        uint32_t ciphertext_len = BIP324_HEADER_LEN + input.size();
      73 | +        WriteLE32(reinterpret_cast<unsigned char*>(&ciphertext_len), ciphertext_len);
      74 | +
      75 | +        std::vector<std::byte> input_vec;
      76 | +        input_vec.resize(BIP324_HEADER_LEN + input.size());
    


    stratospher commented at 8:13 AM on September 18, 2022:

    361e971: input_vec can be defined directly with the required size.


    dhruv commented at 4:32 AM on September 29, 2022:

    Fixed.

  33. in src/crypto/bip324_suite.h:48 in 701a967fde outdated
      43 | +
      44 | +public:
      45 | +    BIP324CipherSuite(const BIP324Key& K_L, const BIP324Key& K_P, const BIP324Key& rekey_salt)
      46 | +        : fsc20{K_L, rekey_salt, REKEY_INTERVAL},
      47 | +          rekey_ctr{0},
      48 | +          msg_ctr{0},
    


    aureleoules commented at 11:49 AM on September 21, 2022:

    use class initializer instead


    dhruv commented at 4:32 AM on September 29, 2022:

    Fixed.

  34. in src/crypto/bip324_suite.h:21 in 701a967fde outdated
      16 | +static constexpr size_t BIP324_HEADER_LEN = 1;       // bytes
      17 | +static constexpr size_t BIP324_LENGTH_FIELD_LEN = 3; // bytes
      18 | +static constexpr size_t REKEY_INTERVAL = 256;        // messages
      19 | +static constexpr size_t NONCE_LENGTH = 12;           // bytes
      20 | +
      21 | +enum BIP324HeaderFlags : uint8_t {
    


    aureleoules commented at 12:44 PM on September 21, 2022:

    maybe use the scoped enum class instead?


    dhruv commented at 4:31 AM on September 29, 2022:

    It's useful to have the implicit type conversion here for fuzz testing as the header flags are a byte that a peer could stuff with arbitrary bits.

  35. in src/crypto/chacha20.h:74 in 701a967fde outdated
      69 | +               const std::array<std::byte, FSCHACHA20_KEYLEN>& rekey_salt,
      70 | +               size_t rekey_interval)
      71 | +        : c20{reinterpret_cast<const unsigned char*>(key.data()), FSCHACHA20_KEYLEN},
      72 | +          rekey_interval{rekey_interval},
      73 | +          messages_with_key{0},
      74 | +          rekey_counter{0},
    


    aureleoules commented at 12:52 PM on September 21, 2022:

    use class initializer


    dhruv commented at 4:31 AM on September 29, 2022:

    Fixed.

  36. in src/bench/rfc8439.cpp:8 in 361e971893 outdated
       4 | @@ -5,6 +5,7 @@
       5 |  #include <assert.h>
       6 |  #include <bench/bench.h>
       7 |  #include <crypto/rfc8439.h>
       8 | +#include <span.h>
    


    stratospher commented at 6:48 PM on September 21, 2022:

    361e971: changes in src/bench/rfc8439.cpp, src/crypto/rfc8439.h, src/test/fuzz/crypto_rfc8439.cpp can be moved to the previous previous commit. (a2a038a)


    dhruv commented at 4:31 AM on September 29, 2022:

    Done where applicable.

  37. DrahtBot cross-referenced this on Sep 22, 2022 from issue Reduce wasted pseudorandom bytes in ChaCha20 + various improvements by sipa
  38. DrahtBot cross-referenced this on Sep 23, 2022 from issue bench: add "priority level" to the benchmark framework by furszy
  39. DrahtBot cross-referenced this on Sep 24, 2022 from issue build: Remove `stdlib.h` from header checks by fanquake
  40. DrahtBot added the label Needs rebase on Sep 25, 2022
  41. dhruv force-pushed on Sep 29, 2022
  42. dhruv commented at 4:33 AM on September 29, 2022: contributor

    Latest push:

  43. DrahtBot removed the label Needs rebase on Sep 29, 2022
  44. dhruv force-pushed on Oct 1, 2022
  45. dhruv commented at 4:40 AM on October 1, 2022: contributor

    Latest push:

    • Changes in working group:
      • Nomenclature changes
      • Encrypted packet length is now just length of contents rather than len(header + contents)
      • Rekey salt is 23 bytes instead of 32 bytes to allow just one SHA256 compression to achieve new_key = SHA256(rekey_salt || old_key)

    Improvements:

    • Cache mid-state when rekeying

    Bug fixes:

    • Found that previous code was using the little endian contents_len to perform encryption. Fixed.
  46. DrahtBot cross-referenced this on Oct 4, 2022 from issue wallet: coverage for receiving txes with same id but different witness data by furszy
  47. DrahtBot cross-referenced this on Oct 6, 2022 from issue fix initialization in FastRandomContext: removes undefined behavior & uninitialized read by martinus
  48. DrahtBot cross-referenced this on Oct 6, 2022 from issue refactor: Nuke policy/fees->mempool circular dependencies by hebasto
  49. DrahtBot cross-referenced this on Oct 10, 2022 from issue test: Remove unused txmempool include from tests by maflcko
  50. DrahtBot added the label Needs rebase on Oct 19, 2022
  51. dhruv force-pushed on Oct 20, 2022
  52. dhruv commented at 10:14 PM on October 20, 2022: contributor

    Rebased. Bringing in upstream changes, ready for further review.

  53. dhruv force-pushed on Oct 21, 2022
  54. dhruv commented at 5:40 AM on October 21, 2022: contributor

    Rebased.

  55. DrahtBot removed the label Needs rebase on Oct 21, 2022
  56. DrahtBot cross-referenced this on Oct 23, 2022 from issue Add pool based memory resource by martinus
  57. DrahtBot added the label Needs rebase on Nov 19, 2022
  58. DrahtBot removed the label Needs rebase on Nov 21, 2022
  59. DrahtBot cross-referenced this on Nov 21, 2022 from issue wallet: bugfix, invalid crypted key "checksum_valid" set by furszy
  60. dhruv force-pushed on Nov 23, 2022
  61. dhruv commented at 5:10 AM on November 23, 2022: contributor

    Bringing in upstream rebase (#26153)

  62. DrahtBot added the label Needs rebase on Nov 30, 2022
  63. dhruv force-pushed on Dec 15, 2022
  64. dhruv commented at 4:50 PM on December 15, 2022: contributor

    Latest push:

    • Rebased upstream changes
    • New rekey ratchet mechanism (forward security) based on ChaCha20 instead of SHA256. BIP changes are WIP and coming soon.
  65. DrahtBot removed the label Needs rebase on Dec 15, 2022
  66. DrahtBot cross-referenced this on Dec 16, 2022 from issue bench: add readblock benchmark by andrewtoth
  67. DrahtBot added the label Needs rebase on Dec 25, 2022
  68. dhruv force-pushed on Jan 12, 2023
  69. dhruv commented at 7:07 PM on January 12, 2023: contributor

    Rebased

  70. DrahtBot removed the label Needs rebase on Jan 12, 2023
  71. DrahtBot cross-referenced this on Jan 12, 2023 from issue doc: Bump copyright years to present (headers only) by maflcko
  72. dhruv force-pushed on Jan 23, 2023
  73. dhruv commented at 10:12 PM on January 23, 2023: contributor

    Rebased changes from #26153

  74. dhruv force-pushed on Feb 2, 2023
  75. dhruv commented at 6:25 AM on February 2, 2023: contributor

    Rebased. Brought in changes from #26153.

  76. DrahtBot cross-referenced this on Feb 7, 2023 from issue test/BIP324: functional tests for v2 P2P encryption by stratospher
  77. DrahtBot added the label Needs rebase on Feb 15, 2023
  78. RFC8439 nonce and counter for ChaCha20 8e04f058a4
  79. RFC8439 implementation and tests 7a9d2fb8cf
  80. Adding forward secure FSChaCha20 acd664e805
  81. dhruv force-pushed on Feb 21, 2023
  82. dhruv commented at 3:31 AM on February 21, 2023: contributor

    Rebased.

  83. DrahtBot removed the label Needs rebase on Feb 21, 2023
  84. in src/crypto/bip324_suite.cpp:28 in 225023d9e3 outdated
      23 | +    for (; n > 0; n--)
      24 | +        ret |= *p1++ ^ *p2++;
      25 | +    return (ret != 0);
      26 | +}
      27 | +
      28 | +#endif // TIMINGSAFE_BCMP
    


    theStack commented at 12:31 AM on March 19, 2023:

    In commit 225023d9e3ea5a89037ef8a4f4404a0fdd3f1cf7: This function is not used anywhere in this file and hence can be removed (or rather moved to rfc8439.cpp, see other review comment).


    dhruv commented at 3:56 AM on March 21, 2023:

    I think I forgot to remove this previously used code. Thanks for pointing it out.

  85. in src/crypto/rfc8439.cpp:25 in 7a9d2fb8cf outdated
      20 | +    for (; n > 0; n--)
      21 | +        ret |= *p1++ ^ *p2++;
      22 | +    return (ret != 0);
      23 | +}
      24 | +
      25 | +#endif // RFC8439_TIMINGSAFE_BCMP
    


    theStack commented at 12:52 AM on March 19, 2023:

    In commit 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620: Is there any reason to prefix the define and the function with "rfc8439"? Note that the point here is to let the build system detect if the function timingsafe_bcmp is available on the system (if yes, the define HAVE_TIMINGSAFE_BCMP is set) and only use the custom implementation if we need to. With this current construct, we would always use the custom one, since RFC8439_TIMINGSAFE_BCMP would obviously never be set. This should probably just be like on master (or in bip324_suite.cpp, see other review comment):

    #ifndef HAVE_TIMINGSAFE_BCMP
    #define HAVE_TIMINGSAFE_BCMP
    
    int timingsafe_bcmp(const unsigned char* b1, const unsigned char* b2, size_t n)
    {
        const unsigned char *p1 = b1, *p2 = b2;
        int ret = 0;
    
        for (; n > 0; n--)
            ret |= *p1++ ^ *p2++;
        return (ret != 0);
    }
    
    #endif // HAVE_TIMINGSAFE_BCMP
    

    dhruv commented at 3:57 AM on March 21, 2023:

    Done. Although I did leave it as-is in the second commit and addressed it in the fourth commit because I was getting a linker error for multiple definitions trying it on the second commit. The net change isn't different however.


    fanquake commented at 7:34 PM on March 21, 2023:

    The net change isn't different however.

    No, the net change is you've pointlessly broken things like git bisect. Please try and avoid doing that.


    dhruv commented at 8:13 PM on March 21, 2023:

    @fanquake not sure I understand. At which commit is the build broken? I specifically did it in a way that avoided that but may be misunderstanding something.


    fanquake commented at 8:24 PM on March 21, 2023:

    @dhruv ah, my misread was that you'd left the linker errors between commits 2 & 4. Although, I think introducing this, just to change in later commits is still not ideal. Will re-mark this as resolved, and post new review below.

  86. BIP324 Cipher Suite 187761fc8a
  87. Allow for RFC8439 AD in cipher suite interface 8405856ed9
  88. dhruv force-pushed on Mar 21, 2023
  89. dhruv commented at 3:58 AM on March 21, 2023: contributor

    Addressed review by @theStack.

  90. in src/crypto/rfc8439.cpp:33 in 7a9d2fb8cf outdated
      28 | +{
      29 | +    return (len % 16 == 0) ? len : (len / 16 + 1) * 16;
      30 | +}
      31 | +
      32 | +void ComputeRFC8439Tag(const std::array<std::byte, POLY1305_KEYLEN>& polykey,
      33 | +                       Span<const std::byte> aad, Span<const std::byte> ciphertext,
    


    theStack commented at 7:03 PM on March 21, 2023:

    nit: const-correctness

                           const Span<const std::byte> aad, const Span<const std::byte> ciphertext,
    
  91. in src/crypto/rfc8439.cpp:60 in 7a9d2fb8cf outdated
      55 | +    std::array<std::byte, POLY1305_KEYLEN> polykey;
      56 | +    c20.Keystream(reinterpret_cast<unsigned char*>(polykey.data()), POLY1305_KEYLEN);
      57 | +    return polykey;
      58 | +}
      59 | +
      60 | +void RFC8439Crypt(ChaCha20& c20, const Span<const std::byte> in_bytes, Span<std::byte> out_bytes)
    


    theStack commented at 7:04 PM on March 21, 2023:

    nit: seems like this function is never needed in another module, also not in follow-up PRs (checked at #24545)

    static void RFC8439Crypt(ChaCha20& c20, const Span<const std::byte> in_bytes, Span<std::byte> out_bytes)
    
  92. theStack commented at 7:04 PM on March 21, 2023: contributor

    Addressed review by @theStack.

    Thanks! Another (non-blocking) suggestion: considering that the keys for RFC8439 have a fixed size, would it be worth it to introduce a RFC8439Key type that uses std::array<std::byte, RFC8439_KEYLEN>? I feel that's the slightly cleaner interface with enforcing the length already at compile-time, and we could remove the asserts in the RFC8439Encrypt/RFC8439Decrypt functions. The only annoying part is that the test/benchmark/fuzz code gets a bit longer, as one needs to convert from vector to array, and for the zero_key, one can't use the comfortable constructor for initializing the object with a repeated count of items.

  93. in src/crypto/rfc8439.cpp:15 in 7a9d2fb8cf outdated
      10 | +#include <cstring>
      11 | +
      12 | +#ifndef RFC8439_TIMINGSAFE_BCMP
      13 | +#define RFC8439_TIMINGSAFE_BCMP
      14 | +
      15 | +int rfc8439_timingsafe_bcmp(const unsigned char* b1, const unsigned char* b2, size_t n)
    


    fanquake commented at 10:36 AM on March 22, 2023:

    In 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620: It is fairly awkward to introduce a second copy of timingsafe_bcmp (one already in chacha_poly_aead), with a different name, and broken #ifdef logic, just to then rename it when you delete the original timingsafe_bcmp in a later commit. Although I guess re-using/extracting timingsafe_bcmp early from the to-be-deleted file is also a bit awkward.

  94. in src/crypto/rfc8439.h:19 in 7a9d2fb8cf outdated
      14 | +
      15 | +constexpr static size_t RFC8439_KEYLEN = 32;
      16 | +
      17 | +void RFC8439Encrypt(const Span<const std::byte> aad, const Span<const std::byte> key, const std::array<std::byte, 12>& nonce, const Span<const std::byte> plaintext, Span<std::byte> output);
      18 | +
      19 | +// returns false if authentication fails
    


    fanquake commented at 10:37 AM on March 22, 2023:

    In 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620: Make these Doxygen comments?

  95. in src/crypto/rfc8439.h:15 in 8405856ed9
      10 | +
      11 | +#include <array>
      12 | +#include <cstddef>
      13 | +#include <vector>
      14 | +
      15 | +constexpr static size_t RFC8439_KEYLEN = 32;
    


    fanquake commented at 10:39 AM on March 22, 2023:

    In 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620: These could be

    constexpr static size_t RFC8439_KEYLEN{32};
    
  96. in src/bench/rfc8439.cpp:5 in 8405856ed9
       0 | @@ -0,0 +1,76 @@
       1 | +// Copyright (c) 2022 The Bitcoin Core developers
       2 | +// Distributed under the MIT software license, see the accompanying
       3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
       4 | +
       5 | +#include <assert.h>
    


    fanquake commented at 10:40 AM on March 22, 2023:

    In 7a9d2fb8cf11eef8b6bd87ad1066df374bf3b620: nit use c++ headers, i.e <cassert>

  97. in src/bench/bip324_suite.cpp:4 in 8405856ed9
       0 | @@ -0,0 +1,117 @@
       1 | +// Copyright (c) 2019-2020 The Bitcoin Core developers
       2 | +// Distributed under the MIT software license, see the accompanying
       3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
       4 | +
    


    fanquake commented at 10:42 AM on March 22, 2023:

    nit: additional newline

  98. in src/bench/bip324_suite.cpp:9 in 8405856ed9
       0 | @@ -0,0 +1,117 @@
       1 | +// Copyright (c) 2019-2020 The Bitcoin Core developers
       2 | +// Distributed under the MIT software license, see the accompanying
       3 | +// file COPYING or http://www.opensource.org/licenses/mit-license.php.
       4 | +
       5 | +
       6 | +#include <assert.h>
       7 | +#include <bench/bench.h>
       8 | +#include <crypto/bip324_suite.h>
       9 | +#include <crypto/rfc8439.h> // for the RFC8439_EXPANSION constant
    


    fanquake commented at 10:43 AM on March 22, 2023:

    Please don't add for xyz comments. They are unmaintainable, and if someone wants to know why an include is used, they can look at IWYU etc. Note that you could also add newly-added files to ci/test/06-script_b.sh to have the includes checked.

  99. in src/crypto/bip324_suite.cpp:54 in 8405856ed9
      49 | +        uint32_t contents_len = input.size();
      50 | +        WriteLE32(reinterpret_cast<unsigned char*>(&contents_len), contents_len);
      51 | +
      52 | +        std::vector<std::byte> header_and_contents(BIP324_HEADER_LEN + input.size());
      53 | +
      54 | +        memcpy(header_and_contents.data(), &flags, BIP324_HEADER_LEN);
    


    fanquake commented at 10:47 AM on March 22, 2023:
            std::memcpy(header_and_contents.data(), &flags, BIP324_HEADER_LEN);
    

    Same for memset etc.

  100. in src/crypto/chacha20.cpp:145 in 8e04f058a4 outdated
     141 | @@ -127,7 +142,7 @@ inline void ChaCha20Aligned::Keystream64(unsigned char* c, size_t blocks)
     142 |          x15 += j15;
     143 |  
     144 |          ++j12;
     145 | -        if (!j12) ++j13;
     146 | +        if (!j12 && !is_rfc8439) ++j13;
    


    sipa commented at 2:40 PM on March 24, 2023:

    I don't think there is a need for these is_rfc8439 values, because overflowing the block counter in RFC8439 mode is simply not allowed. If it happens, both incrementing j13 or not incrementing it are wrong. If you want to do anything, it should be an assertion failure.

  101. in src/bench/rfc8439.cpp:18 in 7a9d2fb8cf outdated
      13 | +static constexpr uint64_t AAD_SIZE = 32;
      14 | +static constexpr uint64_t PLAINTEXT_SIZE_TINY = 64;
      15 | +static constexpr uint64_t PLAINTEXT_SIZE_SMALL = 256;
      16 | +static constexpr uint64_t PLAINTEXT_SIZE_LARGE = 1024 * 1024;
      17 | +
      18 | +static std::vector<std::byte> zero_key(32, std::byte{0x00});
    


    sipa commented at 2:41 PM on March 24, 2023:

    I think this variable can be constant (and if it isn't, it probably shouldn't be a global). Also, globals should be upper case.

  102. in src/bench/rfc8439.cpp:20 in 7a9d2fb8cf outdated
      15 | +static constexpr uint64_t PLAINTEXT_SIZE_SMALL = 256;
      16 | +static constexpr uint64_t PLAINTEXT_SIZE_LARGE = 1024 * 1024;
      17 | +
      18 | +static std::vector<std::byte> zero_key(32, std::byte{0x00});
      19 | +static std::vector<std::byte> aad(AAD_SIZE, std::byte{0x00});
      20 | +std::array<std::byte, 12> nonce = {std::byte{0x00}, std::byte{0x01}, std::byte{0x02}, std::byte{0x03},
    


    sipa commented at 2:42 PM on March 24, 2023:

    static const here as well?

  103. DrahtBot cross-referenced this on Apr 18, 2023 from issue BIP324: ElligatorSwift integrations by sipa
  104. fanquake commented at 11:11 AM on May 6, 2023: member

    Closing for now. This will be picked up again later. BIP324 review attention should be directed towards #27479 and https://github.com/bitcoin-core/secp256k1/pull/1129.

  105. fanquake closed this on May 6, 2023

  106. sipa cross-referenced this on May 12, 2023 from issue BIP324 tracking issue by sipa
  107. sipa cross-referenced this on Jun 27, 2023 from issue Add support for RFC8439 variant of ChaCha20 by sipa
  108. sipa cross-referenced this on Jun 28, 2023 from issue Make poly1305 support incremental computation + modernize by sipa
  109. sipa cross-referenced this on Jun 29, 2023 from issue BIP324 ciphersuite by sipa
  110. achow101 referenced this in commit b4794740f8 on Jul 12, 2023
  111. sidhujag referenced this in commit 4adf185bcf on Jul 12, 2023
  112. achow101 referenced this in commit 306157ae92 on Jul 17, 2023
  113. sidhujag referenced this in commit 177a9921d0 on Jul 19, 2023
  114. fanquake referenced this in commit b2ec0326fd on Aug 10, 2023
  115. bitcoin locked this on May 5, 2024

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-05-20 06:53 UTC