refactor: tbs #4009

joschisan · 2024-01-04T07:34:10Z

We remove generics to make the code more explicit and adhere to a consistent functional style.

codecov · 2024-01-04T08:54:08Z

Codecov Report

Attention: 20 lines in your changes are missing coverage. Please review.

Comparison is base (320dbc7) 58.35% compared to head (2155907) 58.82%.

Files	Patch %	Lines
crypto/tbs/src/lib.rs	86.08%	16 Missing ⚠️
modules/fedimint-mint-server/src/lib.rs	92.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4009      +/-   ##
==========================================
+ Coverage   58.35%   58.82%   +0.47%     
==========================================
  Files         194      192       -2     
  Lines       42581    42117     -464     
==========================================
- Hits        24847    24777      -70     
+ Misses      17734    17340     -394

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

elsirion

I think removing some of the type-level complexity that I introduced in an overzealous attempt at avoiding code duplication is good, but we should not expose functions that are easy to mis-use. The tbs crate should be kinda idiot-proof. We might not get around exposing some of the lower level stuff for the DKG, which we might not be able to move into the tbs crate (but would be worth a try), but the mint module should not need to know that it cannot accumulate more than a given number of notes.

crypto/tbs/src/lib.rs

elsirion · 2024-01-04T15:07:53Z

crypto/tbs/src/lib.rs

+            .map(|(lm, y)| lm * y.0)
+            .reduce(|a, b| a + b)


Please use more descriptive names, this ain't a math paper 😜, e.g. lagrange_m instead of lm (whatever m stands for xD)

IMO if the scope of a symbol is tiny enough, it's kind of OK to go supa-short-names, as it helps readability.

Also reduce(Add::add) works and avoids naming anything.

And isn't the whole thing just a .sum()?

