[main] Crypto extension wrappers and data types for C++ #52

linh2931 · 2022-10-12T14:21:23Z

Change Description

Resolve #45.

Add types to abstract G1 point, G2 Point, and big integer
Add wrappers using those new types
Add checks on result buffer sizes
Add unit and integration tests

API Changes

API Changes

Documentation Additions

Documentation Additions

smlu · 2022-10-13T18:28:24Z

libraries/eosiolib/core/eosio/crypto_ext.hpp

+       * @param x_ - The x coordinate, a vector of chars
+       * @param y_ - The y coordinate, a vector of chars
+       */
+      g1_view(std::vector<char>& x_, std::vector<char>& y_)


I would suggest using std::span<char> instead of std::vector<char> so other containers can be used (e.g. compile-time defined std::array<char>) or offsets into binary data.

Thanks for your suggestion here and in the associated issue. std::span is in C++20 while we still need to support C++17 at the moment.

Oh sorry I didn't realize support for C++17 is still required.

Ok I guess this would be proper for separate issue/PR, but how about creating a new span like type for C++17 (internal namespace) and exposing only 'bytes view' like span alias to cdt? When support for C++17 is dropped in the future only this internal span implementation has to be removed but the alias would stay intact without breaking api.

Just for reference, I made one simple type imitating span for such cases: https://github.com/ZeroPass/antelope.ck/blob/1bf203db28d053e9d5b171fb92f27c44b94acdc4/include/eosiock/span.hpp

smlu · 2022-10-13T18:45:04Z

libraries/eosiolib/core/eosio/crypto_ext.hpp

+   struct bigint {
+      /**
+       * Construct a bigint given a vector of chars (bytes)
+       *
+       * @param s - The source bytes
+       */
+      bigint(std::vector<char>& s)
+      :data(s.data()), size(s.size())
+      {
+      };
+
+      char* data;
+      uint32_t size;
+   };


Instead of introducing new type I suggest making an alias for std::span<char> aka bytes_view.

smlu · 2022-10-13T18:49:58Z

libraries/eosiolib/core/eosio/crypto_ext.hpp

