Use traits implemented for all Uint/Int types

[fjarri]

Closes #793

• Split and Concat reworked to be implementable for all Uints generically.
• SplitMixed and ConcatMixed removed (Split and Concat implement their functionality).
• RemMixed removed - Rem already allows custom rhs sizes. Moreover, the name RemMixed/rem_mixed() was misleading because it actually used variable time division inside.
• Encoding implemented for all Uints. Encoding::Repr for Uint is now an EncodedUint struct instead of an array.
• Macros deriving Encoded, SplitMixed, ConcatMixed, and RemMixed are removed.
• Relaxed the bound on the error of Encoding::Repr as TryFrom<&[u8]> from a concrete type to a core::error::Error trait.

Notes:

• The new split/concat errors for incorrect sizes, while being compile time, may not be entirely informative. First, I can't format the error string in the panic!() (without another dependency), second, the compiler often does not say which monomorphization caused the error.
• Uint::to_le/be_bytes() are quite similar to Uint::write_le/be_bytes(), the former being const fn, the latter being faster (since they can operate with Word-sized chunks instead of single bytes).
• the extra_sizes feature may be removed since now it only defines Uint aliases and doesn't impl any traits.
• We use serdect for constant-time serialization, so we are concerned about serializing secrets... But Encoding methods litter the stack with the serialized data (not introduced in this PR, it was the same before). Is that a concern as well?

[codecov]

Codecov Report

❌ Patch coverage is 90.67358% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 79.86%. Comparing base (9cbea3a) to head (c2c0117).
⚠️ Report is 1 commits behind head on master.

Files with missing lines	Patch %	Lines
src/uint/encoding.rs	90.41%	7 Missing ⚠️
src/uint/split.rs	78.26%	5 Missing ⚠️
src/uint/concat.rs	86.95%	3 Missing ⚠️
src/uint/mul.rs	77.77%	2 Missing ⚠️
src/traits.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1008      +/-   ##
==========================================
- Coverage   79.87%   79.86%   -0.02%     
==========================================
  Files         163      162       -1     
  Lines       17593    17575      -18     
==========================================
- Hits        14053    14036      -17     
+ Misses       3540     3539       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tarcieri]

Definitely liking this overall direction

[fjarri]

@tarcieri I would like to implement Serialize/Deserialize for all Uints as well, there are two options:

• use bytemuck, which requires implementing some unsafe marker traits, which is currently prohibited by an attribute in the top module;
• add functions to serdect that work with byte iterators instead of &[u8] (so that I can iterate over an integer's bytes without actually using a temporary buffer)

What do you think is a better way?

Actually, the second option may not be possible with serde API... need to check that

Edit: yes, I don't think it will be possible. serde needs some kind of a serialize_bytes_iter() method, and given the glacial pace with which serde moves, and the maintainer's distaste for any changes whatsoever, it'll takes months at best to get that in.

[tarcieri]

which is currently prohibited by an attribute in the top module

It's not prohibited (it's deny instead of forbid) and we do have unsafe a few places, however I would be a bit more concerned about adding an additional hard dependency, even if it is a widely used one. We can consider it, though.

the second option may not be possible with serde API... need to check that

It's possible with serde but not possible with the way serdect works, AFAIK

[fjarri]

Sorry, edited your comment by mistake at first...

It's possible with serde but not possible with the way serdect works, AFAIK

How is it possible? serialize_seq/serialize_tuple will lead to inefficient representations.

[tarcieri]

@fjarri yes, that's what I meant, it's possible with serde, but uses those representations that aren't necessarily constant-time on formats like MsgPack, which is why serdect migrated away from them

[tarcieri]

Another option would be to make the serde feature dependent on hybrid-array and use typenum to compute the size of the output array, which would require making the ArrayEncoding impl valid for all sizes too (or at least, all sizes that can be represented by hybrid-array)

There are a couple ways to do the linkage between const generics and typenum. One is typenum::U. The other is hybrid_array::ArrayN.

[fjarri]

Implemented with bytemuck.

[tarcieri]

Okay, that's fine for now. If you're done can you mark the PR ready for review?

[fjarri]

Ok, but let me check the thing with Encoding::Repr as TryFrom<&[u8]> bound before merging.

[tarcieri]

@fjarri yeah, I remember you expressing wanting to have the concrete error type for the TryFrom bound: #261

[fjarri]

I replaced the strict type with a bound on core::error::Error, I think this will be enough.

[tarcieri]

@fjarri it seems like you're using bytemuck to do effectively two things:

1. call the equivalent of e.g. u64::to_be / u64::from_be (and so on for u32 and _le)
2. perform a pointer cast