Agree for add (why doesn't .sum work btw?), but I had to stop and think what lm meant.

sum does not work because in case of an empty iterator it returns the zero type but we want to panic

elsirion · 2024-01-04T15:08:27Z

crypto/tbs/src/lib.rs

+            .zip(shares.values())
+            .map(|(lm, y)| lm * y.0)
+            .reduce(|a, b| a + b)
+            .expect("We have at least one share")


How do we handle the single-sig case? Probably elsewhere?

elsirion · 2024-01-04T15:09:30Z

crypto/tbs/src/lib.rs

+        lagrange_multiplier(shares.keys().cloned().map(Scalar::from).collect())
+            .into_iter()
+            .zip(shares.values())
+            .map(|(lm, y)| lm * y.0)
+            .reduce(|a, b| a + b)
+            .expect("We have at least one share")
+            .to_affine(),


Feels like parts of this could be refactored out and de-duplicated xD but I guess your goal was to simplify and accept redundancy.

crypto/tbs/src/lib.rs

elsirion · 2024-01-04T15:12:46Z

crypto/tbs/src/poly.rs

-    pub fn coefficients(&self) -> Iter<G> {
-        self.coefficients.iter()
-    }
+pub fn random_scalar_coefficients(degree: usize, rng: &mut impl RngCore) -> Vec<Scalar> {


Technically we probably also want to require CryptoRng to be implemented by the rng

elsirion · 2024-01-04T15:15:09Z

crypto/tbs/src/poly.rs

-            _pd: PhantomData,
-        }
-    }
+pub fn evaluate_polynomial_scalar(coefficients: &[Scalar], x: &Scalar) -> Scalar {


Documentation needed: what are assumptions about inputs (at least one coeff, …)

joschisan · 2024-01-05T08:07:22Z

I think there a two further conceptual issues with the tbs crate that relate to some of the comments here:

The aggregation of public key shares: after the dkg every peer obtains the same polynomial that it uses to calculate the public key shares for every peer. Then the shares are stored in the MintConsensusConfig and are later aggregated (in two different places even) into the public keys with the same Lagrange interpolation as the blind signatures which causes the code duplication @elsirion commented on. However the public key is just the polynomial obtained from the dkg evaluated at zero so the information is trivially available when creating the MintConsensusConfig. Therefore the aggregated public keys should really be stored in the MintConsensusConfig. Aggregating the pk shares is just weird. The main question here, it seems, is how to do it not if?

The amalgamation of key generation with tbs. Both tbs and the threshold point encryption for the ng ln module are quite simple, the design space is small. However, both of them require the same dkg while the dkg is comparatively complicated and has a larger design space as well. That's why I think it makes sense to split the dkg from tbs and tpe - then we would also not have to expose low level functions like evaluate_polynomial in the tbs crate. The dealer_keygen in tbs would be private for testing reasons only. Though this would lead to some code duplication of the helper functions, like dealer_keygen and evaluate_polynomial_scalar it would allow for clean conceptual separation between the crates. This could be done with this pr and the question is if we want to do it at all?

dpc · 2024-01-05T17:36:41Z

crypto/tbs/benches/tbs.rs


-        bencher.iter(move || combine_valid_shares(shares.clone(), 4));
+        bencher.iter(move || aggregate_signature_shares(&shares.clone()));


Both & and .clone() looks unnecessary, but I am probably missing something.

elsirion · 2024-01-07T18:11:28Z

The aggregation of public key shares

Maybe we want to save the polynomial in the cfg instead of the key sahres then and calculate both aggregated pub key and key shares on startup? That should be much more straight forward (and is also more in line with the design of the old threshold crypto crate).

As long as we don't have redundancy in the config I'm ok with it.

elsirion · 2024-01-07T18:16:17Z

That's why I think it makes sense to split the dkg from tbs and tpe - then we would also not have to expose low level functions like evaluate_polynomial in the tbs crate.

I'm a bit confused: if the tbs+tpe crates don't expose lower-level functionality, how do you imple the DKG on top? Or would there be a common poly crate that is consumed by dkg, tbs and tpe and allows dkg to be used for both tbs and tpe as kind of a blackbox?

joschisan · 2024-01-08T08:20:12Z

The aggregation of public key shares

Maybe we want to save the polynomial in the cfg instead of the key sahres then and calculate both aggregated pub key and key shares on startup? That should be much more straight forward (and is also more in line with the design of the old threshold crypto crate).

As long as we don't have redundancy in the config I'm ok with it.

Yes that would be a clean way to do it. However we would have to write a db migration that recovers the original polynomial coefficients from the evaluations. Though this is possible we would have to implement new crypto code to do it. Therefore I would suggest to just add the aggregated pk as we already have the code for the db migration this would require. Though this technically adds redundancy to the config please keep in mind that the config already has redundancy by saving the public key shares for every peer instead of just the polynomial.

joschisan · 2024-01-09T08:19:56Z

That's why I think it makes sense to split the dkg from tbs and tpe - then we would also not have to expose low level functions like evaluate_polynomial in the tbs crate.

I'm a bit confused: if the tbs+tpe crates don't expose lower-level functionality, how do you imple the DKG on top? Or would there be a common poly crate that is consumed by dkg, tbs and tpe and allows dkg to be used for both tbs and tpe as kind of a blackbox?

I think a common poly crate is overkill as the shared code from polynomial evaluation are just a few lines. Right now I opted for duplication of the poly eval code and also duplicated the dealer_keygen function such that tbs actually only exposes threshold blind signing as the name implies. In the future we may create a keygen crypto crate for dealer_keygen and all cryptographic primitives for dkg such as Feldman or Pedersen commitments, polynomial evaluation. Then we could remove the duplicated code again. However as I do not consider the keygen code stable as tbs I would hold of on this.

crypto/tbs/src/lib.rs

elsirion · 2024-01-18T10:16:02Z

modules/fedimint-mint-server/src/lib.rs

+            let keys = (1_u64..)
+                .zip(keys.into_iter().cloned())
+                .take(cfg.consensus.peer_tbs_pks.threshold())
+                .collect();
+
+            (amt, aggregate_public_key_shares(&keys))


More a critique of my existing code: the key index being implicit seems like a bad idea.

modules/fedimint-mint-server/src/lib.rs

fedimint-server/src/config/distributedgen.rs

crypto/tbs/src/lib.rs

elsirion · 2024-01-22T10:45:29Z

crypto/tbs/src/lib.rs

-    /// **IMPORTANT**: `from_bytes` includes a tag in the hash, this doesn't
-    pub fn from_hash(hash: impl Digest<OutputSize = U32>) -> Message {
-        Message(hash_to_curve::<G1Projective, _>(hash).to_affine())
-    }


I guess we never used this?

crypto/tbs/src/lib.rs

elsirion · 2024-01-22T10:51:13Z

modules/fedimint-mint-server/src/lib.rs

-            .enumerate()
-            .collect::<Vec<_>>();
+            }))
+            .take(server_cfgs.len() - ((server_cfgs.len() - 1) / 3))