+    *  @param result - the result of the compression
+    *  @return -1 if there is an error otherwise 0
+    */
+   int32_t blake2_f( uint32_t rounds, const std::vector<char>& state, const std::vector<char>& msg, const std::vector<char>& t0_offset, const std::vector<char>& t1_offset, bool final, std::vector<char>& result) {


I suggest using std::span based 'bytes view' type here as well for the inputs instead of const std::vector<char>.

smlu · 2022-10-13T19:04:49Z

tests/integration/crypto_tests.cpp

+
+   produce_blocks();
+


I suggest to also add additional test with smaller exponent and larger base number, same size as modulus (to emulate RSA crypto).

Thanks. Will add more tests for this.

dimas1185 · 2022-10-21T18:07:38Z

libraries/eosiolib/core/eosio/crypto_ext.hpp

+      /**
+       *  Returns packed x and y
+       */
+      std::vector<char> packed() const {


Unless there are some recent changes that I'm not aware of, we are using non-class member functions like:
eosio::pack and eosio::unpack all over the code. So I suggest to adhere to those standards.

The name here is misleading. The intention was to provide a way to serialize x and y only, and populate a G1 or G2 point from a serialized form. Will change the names.

ok, got you. Anyway it looks like some non-standard approach for serialization. There is CDT_REFLECT macro in the eosiolib, have you considered it? it looks like a good approach for your purpose. it could be a bit tricky to use in your case but that way we can stick to standard approach. please let me know what do you think.

tests/integration/crypto_tests.cpp

larryk85 · 2022-10-26T12:39:59Z

libraries/eosiolib/core/eosio/crypto_ext.hpp

+      /**
+       * Pointer to the x coordinate
+       */
+      char* x;


Since this a view, we should make these const char*

Because you need to be able to return a point_view in some of the functions, I suggest that we have point_view and point. Where point holds std::vectors for x and y so we don't need to worry about memory issues.

Thanks for your suggestions! I was struggling on how to mutate point_view to return a result.

larryk85 · 2022-10-26T19:49:30Z

libraries/eosiolib/core/eosio/crypto_ext.hpp

+    *
+    *  @ingroup crypto
+    */
+   struct g1_view : public point_view {


I would just make these templated and add a typedefs.

So, something like

template <std::size_t Size = 32> struct point_view { point_view(const std::vector<char>& x, const std::vector<char>& y) { >> The other constructor code here from above << eosio::check ( size == Size, "point size must match"); } . . . using g1_view = point_view<g1_coordinate_size>; using g2_view = point_view<g2_coordinate_size>;

Will change. Thanks.

larryk85 · 2022-10-26T19:57:49Z

libraries/eosiolib/core/eosio/crypto_ext.hpp

+       * @param x_ - The x coordinate, a vector of chars
+       * @param y_ - The y coordinate, a vector of chars
+       */
+      g1_view(std::vector<char>& x_, std::vector<char>& y_)


Instead of the 3 separate types these can be collapsed in to point_view and have point_view become templates.

template <std::size_t Size = 32> struct point_view

Then,

using g1_view = point_view<g1_coordinate_size>; using g2_view = point_view<g2_coordinate_size>;

larryk85 · 2022-10-26T20:00:08Z

libraries/eosiolib/core/eosio/crypto_ext.hpp

+   struct bigint {
+      /**
+       * Construct a bigint given a vector of chars (bytes)
+       *


Given our uses of bigint at this point, I would suggest we just go with an std::vector or a typedef.

So,

using bigint = std::vector<char>;

I think std::vector<char> would be better aliased as bytes since it could be used in other non-crypto related cases. Also, since ABI already defines bytes type for std::vector<char> I believe such alias would also improve code readability.

Did you mean

using bytes = std::vector<char>; using bigint = bytes;

?

I guess you could abstract it that way. Tho in this case, the bigint alias just brings another abstraction layer with no additional functionality and I don't know if such abstraction is ok in terms of code readability and usage (i.e. 2 layers bigint -> bytes to know it's an alias for std::vector<char>). If it were up to me I'd avoid bigint alias.

Agreed. Two layers of abstraction is not good. Keep bigint as is for future extensions.

larryk85 · 2022-10-26T20:07:09Z

libraries/eosiolib/core/eosio/crypto_ext.hpp

+      * @param source - The source to be copied from
+      */
+      void copy_from(const std::vector<char>& source) {
+         eosio::check( source.size() == 2 * size, "size of souce buffer must be size of x + y" );


Typo 'souce'

Oh my bad. Thanks for spotting this.

larryk85 · 2022-10-26T20:08:09Z

libraries/eosiolib/core/eosio/crypto_ext.hpp

+      *
+      * @param source - The source to be copied from
+      */
+      void copy_from(const std::vector<char>& source) {


This is a view, I don't think it should have a function to mutate.

larryk85 · 2022-10-26T20:09:42Z

libraries/eosiolib/core/eosio/crypto_ext.hpp

+    *  @param result - result of the addition operation
+    *  @return -1 if there is an error otherwise 0
+    */
+   inline int32_t alt_bn128_add( const g1_view& op1, const g1_view& op2, g1_view& result) {


For these we can check the internal return value and assert. And return a "result" from this instead of making it an in/out param.

Will change.

larryk85 · 2022-10-26T20:12:06Z

libraries/eosiolib/core/eosio/crypto_ext.hpp

+      /**
+       * Pointer to the x coordinate
+       */
+      char* x;


Because you need to be able to return a point_view in some of the functions, I suggest that we have point_view and point. Where point holds std::vectors for x and y so we don't need to worry about memory issues.

…porate other review comments

larryk85 · 2022-11-01T13:59:14Z

libraries/eosiolib/core/eosio/crypto_ext.hpp

+       * @param p - The serialized point
+       */
+      ec_point(std::vector<char>& p)
+      :x(p.data(), p.data() + p.size()/2), y(p.data() + p.size()/2, p.data() + p.size())


p.size()/2 can just be Size as we check that p.size() == Size*2 later.

larryk85 · 2022-11-01T14:01:41Z

libraries/eosiolib/core/eosio/crypto_ext.hpp

+       * @param x_ - The x coordinate, a vector of chars
+       * @param y_ - The y coordinate, a vector of chars
+       */
+      ec_point_view(std::vector<char>& x_, std::vector<char>& y_)


We should have a constructor that accepts a ec_point with ec_point_view::Size == ec_point::Size` via static assertion.

larryk85 · 2022-11-01T14:03:11Z

libraries/eosiolib/core/eosio/crypto_ext.hpp

+       * @param x_ - The x coordinate, a vector of chars
+       * @param y_ - The y coordinate, a vector of chars
+       */
+      ec_point_view(std::vector<char>& x_, std::vector<char>& y_)


This shouldn't take an std::vector<char>& but a const char*.

larryk85 · 2022-11-01T14:03:41Z

libraries/eosiolib/core/eosio/crypto_ext.hpp

+       *
+       * @param p - The serialized point
+       */
+      ec_point_view(std::vector<char>& p)


Same as above constructor.

…an existing ec_point_view constructor to take char* as an input

Crypto extension wrappers for C++

de1746c

linh2931 requested a review from larryk85 October 12, 2022 14:21

smlu reviewed Oct 13, 2022

View reviewed changes

add additional tests for modexp to emulate RSA usage

6c9f4d9

dimas1185 reviewed Oct 21, 2022

View reviewed changes

tests/integration/crypto_tests.cpp Outdated Show resolved Hide resolved

linh2931 added 2 commits October 22, 2022 18:56

use common tester class for crypto primitives tests

5d571bf

rename pack and unpack to avoid confusions

3ca87ee

larryk85 suggested changes Oct 26, 2022

View reviewed changes

add mutable and readonly point and point view to simplify APIs, incor…

fdb19bd

…porate other review comments

larryk85 suggested changes Nov 1, 2022

View reviewed changes

add an ec_point_view constructor taking ec_point as an input; change …

d3d9716

…an existing ec_point_view constructor to take char* as an input

larryk85 approved these changes Nov 7, 2022

View reviewed changes

Merge branch 'main' into crypto_ext_c_plus_plus

a56e9b8

linh2931 merged commit 29396b9 into main Nov 7, 2022

linh2931 deleted the crypto_ext_c_plus_plus branch November 7, 2022 16:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[main] Crypto extension wrappers and data types for C++ #52

[main] Crypto extension wrappers and data types for C++ #52

linh2931 commented Oct 12, 2022

smlu Oct 13, 2022

linh2931 Oct 13, 2022

smlu Oct 14, 2022

smlu Oct 13, 2022

smlu Oct 13, 2022

smlu Oct 13, 2022

linh2931 Oct 13, 2022

dimas1185 Oct 21, 2022

linh2931 Oct 22, 2022 •

edited

Loading

dimas1185 Oct 24, 2022

larryk85 Oct 26, 2022

larryk85 Oct 26, 2022

linh2931 Oct 26, 2022

larryk85 Oct 26, 2022

linh2931 Oct 26, 2022

larryk85 Oct 26, 2022

larryk85 Oct 26, 2022

smlu Oct 27, 2022

linh2931 Oct 28, 2022

smlu Nov 2, 2022

linh2931 Nov 2, 2022

larryk85 Oct 26, 2022

linh2931 Oct 26, 2022

larryk85 Oct 26, 2022

larryk85 Oct 26, 2022

linh2931 Oct 26, 2022

larryk85 Oct 26, 2022

larryk85 Nov 1, 2022

larryk85 Nov 1, 2022

larryk85 Nov 1, 2022

larryk85 Nov 1, 2022

[main] Crypto extension wrappers and data types for C++ #52

[main] Crypto extension wrappers and data types for C++ #52

Conversation

linh2931 commented Oct 12, 2022

Change Description

API Changes

Documentation Additions

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linh2931 Oct 22, 2022 • edited Loading

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

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

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

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

linh2931 Oct 22, 2022 •

edited

Loading