Test reshare now takes 2 Ts with signer and 3 Ns #989

JesseAbram · 2024-08-07T20:40:17Z

Add test for new signer without key added to network (currently they all have keys

ameba23

The code changes look great, but i don't understand whats going on with the chainspec changes for tests.

It looks like the idea is to be able to test a reshare where one leaves and one joins but with only three TS servers running - and we do this by mocking that an extra one - dave? - was in the initial jumpstart dkg, but is now leaving the signing committee and alice is joining.

But we don't actually run a dkg involving dave? You're gonna have to explain it to me.

ameba23 · 2024-08-08T06:25:54Z

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

    let party_ids: BTreeSet<PartyId> =
        validators_info.iter().cloned().map(|x| PartyId::new(x.tss_account)).collect();
+    // let mut new_signer_address = data.new_signer;


rm commented code

ameba23 · 2024-08-08T06:30:41Z

node/cli/src/chain_spec/mod.rs

@@ -78,12 +78,12 @@ pub mod tss_account_id {
    /// Not sure what mnemonic is used to derive the following `AccountId`.


Why has this changed? Probably we want to update this comment - not sure if the mnemonic below is correct

these were created with subkey so although they used that mnemonic they are not the right keys, they need to be created with the hpke flow

ameba23 · 2024-08-08T06:30:53Z

node/cli/src/chain_spec/mod.rs

@@ -78,12 +78,12 @@ pub mod tss_account_id {
    /// Not sure what mnemonic is used to derive the following `AccountId`.
    /// Mnemonic: "beef dutch panic monkey black glad audit twice humor gossip wealth drive"
    pub static ref DAVE: sp_runtime::AccountId32  =
-            super::hex!["12bf1c8e365c20cc2af606e3814b98b192857d85d182dac5e33fb90d0380ca75"].into();
+            super::hex!["0a9054ef6b6b8ad0dd2c89895b2515583f2fbf1edced68e7328ae456d86b9402"].into();

    /// The `DEFAULT_CHARLIE_MNEMONIC` is used to derive the following `AccountId`.


Likewise here

ameba23 · 2024-08-08T06:39:23Z

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

    let party_ids: BTreeSet<PartyId> =
        validators_info.iter().cloned().map(|x| PartyId::new(x.tss_account)).collect();
+    // let mut new_signer_address = data.new_signer;
+    let pruned_old_holders =
+        prune_old_holders(&api, &rpc, data.new_signer, validators_info.clone()).await?;


Can we not also get the old holders with the staking extension signers query? Although this is fine as it is.

yes and this goes with your below comment......actually gonna write it there

ameba23 · 2024-08-08T06:55:05Z

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

    let old_holders: BTreeSet<PartyId> =
-        old_holders_info.iter().cloned().map(|x| PartyId::new(x.tss_account)).collect();
+        pruned_old_holders.into_iter().map(|x| PartyId::new(x.tss_account)).collect();


I might have got this wrong, but I think that old_holders also needs to exclude the one old holder who is not present in this session (the one who is leaving the signing committee). That is, old holders means 'participants of this session who have an existing keyshare', not 'everyone who has an existing keyshare'.

Since the reshare protocol runs successfully then probably i've got this wrong, but can't be sure until we have a test which demonstrates signing a message after a reshare, which needs the signing flow to get done first. So i would leave this as it is and once we have that test we'll know.

ya, this is why I use next signer and I prune the new addition. It saves an extra rpc call, pretty much you can do this in two ways,

get signers and next signers and prune out the one no longer in it

use next_signers and prune out the new addition

next_signer has one lest rpc call since we have it already for other reasons

if old_holders can old eveyone and take signers without the prune, then maybe worthwhile to swap but I doubt it

ah ok right i get it

ameba23 · 2024-08-08T07:09:41Z

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

@@ -163,6 +166,21 @@ async fn test_reshare_validation_fail_not_in_reshare() {
    clean_tests();
 }

+#[tokio::test]
+#[serial]
+async fn test_empty_next_signer() {


Good to test this - but this should never happen right? It would mean the propagation pallet had initiated a reshare with the new signer being set to an empty vector.

Introduced in the last PR and was fine until I had to convert next signer to an address, converting to the [u8: 32] would fail if it was empty so wanted to add the test but pretty much this can happen when we reshare down if we reduce the signer count, it would just prune one and send an empty vec as the next signer

ameba23 · 2024-08-08T07:24:42Z

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

@@ -173,7 +191,7 @@ async fn setup_for_reshare(
    let jump_start_request = entropy::tx().registry().jump_start_network();
    let _result = submit_transaction(api, rpc, &signer, &jump_start_request, None).await.unwrap();

-    let validators_names = vec![ValidatorName::Alice, ValidatorName::Bob, ValidatorName::Charlie];
+    let validators_names = vec![ValidatorName::Bob, ValidatorName::Charlie, ValidatorName::Dave];


Im trying to figure out whats going on here.

This means bob, charlie and dave become the current signer set because they are the ones who submitted jump start confirm transactions.

exactly then, alice is in the next_signers and Dave is not so we have current signers as [bob, charlie, dave], next signers as [bob, charlie, alice]

JesseAbram · 2024-08-08T14:53:45Z

The code changes look great, but i don't understand whats going on with the chainspec changes for tests.

It looks like the idea is to be able to test a reshare where one leaves and one joins but with only three TS servers running - and we do this by mocking that an extra one - dave? - was in the initial jumpstart dkg, but is now leaving the signing committee and alice is joining.

But we don't actually run a dkg involving dave? You're gonna have to explain it to me.

you pretty much have this exactly right, the jumpstart is mocked......think about it like this, the jumpstart sets the initial signers and is required to be in the completed state to do a reshare, the jumpstart however does not need to be run in the testing state as we just place a testing key in the kvdb directly so that part is mocked out, but I do need to move the chianstate into jumpstart done so that is why I mock out finishing the jumpstart there. So by doing this the chainstate and tss server is now in the right state as there is a parent key (we placed in manually in test spin up) the chain now knows about this and is in the jumpstate::Done position and also has the next signers ready to go

HCastano · 2024-08-08T15:08:12Z

node/cli/src/chain_spec/mod.rs

    /// Mnemonic: "beef dutch panic monkey black glad audit twice humor gossip wealth drive"
    pub static ref DAVE: sp_runtime::AccountId32  =
-            super::hex!["12bf1c8e365c20cc2af606e3814b98b192857d85d182dac5e33fb90d0380ca75"].into();
+            super::hex!["0a9054ef6b6b8ad0dd2c89895b2515583f2fbf1edced68e7328ae456d86b9402"].into();


@JesseAbram do you mind pulling these changes out into their own PR? There's a bit more backstory here with the PR I shared with you a while back

JesseAbram added 4 commits August 7, 2024 16:39

working

d135a62

clean tests

3522f67

add test

25ad441

clean

ee1536f

JesseAbram marked this pull request as ready for review August 8, 2024 00:47

JesseAbram requested review from ameba23 and HCastano August 8, 2024 00:47

ameba23 approved these changes Aug 8, 2024

View reviewed changes

clean

d768eec

HCastano reviewed Aug 8, 2024

View reviewed changes

JesseAbram merged commit 9dac61d into master Aug 8, 2024
14 checks passed

JesseAbram deleted the test-reshare-with-T-of-2 branch August 8, 2024 17:03

github-actions bot locked and limited conversation to collaborators Aug 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test reshare now takes 2 Ts with signer and 3 Ns #989

Test reshare now takes 2 Ts with signer and 3 Ns #989

JesseAbram commented Aug 7, 2024 •

edited

Loading

ameba23 left a comment

ameba23 Aug 8, 2024

ameba23 Aug 8, 2024

JesseAbram Aug 8, 2024

ameba23 Aug 8, 2024

ameba23 Aug 8, 2024

JesseAbram Aug 8, 2024

ameba23 Aug 8, 2024

JesseAbram Aug 8, 2024

ameba23 Aug 8, 2024

ameba23 Aug 8, 2024

JesseAbram Aug 8, 2024

ameba23 Aug 8, 2024

JesseAbram Aug 8, 2024

JesseAbram commented Aug 8, 2024

HCastano Aug 8, 2024

		@@ -78,12 +78,12 @@ pub mod tss_account_id {
		/// Not sure what mnemonic is used to derive the following `AccountId`.

Test reshare now takes 2 Ts with signer and 3 Ns #989

Test reshare now takes 2 Ts with signer and 3 Ns #989

Conversation

JesseAbram commented Aug 7, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JesseAbram commented Aug 8, 2024

Choose a reason for hiding this comment

JesseAbram commented Aug 7, 2024 •

edited

Loading