Otoh, it might be good to independently verify that our thresholds are correct, otoh we already have a function to calculate the threshold.

modules/fedimint-mint-server/src/lib.rs

elsirion · 2024-01-22T17:13:49Z

Dev call: Maybe add a reference to "D. L. Vo, F. Zhang, and K. Kim, “A new threshold blind signature scheme from pairings,” 2003."

dpc

LGTM, but my expertise here is shallow.

joschisan requested review from a team as code owners January 4, 2024 07:34

joschisan force-pushed the tbs_refactor branch 2 times, most recently from a30c8d6 to 0955a87 Compare January 4, 2024 08:46

joschisan force-pushed the tbs_refactor branch from 0955a87 to ac33b5c Compare January 4, 2024 09:25

elsirion reviewed Jan 4, 2024

View reviewed changes

joschisan force-pushed the tbs_refactor branch 2 times, most recently from b8a1fc6 to 38b8464 Compare January 5, 2024 07:36

joschisan force-pushed the tbs_refactor branch 2 times, most recently from 797a224 to 76889e1 Compare January 5, 2024 08:13

dpc reviewed Jan 5, 2024

View reviewed changes

joschisan force-pushed the tbs_refactor branch 5 times, most recently from 0e86892 to 36fa9b6 Compare January 9, 2024 07:27

joschisan force-pushed the tbs_refactor branch from bac8105 to e247f03 Compare January 9, 2024 08:53

elsirion reviewed Jan 18, 2024

View reviewed changes

joschisan force-pushed the tbs_refactor branch 2 times, most recently from 20449d0 to f6b8767 Compare January 20, 2024 08:21

elsirion previously approved these changes Jan 22, 2024

View reviewed changes

joschisan dismissed elsirion’s stale review via 2eeb143 January 22, 2024 11:59

joschisan force-pushed the tbs_refactor branch from f6b8767 to 2eeb143 Compare January 22, 2024 11:59

elsirion approved these changes Jan 22, 2024

View reviewed changes

modules/fedimint-mint-server/src/lib.rs Show resolved Hide resolved

refactor: remove generics, use consistent functional style

2155907

joschisan force-pushed the tbs_refactor branch from 2eeb143 to 2155907 Compare January 23, 2024 07:44

dpc approved these changes Jan 23, 2024

View reviewed changes

dpc added this pull request to the merge queue Jan 23, 2024

Merged via the queue into fedimint:master with commit e17b6ee Jan 23, 2024
20 checks passed

joschisan deleted the tbs_refactor branch April 20, 2024 07:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: tbs #4009

refactor: tbs #4009

joschisan commented Jan 4, 2024 •

edited

codecov bot commented Jan 4, 2024 •

edited

elsirion left a comment

elsirion Jan 4, 2024

dpc Jan 5, 2024

elsirion Jan 7, 2024

joschisan Jan 9, 2024

elsirion Jan 4, 2024

elsirion Jan 4, 2024

elsirion Jan 4, 2024

elsirion Jan 4, 2024

joschisan commented Jan 5, 2024

dpc Jan 5, 2024

elsirion commented Jan 7, 2024

elsirion commented Jan 7, 2024

joschisan commented Jan 8, 2024

joschisan commented Jan 9, 2024

elsirion Jan 18, 2024

elsirion Jan 22, 2024

elsirion Jan 22, 2024

elsirion commented Jan 22, 2024

dpc left a comment


		bencher.iter(move \|\| combine_valid_shares(shares.clone(), 4));
		bencher.iter(move \|\| aggregate_signature_shares(&shares.clone()));

refactor: tbs #4009

refactor: tbs #4009

Conversation

joschisan commented Jan 4, 2024 • edited

codecov bot commented Jan 4, 2024 • edited

Codecov Report

elsirion 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joschisan commented Jan 5, 2024

Choose a reason for hiding this comment

elsirion commented Jan 7, 2024

elsirion commented Jan 7, 2024

joschisan commented Jan 8, 2024

joschisan commented Jan 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elsirion commented Jan 22, 2024

dpc left a comment

Choose a reason for hiding this comment

joschisan commented Jan 4, 2024 •

edited

codecov bot commented Jan 4, 2024 •

edited