Refactor of Refresh & Recovery procedures #158

cygnusv · 2023-08-31T17:23:10Z

Type of PR:

Required reviews:

1
2
3

What this does:
This PR is mainly a refactor of code related to refresh & recovery procedures. The main contributions are:

Relocation of refresh & recovery code from the tpke crate to the ferveo crate
Fixing incorrect refresh & recovery tests (see e.g., 8ef9e7f)
General improvements in tests

Issues fixed/closed:

Closes Devise and test a share refreshing scenario with multiple delears #73

Why it's needed:
This work is the basis to implementing the recovery & refresh protocols (see #163)

Notes for reviewers:
No breaking changes!

Work in this PR has surfaced these issues:

piotr-roslaniec · 2023-08-31T20:46:04Z

@cygnusv There is a build error on CI, unrelated to this error. Please see: #159

Technically, this helper function creates a random polynomial where the `root` parameter is a root of the polynomial, so `make_random_polynomial_with_root` is a better name.

There was no function for the refresh case, and since it's very similar to the recovery case, we define here a common method for both (`prepare_share_updates_with_root`)

The concept of "threshold" doesn't belong to the abstraction level of polynomials, which actually deal with degrees. For the record, it's always the case that `threshold == degree + 1`.

Test using secret reconstruction directly, rather than via ciphertext decryption.

These functions assumed access to an update polynomial, but we're actually using several distributed polynomials (one for each participant) and we only have access to their evaluated points. Note that the commits before did the preparation to remove this now.

Updates for shares were not applied correctly (see diff in L418-423). For the moment, we're recovering the same domain point since it facilitates debugging, but in the next commit I'll restore the test so there's a new validator, with a random domain point.

codecov-commenter · 2023-09-12T11:21:25Z

Codecov Report

Merging #158 (8407f78) into main (0a73aa3) will increase coverage by 0.06%.
The diff coverage is 99.77%.

@@            Coverage Diff             @@
##             main     #158      +/-   ##
==========================================
+ Coverage   77.87%   77.94%   +0.06%     
==========================================
  Files          23       23              
  Lines        5031     5051      +20     
==========================================
+ Hits         3918     3937      +19     
- Misses       1113     1114       +1

Files Changed	Coverage Δ
ferveo/src/lib.rs	`97.57% <99.18%> (+0.12%)`	⬆️
ferveo/src/dkg.rs	`96.11% <100.00%> (ø)`
ferveo/src/pvss.rs	`91.16% <100.00%> (-0.54%)`	⬇️
ferveo/src/refresh.rs	`100.00% <100.00%> (ø)`
tpke/src/lib.rs	`99.78% <100.00%> (-0.08%)`	⬇️

* Removed benchmark for random polynomial with root (not much value as it's essentially benchmarking arkworks) * Commented recovery & refresh benchmarks for when #162 is solved

piotr-roslaniec · 2023-09-14T07:48:41Z

ferveo/src/lib.rs

@@ -419,28 +430,37 @@ mod test_dkg_full {
                    .decryption_key;

                // Creates updated private key shares
+                // TODO: Why not using dkg.aggregate()?


IIRC dkg.aggregate would throw if we didn't feed enough transcripts into it.

dkg has an internal state machine that tracks the progress of the DKG ritual, and we would have to strictly adhere to its protocol in order to successfully aggregate here. Since refreshing/recovery is not implemented in the said state machine, it's easier to skip different DKG states and just call aggregate directly here.

piotr-roslaniec · 2023-09-14T08:06:40Z

ferveo/src/lib.rs

+                let deltas_i = prepare_share_updates_for_refresh::<E>(
+                    &domain_points,
+                    &dkg.pvss_params.h.into_affine(),
+                    dkg.dkg_params.security_threshold as usize,
+                    rng,
+                );


Just a comment on the subject of the future refactor: We can see how this could be rewritten as let deltas_i = dkg.prepare_share_updates_for_refresh(rng);

piotr-roslaniec · 2023-09-14T08:13:51Z

ferveo/src/lib.rs

+                    DecryptionShareSimple::create(
+                        &validator_keypair.decryption_key,
+                        &updated_shares.get(share_index).unwrap(),
+                        &ciphertext.header().unwrap(),
+                        aad,
+                        &dkg.pvss_params.g_inv(),
+                    )
+                    .unwrap()


Not specific to this PR but to the future refactor:
Perhaps we could avoid this section by creating a DecryptionShareSimple in pvss_aggregated.update_private_key_share_for_recovery? I think there is no other reason for moving updated_shares around other than eventually creating a DecryptionShareSimple, so we could collapse this logic into one step. Naming TBD.

Not really, updating private shares and creating decryption shares are absolutely independent. We create decryption shares here since it's an acceptance test.

You're right, this is completely the other way around - The nucypher usage of ferveo API confirms that it happens at two different time steps. Ursulas would never update their shares & create a decryption share at the same time, these are two completely different user flows.

piotr-roslaniec · 2023-09-14T08:17:22Z

ferveo/src/refresh.rs

+use rand_core::RngCore;
+use tpke::{lagrange_basis_at, PrivateKeyShare};
+
+// SHARE UPDATE FUNCTIONS:


We could always put those functions into a module if we think it would help organize them better:

pub mod share_update { pub fn prepare_share_updates_for_recover ... pub fn apply_updates_to_private_share ... } // etc.

But on second thought, these sections will be different by the time we're done with the refactoring.

Anyway, just throwing it out there - It's an option.

oh that's a nice idea, I didn't know that. Let's consider that for the next refactor.

piotr-roslaniec · 2023-09-14T08:22:41Z

ferveo/src/refresh.rs

+    debug_assert!(poly.evaluate(root) == E::ScalarField::zero());
+    debug_assert!(poly.coeffs.len() == degree + 1);


Just a comment on this macro usage:
debug_assert! doesn't show up in a release target, i.e. this assertion would be removed in production. If we want this assertion to actually throw in production, we should use assert! instead. I didn't think it would be necessary, so I opted for the former.

piotr-roslaniec · 2023-09-14T08:28:29Z

ferveo/src/refresh.rs

+        // Finally, let's recreate the shared private key from the refreshed shares
+        let new_shared_private_key = recover_share_from_updated_private_shares(
+            &ScalarField::zero(),
+            &domain_points[..threshold],
+            &refreshed_shares[..threshold],
+        );
+
+        assert_eq!(
+            shared_private_key,
+            new_shared_private_key.private_key_share
+        );


We could test the red path here (not enough shares) similarly as we do in tdec_simple_variant_share_recovery_at_selected_point

piotr-roslaniec · 2023-09-14T08:30:12Z

ferveo/src/refresh.rs

+                &domain_points[..(threshold - 1)],
+                &new_share_fragments[..(threshold - 1)],


We could throw in an assert! here to make sure the domain_points and new_share_fragments have a proper length. It's not strictly necessary, but it relates to some of the original issues we had with those tests.

piotr-roslaniec

LGTM 🎉

Just some comments, but nothing blocking.

derekpierre

🎸 - a bit intricate for me to provide a comprehensive review, but looks good.

cygnusv added 2 commits September 4, 2023 09:02

Remove Pvss type alias

59aacbb

Relocate refresh.rs module from tpke to ferveo crate

372f5d7

cygnusv force-pushed the detection branch from 37bb054 to ea78ae7 Compare September 7, 2023 11:41

cygnusv and others added 9 commits September 11, 2023 17:50

Adapting recovery & refresh tests

5c141e1

Rename random polynomial function for refresh & recovery

b6d1f41

Technically, this helper function creates a random polynomial where the `root` parameter is a root of the polynomial, so `make_random_polynomial_with_root` is a better name.

Helper functions to prepare share updates for both recovery & refresh

39ec865

There was no function for the refresh case, and since it's very similar to the recovery case, we define here a common method for both (`prepare_share_updates_with_root`)

Cleaning up the DKG recovery test, in preparation for bug fixing

2d30abf

Clarify how the degree in make_random_polynomial_with_root is defined

6115976

The concept of "threshold" doesn't belong to the abstraction level of polynomials, which actually deal with degrees. For the record, it's always the case that `threshold == degree + 1`.

Make sure test for recovery at original point is correct

b65798f

Simplify simple recovery test

7c26487

Test using secret reconstruction directly, rather than via ciphertext decryption.

Simplify share refresh test

d732196

cygnusv force-pushed the detection branch from ea78ae7 to 8a42529 Compare September 11, 2023 15:51

cygnusv added 3 commits September 12, 2023 13:01

Definitely fix test_dkg_simple_tdec_share_recovery

ab5e58c

Fix test_dkg_simple_tdec_share_refreshing()

8407f78

cygnusv force-pushed the detection branch from 8a42529 to 8407f78 Compare September 12, 2023 11:04

cygnusv added 2 commits September 13, 2023 12:23

TODOs and comments

3b3ff48

Disable affected benchmarks

29101b1

* Removed benchmark for random polynomial with root (not much value as it's essentially benchmarking arkworks) * Commented recovery & refresh benchmarks for when #162 is solved

cygnusv force-pushed the detection branch from d51b516 to 29101b1 Compare September 13, 2023 10:39

cygnusv changed the title ~~[WIP] Revision of Refresh & Recovery procedures~~ Refactor of Refresh & Recovery procedures Sep 14, 2023

cygnusv requested review from piotr-roslaniec and theref September 14, 2023 06:12

cygnusv marked this pull request as ready for review September 14, 2023 06:12

cygnusv requested review from jMyles, KPrasch, derekpierre and manumonti September 14, 2023 06:13

cygnusv added the enhancement New feature or request label Sep 14, 2023

piotr-roslaniec reviewed Sep 14, 2023

View reviewed changes

piotr-roslaniec approved these changes Sep 14, 2023

View reviewed changes

Linting and stuff

1704f86

derekpierre approved these changes Sep 15, 2023

View reviewed changes

cygnusv merged commit a979fd6 into nucypher:main Sep 15, 2023
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor of Refresh & Recovery procedures #158

Refactor of Refresh & Recovery procedures #158

cygnusv commented Aug 31, 2023 •

edited

Loading

piotr-roslaniec commented Aug 31, 2023

codecov-commenter commented Sep 12, 2023

piotr-roslaniec Sep 14, 2023 •

edited

Loading

piotr-roslaniec Sep 14, 2023

cygnusv Sep 14, 2023

piotr-roslaniec Sep 14, 2023

cygnusv Sep 14, 2023

piotr-roslaniec Sep 14, 2023

piotr-roslaniec Sep 14, 2023 •

edited

Loading

cygnusv Sep 14, 2023

piotr-roslaniec Sep 14, 2023

piotr-roslaniec Sep 14, 2023

piotr-roslaniec Sep 14, 2023

piotr-roslaniec left a comment

derekpierre left a comment

		debug_assert!(poly.evaluate(root) == E::ScalarField::zero());
		debug_assert!(poly.coeffs.len() == degree + 1);

		&domain_points[..(threshold - 1)],
		&new_share_fragments[..(threshold - 1)],

Refactor of Refresh & Recovery procedures #158

Refactor of Refresh & Recovery procedures #158

Conversation

cygnusv commented Aug 31, 2023 • edited Loading

piotr-roslaniec commented Aug 31, 2023

codecov-commenter commented Sep 12, 2023

Codecov Report

piotr-roslaniec Sep 14, 2023 • 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

piotr-roslaniec Sep 14, 2023 • 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

piotr-roslaniec left a comment

Choose a reason for hiding this comment

derekpierre left a comment

Choose a reason for hiding this comment

cygnusv commented Aug 31, 2023 •

edited

Loading

piotr-roslaniec Sep 14, 2023 •

edited

Loading

piotr-roslaniec Sep 14, 2023 •

edited

Loading