Clean out the BigInt interface #4056

randombit · 2024-05-12T07:12:00Z

This class has grown in an unprincipled way out of accretion and it's quite a mess. Many functions are implemented which are just useful for 1 or2 algorithms. Deprecate everything along these lines.

I don't have an immediate plan for how we'll get access to the functionality later from within the library, but sine we can't remove anything until Botan 4 anyway it doesn't really matter right now.

Also add new serialization and deserialization functions based on span. The new serialize_to avoids the bizarre trunction behavior of binary_encode when the output buffer is too small - I'm not sure why or when that was ever useful.

coveralls · 2024-05-12T08:07:37Z

coverage: 91.812% (+0.003%) from 91.809%
when pulling 24da08f on jack/cleanup-bigint-interface
into 100a065 on master.

reneme · 2024-05-24T13:17:14Z

src/lib/math/bigint/bigint.h

@@ -60,17 +64,20 @@ class BOTAN_PUBLIC_API(2, 0) BigInt final {
       * Create BigInt from a word (limb)
       * @param n initial value of this BigInt
       */
+      //BOTAN_DEPRECATED("Use BigInt::from_u64 instead")


Commented out on purpose? Same with BigInt(std::string_view).

reneme

No full review so far. Just a few suggestions.

reneme · 2024-05-24T13:24:16Z

src/lib/math/bigint/bigint.h

+         // TODO this supports std::vector and secure_vector
+         // it would be nice if this also could work with std::array as in
+         //   bn.serialize_to<std::array<uint8_t, 32>>(32);


Perhaps just add another overload like so:

template<size_t len> std::array<uint8_t, len> serialize() const { std::array<uint8_t, len> out; this->serialize_to(out); return out; }

usage would then look like this:

bn.serialize<32>();

... this style would be similar to the implementation in the BufferSlicer, where one would bs.take<32>().

For call-site readability it might make sense to rename that overload to something like serialize_as_array<len>().

My take at this point is that it's probably quite rare to know at compile time the exact size of the integer being encoded. Can always revisit this issue later, and as always hard to take it back once it is in a release, so I'm going to defer.

reneme · 2024-05-24T13:29:02Z

src/lib/math/bigint/bigint.h

+      /**
+       * Read integer value from a byte vector (big endian)
+       * @param bytes the span of bytes to load
+       */
+      void assign_from_bytes(std::span<const uint8_t> bytes);


Do we even need that? Or is a simple assignment enough?

BigInt some_existing_bn; some_existing_bn = BigInt::from_bytes(some_byte_array);

... with an appropriate move-assignment operator that might not even have a performance penalty.

See also: #4056 (comment)

src/lib/math/bigint/bigint.h

Most of these are only useful internally to implement specific algorithms, and should never have been exposed to users.

Deprecate all of the existing interface.

This allows us to deprecate BigInt::data and still use it in headers/inlines without triggering a warning in user code.

reneme

Looks good to me. Just some nits and a few minor suggestions.

src/lib/math/bigint/big_code.cpp

reneme · 2024-05-27T08:37:37Z

src/lib/math/bigint/big_code.cpp

+   if(base == Binary) {
+      return BigInt::from_bytes(buf);
+   }
+   return BigInt::decode(buf.data(), buf.size(), base);


Perhaps BigInt::decode(std::span<const uint8_t>, ..)?

For these functions using BigInt::Base I'm inclined to not do anything with span since already the whole set of functions and the enum itself are deprecated. We'd just be introducing new interfaces which begin life deprecated, which seems odd.

src/lib/math/bigint/big_code.cpp

src/lib/math/bigint/bigint.cpp

src/lib/pubkey/ec_group/ec_point.cpp

src/lib/pubkey/pubkey.cpp

src/lib/pubkey/rfc6979/rfc6979.cpp

reneme · 2024-05-27T09:24:43Z

src/tests/test_ed448.cpp

         std::array<uint8_t, 56> res_bytes = {0};
-         res.binary_encode(res_bytes.data(), res_bytes.size());
+         res.serialize_to(res_bytes);


For reference: If we add a serialization overload that produces an array, this could become:

auto res_bytes = res.serialize_as_array<56>();

doc/api_ref/bigint.rst

reneme · 2024-05-27T09:30:51Z

src/lib/asn1/ber_dec.cpp

         out.flip_sign();
      } else {
-         out = BigInt(obj.bits(), obj.length());
+         out.assign_from_bytes(obj.data());


There's no way to access a BER_Object's data as a span?

BER_Object::data is exactly this?

(This was just recently added in cf82307)

Co-authored-by: René Meusel <[email protected]>

Interface changes/deprecations were introduced in #4056 and #4038, bump the module version to make it easier for users to handle this if they want to.

randombit force-pushed the jack/cleanup-bigint-interface branch 3 times, most recently from c67df9c to ff12476 Compare May 12, 2024 07:34

randombit force-pushed the jack/cleanup-bigint-interface branch 3 times, most recently from 1a9974e to 79db481 Compare May 12, 2024 14:24

randombit added this to the Botan 3.5.0 milestone May 22, 2024

reneme reviewed May 24, 2024

View reviewed changes

randombit added 3 commits May 24, 2024 18:19

Deprecate a wide range of BigInt functions

528c5fc

Most of these are only useful internally to implement specific algorithms, and should never have been exposed to users.

Cleanup BigInt::operator<< a little

af5bf2c

Add a new set of BigInt serialization/deserialization functions

596fb93

Deprecate all of the existing interface.

randombit force-pushed the jack/cleanup-bigint-interface branch from 79db481 to e152c76 Compare May 24, 2024 22:50

randombit added 3 commits May 24, 2024 18:54

Update callers within the library to use the new BigInt APIs

bc5137c

Update BigInt docs

18a5c39

Add BigInt::_data

ae49fef

This allows us to deprecate BigInt::data and still use it in headers/inlines without triggering a warning in user code.

randombit force-pushed the jack/cleanup-bigint-interface branch from e152c76 to ae49fef Compare May 24, 2024 22:54

reneme approved these changes May 27, 2024

View reviewed changes

randombit force-pushed the jack/cleanup-bigint-interface branch from bb10382 to b5498fd Compare May 29, 2024 07:54

randombit and others added 2 commits May 29, 2024 03:54

Apply some suggestions from review

b5498fd

Co-authored-by: René Meusel <[email protected]>

Make assign_from_bytes private

24da08f

randombit mentioned this pull request May 29, 2024

Somehow load_{be,le} of integer without offset doesn't work #4080

Closed

randombit merged commit e101afd into master May 29, 2024
43 checks passed

randombit deleted the jack/cleanup-bigint-interface branch May 29, 2024 10:03

reneme mentioned this pull request May 29, 2024

Follow-up: Use load_be more efficiently #4085

Merged

randombit mentioned this pull request Jun 5, 2024

[std::span] BigInt - use std::span<> in some internal implementations #3855

Closed

randombit added a commit that referenced this pull request Jun 15, 2024

Bump module versions for bigint and ec_group

a8a2c57

Interface changes/deprecations were introduced in #4056 and #4038, bump the module version to make it easier for users to handle this if they want to.

randombit mentioned this pull request Jun 15, 2024

Bump module versions for bigint and ec_group #4128

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clean out the BigInt interface #4056

Clean out the BigInt interface #4056

randombit commented May 12, 2024

coveralls commented May 12, 2024 •

edited

Loading

reneme May 24, 2024 •

edited

Loading

reneme left a comment

reneme May 24, 2024

randombit May 29, 2024

reneme May 24, 2024

reneme May 27, 2024

reneme left a comment

reneme May 27, 2024

randombit May 29, 2024

reneme May 27, 2024

reneme May 27, 2024

randombit May 29, 2024

Clean out the BigInt interface #4056

Clean out the BigInt interface #4056

Conversation

randombit commented May 12, 2024

coveralls commented May 12, 2024 • edited Loading

reneme May 24, 2024 • edited Loading

Choose a reason for hiding this comment

reneme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reneme left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented May 12, 2024 •

edited

Loading

reneme May 24, 2024 •

edited

Loading