I guess I'm fine with that to get the feature implemented, but it seems like it should be fairly straightforward to remove and replace with something that calls the equivalent core functionality to do the same.

[fjarri]

Perhaps, but that would require an unsafe block. Should I do that?

[tarcieri]

Yes, that's fine, there are other unsafe pointer casts already, e.g. Uint::as_(mut_)words

[tarcieri]

One nit but this seems like a big improvement

[fjarri]

Checking out the changes required in crypto-primes, and it seems like this PR makes writing certain generic functions impossible. Namely, we need to extend a given generic Uint by 128 bits, perform some operations, and then drop the high 128 bits. Formerly this could be done, provided the Uint passed by the user had the required ConcatMixed defined.

One can argue that it just makes the algorithm deficiency immediately obvious to the library writer instead of the end user realizing their chosen Uint size doesn't impl the required trait. But still I wonder if this PR will break some downstream code, with the required changes going beyond some simple fixes.

(In my case, something like Uint::div_wide() that takes lo and hi parts of the dividend would help)

Edit: also, as I noted in the description, debugging the compile time failures is kind of hard since the compiler doesn't provide a "backtrace". Although it is probably much simpler in the new code where you can tell which change led to the error.

[tarcieri]

@fjarri oof, I did ask about crypto-primes. So you have a workaround, or need to revert?

[fjarri]

I'll think about it, but even if I do find a solution, other people might struggle with something else. The question is, is that because this PR makes such generic code "fail early", so to speak, or because the PR is fundamentally flawed. I'm leaning towards reverting and giving it more consideration.

Honestly, these lacunas in the compiler functionality are the most frustrating things about Rust.

[Fethbita]

Checking out the changes required in crypto-primes, and it seems like this PR makes writing certain generic functions impossible. Namely, we need to extend a given generic Uint by 128 bits, perform some operations, and then drop the high 128 bits. Formerly this could be done, provided the Uint passed by the user had the required ConcatMixed defined.

I tried to find where this usage is in crypto-primes but couldn't. Could you point out where it is used?

I would also like to use the same operation on my code (generic U4096 -> U4224 -> operations) but could not figure out how to achieve it.

[fjarri]

@Fethbita https://github.com/entropyxyz/crypto-primes/blob/master/src/hazmat/primecount.rs#L76

Actually, now that I look at it it just doubles the width of integers, which I guess is easier from the typing point of view, but it presents the same problems with this PR.

[tarcieri]

@fjarri so what's the verdict? should we revert, or have you found a workaround?

can you maybe put together a playground with a minimal example of the problem and I can ask around for some solutions?

[Fethbita]

Actually, now that I look at it it just doubles the width of integers, which I guess is easier from the typing point of view, but it presents the same problems with this PR.

I saw that as well. What do you mean by but it presents the same problems with this PR? Currently without this PR, it is possible to double the integer size with the same method used just like the link you shared, but this is also possible after the PR, isn't it? I don't think increasing the width of an integer by 128 was possible before, and it is also not possible after (and have it connected with the traits). Am I wrong?

[fjarri]

@Fethbita , before the PR you could use the ConcatMixed trait. The problem is, it was defined for a small number of uints (up to 1024 I think), because of quadratic expansion of generated code.

but this is also possible after the PR, isn't it?

Well, you'd have to have the second generic parameter (RHS_LIMBS), but since now it's not determined by a trait bound, the user would have to specify it explicitly, which is quite annoying.

so what's the verdict? should we revert, or have you found a workaround?

I would vote for revert. I can work around this stuff, but I expect a lot of frustration from other downstream users. The thing is, the approach in this PR works when you have concrete types for inputs and outputs of concat/split; but if you want to temporarily extend and then shrink back an integer (like in the primecount link above), this does not work - the compiler cannot derive the extended type.

Having said that, I will think about fixing the primecount function so that it doesn't rely on Concat because it just means that its potential users might suddenly hit a problem when applying it to some Uint without an alias.

[tarcieri]

Opened #1015 to revert

[andrewwhitehead]

Can the encoding changes be separated out from the rest? They don't seem to be dependent.

[fjarri]

Good idea, see #1016

[fjarri]

Btw, the removal of RemMixed may deserve its own PR as well.

[tarcieri]

I was also wondering if ConcatenatingMul could replace ConcatMixed independently of everything else

[andrewwhitehead]

For adding a fixed number of limbs, it could be more useful to expose the ExtendedUint used in bingcd and add functionality there as needed. Uint could have extended_mul and extended_square methods returning this type.