Check current signers still validators #1097

JesseAbram · 2024-10-03T18:35:43Z

Related #1114

…k-if-current-signers-are-still-validators

ameba23

⭐ this is great.

Is it possible to further improve this by having the parties who are leaving the signing set also participate in the reshare, but not receive new shares? So that we don't have to keep t parties the same. I can't remember whether synderion lets you do this.

crates/shared/src/types.rs

ameba23 · 2024-10-18T08:05:18Z

crates/threshold-signature-server/src/validator/api.rs

@@ -169,7 +170,8 @@ pub async fn new_reshare(
    .await?;

    let (new_key_share, aux_info) =
-        execute_reshare(session_id.clone(), channels, signer.signer(), inputs, None).await?;
+        execute_reshare(session_id.clone(), channels, signer.signer(), inputs, &new_holders, None)


Maybe ive misunderstood something, but it looks like new_holders is the same as inputs.new_holders - so why add the extra argument to the execute_reshare function?

explained lower down

ameba23 · 2024-10-18T08:12:13Z

crates/threshold-signature-server/src/validator/api.rs

@@ -273,8 +275,15 @@ pub async fn validate_new_reshare(
        .await?
        .ok_or_else(|| ValidatorErr::ChainFetch("Not Currently in a reshare"))?;

-    if reshare_data.new_signer != chain_data.new_signer
-        || chain_data.block_number != reshare_data.block_number
+    let mut hasher_chain_data = Blake2s256::new();


Do we need to hash this? Can we not compare two Vec<Vec<u8>>s for equality?

yes you are right a holdover of a change I revereted from #1114 but will change back

ameba23 · 2024-10-18T08:20:36Z

crates/threshold-signature-server/src/validator/api.rs

-            .collect()
+    Ok(if !new_signers.is_empty() {
+        let mut filtered_validators_info = vec![];
+        for new_signer in new_signers {


This works fine - but since we are anyway about to convert this to a BTreeSet, we can use .difference()

ameba23 · 2024-10-18T08:37:37Z

pallets/staking/src/lib.rs

+                    remove_index_len = remove_indexs.len();
+                    let remove_indexs_reversed: Vec<_> = remove_indexs.iter().rev().collect();
+                    // TODO: Only remove up to threhsold https://github.com/entropyxyz/entropy-core/issues/1114
+                    let truncated = if remove_index_len > signers_info.threshold as usize {


Maybe im getting confused, but should this not be signers_info.total_signers - signers_info.threshold? As in the number of redundant signers (n - t) rather than t.
So with 2 of 3, we can afford to loose at most 1 signer, not 2.

yes, writing a better test for regression now

ameba23 · 2024-10-18T08:45:20Z

pallets/staking/src/lib.rs

                let mut randomness = Self::get_randomness();
                // grab a current signer to initiate value
-                let mut next_signer_up = &current_signers[0].clone();
+                let mut next_signer_up = &validators[0].clone();


Should this not be a random validator rather than the first in the list?

just a place holder for rust not complaining, gets changed lower down (not initialized compile error)

it looks to me like it only gets changed lower down if validators[0] is currently a member of the signer set

right this should be grabbed from current_signers to initiate it

ameba23 · 2024-10-18T09:03:16Z

crates/protocol/src/execute_protocol.rs

@@ -334,6 +334,7 @@ pub async fn execute_reshare(
    chans: Channels,
    threshold_pair: &sr25519::Pair,
    inputs: KeyResharingInputs<KeyParams, PartyId>,
+    verifiers: &BTreeSet<PartyId>,


im not sure we need this argument as it looks like whenever we call this function verifiers is the same as inputs.new_holders

yes originally I changed it to allow for old holders to be more and not in new holders however ran into the #1114 issue, didn't change it back because this is still more right imo and adds no overhead, and hopefully will be needed when the issue is resolved

JesseAbram · 2024-10-18T16:20:13Z

⭐ this is great.

Is it possible to further improve this by having the parties who are leaving the signing set also participate in the reshare, but not receive new shares? So that we don't have to keep t parties the same. I can't remember whether synderion lets you do this.

not currently opened an issue, a limitation with synedrion see #1114

Co-authored-by: peg <[email protected]>

…k-if-current-signers-are-still-validators

pallets/staking/src/benchmarking.rs

HCastano · 2024-10-21T16:15:34Z

pallets/staking/src/benchmarking.rs

@@ -427,13 +429,22 @@ benchmarks! {
  new_session {
    let c in 1 .. MAX_SIGNERS as u32 - 1;
    let l in 0 .. MAX_SIGNERS as u32;
+    let v in 0 .. MAX_SIGNERS as u32;


Shouldn't this value be higher than MAX_SIGNERS?

HCastano · 2024-10-21T16:43:44Z

pallets/staking/src/lib.rs

+            if current_signers_length >= signers_info.total_signers as usize {
+                let mut remove_indexs = vec![];
+                for (i, current_signer) in current_signers.clone().into_iter().enumerate() {
+                    if !validators.contains(&current_signer) {
+                        remove_indexs.push(i);
+                    }
+                }
+                if remove_indexs.is_empty() {
+                    current_signers.remove(0);
+                } else {
+                    remove_index_len = remove_indexs.len();
+                    let remove_indexs_reversed: Vec<_> = remove_indexs.iter().rev().collect();
+                    let truncated = remove_indexs_reversed
+                        [..(signers_info.total_signers as usize - signers_info.threshold as usize)]
+                        .to_vec();
+                    for remove_index in truncated {
+                        current_signers.remove(*remove_index);
+                    }
+                }


Trying to follow the transformations here is pretty confusing. Maybe you could make use of Vec's .retain and .truncate methods here to clean stuff up instead of things like .rev and slices.

commented it up more, for what it is worth the truncated should eventually be removed

pallets/staking/src/lib.rs

pallets/staking/src/tests.rs

crates/threshold-signature-server/src/validator/tests.rs

HCastano · 2024-10-21T16:50:37Z

crates/threshold-signature-server/src/validator/tests.rs

-        OcwMessageReshare { new_signer: new_signer.0.to_vec(), block_number };
+    let onchain_reshare_request = OcwMessageReshare {
+        new_signers: vec![new_signer.0.to_vec()],
+        block_number: block_number - 1,


Why change this instead of keeping the +1 in run_to_block?

because this needs to match the onchain info or it will not pass the validate check

Co-authored-by: Hernando Castano <[email protected]>

dvdplm · 2024-10-23T16:37:45Z

crates/shared/src/types.rs

-    // Stash address of new signer
-    pub new_signer: Vec<u8>,
+    // Stash addresses of new signers
+    pub new_signers: Vec<Vec<u8>>,


A few lines above, in ValidatorInfo, Vecs are provided with the full path, codec::alloc::vec::Vec<u8>. Should we be consistent? The short form here seems better.

dvdplm · 2024-10-23T16:43:32Z

crates/threshold-signature-server/src/validator/api.rs

+    // old holders -> next_signers - new_signers (will be at least t)
+    let old_holders =
+        &prune_old_holders(&api, &rpc, data.new_signers, validators_info.clone()).await?;


I can't find where the (will be at least t) invariant is checked; doesn't seem to happen in prune_old_holders (also, but unrelated to this PR, that method seems to be cloning wildly but not sure that's needed).

ya this is a little hard to follow flow this comes from the chain itself here https://github.com/entropyxyz/entropy-core/pull/1097/files#diff-9be73ee9078236e99d5b726b1a2d9acd38446cedbe770d2f23d39ef51b1763ceR720

crates/threshold-signature-server/src/validator/api.rs

dvdplm · 2024-10-23T16:48:26Z

crates/threshold-signature-server/tests/register_sign_reshare_sign.rs

-        OcwMessageReshare { new_signer: new_signer.0.to_vec(), block_number };
+    let onchain_reshare_request = OcwMessageReshare {
+        new_signers: reshare_data.new_signers.into_iter().map(|s| s.to_vec()).collect(),
+        block_number: block_number - 1,


Does this blow up on block 0? Maybe checked_sub?

the block number is hard coded one line above to a number way over 0

Co-authored-by: David <[email protected]>

JesseAbram added 17 commits October 3, 2024 14:17

Check current signers still validators

454c9ad

update

940b650

tests

a27a9ec

end of day push

0fea1bf

working reshare

7f2346d

clean

a54dd56

remove mutable

ae41b50

test

ee8c1fa

update

2d91051

threhsold limit

f5daa0f

fix test

ed32eba

remove old signer

4482fd6

clean

b1d93ac

Merge branch 'master' of github.com:entropyxyz/entropy-core into chec…

84cf720

…k-if-current-signers-are-still-validators

metadata

97f54a9

fmt

0e8c7b4

test

4ec4f36

JesseAbram mentioned this pull request Oct 16, 2024

Signers Resharing out when not in new validator set (greater than T) #1114

Open

JesseAbram added 3 commits October 16, 2024 18:21

update benchmarks

3a58960

working benchmarks

1864b4d

fix benches

4f7167b

JesseAbram marked this pull request as ready for review October 17, 2024 19:59

JesseAbram requested review from ameba23 and HCastano October 17, 2024 19:59

ameba23 approved these changes Oct 18, 2024

View reviewed changes

JesseAbram and others added 4 commits October 18, 2024 09:22

Update crates/shared/src/types.rs

3328151

Co-authored-by: peg <[email protected]>

correct truncation

118fd90

remove hash comparison

516fa7b

Merge branch 'master' of github.com:entropyxyz/entropy-core into chec…

3d9bd90

…k-if-current-signers-are-still-validators

HCastano reviewed Oct 21, 2024

View reviewed changes

JesseAbram and others added 8 commits October 22, 2024 08:13

Apply suggestions from code review

4154e88

Co-authored-by: Hernando Castano <[email protected]>

fix

75e73f5

fix

3880ff1

fix benchmark

afe674e

fix benchmark

1154f1e

fmt

72c89e5

fix

b5d2ebd

comment

7312376

JesseAbram requested a review from HCastano October 23, 2024 15:24

dvdplm reviewed Oct 23, 2024

View reviewed changes

JesseAbram and others added 3 commits October 23, 2024 10:01

Update crates/threshold-signature-server/src/validator/api.rs

537f1b4

Co-authored-by: David <[email protected]>

fix type

f44a7e3

Merge branch 'master'

0c1dc4b

JesseAbram merged commit 9eab023 into master Oct 24, 2024
8 checks passed

JesseAbram deleted the check-if-current-signers-are-still-validators branch October 24, 2024 18:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check current signers still validators #1097

Check current signers still validators #1097

JesseAbram commented Oct 3, 2024 •

edited

Loading

ameba23 left a comment

ameba23 Oct 18, 2024

JesseAbram Oct 18, 2024

ameba23 Oct 18, 2024

JesseAbram Oct 18, 2024

ameba23 Oct 18, 2024

ameba23 Oct 18, 2024

JesseAbram Oct 18, 2024

ameba23 Oct 18, 2024

JesseAbram Oct 18, 2024

ameba23 Oct 19, 2024

JesseAbram Oct 22, 2024

ameba23 Oct 18, 2024 •

edited

Loading

JesseAbram Oct 18, 2024

JesseAbram commented Oct 18, 2024

HCastano Oct 21, 2024

HCastano Oct 21, 2024

JesseAbram Oct 23, 2024

HCastano Oct 21, 2024

JesseAbram Oct 22, 2024

dvdplm Oct 23, 2024

dvdplm Oct 23, 2024

JesseAbram Oct 23, 2024

dvdplm Oct 23, 2024

JesseAbram Oct 23, 2024

Check current signers still validators #1097

Check current signers still validators #1097

Conversation

JesseAbram commented Oct 3, 2024 • edited Loading

ameba23 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ameba23 Oct 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesseAbram commented Oct 18, 2024

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

JesseAbram commented Oct 3, 2024 •

edited

Loading

ameba23 Oct 18, 2024 •

edited

Loading