Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Aug 8, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Binary file modified crates/client/entropy_metadata.scale
Binary file not shown.
53 changes: 38 additions & 15 deletions crates/threshold-signature-server/src/validator/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -65,15 +65,12 @@ pub async fn new_reshare(
let api = get_api(&app_state.configuration.endpoint).await?;
let rpc = get_rpc(&app_state.configuration.endpoint).await?;
validate_new_reshare(&api, &rpc, &data, &app_state.kv_store).await?;
let signers_query = entropy::storage().staking_extension().signers();
let signers = query_chain(&api, &rpc, signers_query, None)
.await?
.ok_or_else(|| ValidatorErr::ChainFetch("Error getting signers"))?;

let next_signers_query = entropy::storage().staking_extension().signers();
let next_signers_query = entropy::storage().staking_extension().next_signers();
let next_signers = query_chain(&api, &rpc, next_signers_query, None)
.await?
.ok_or_else(|| ValidatorErr::ChainFetch("Error getting next signers"))?;
.ok_or_else(|| ValidatorErr::ChainFetch("Error getting next signers"))?
.next_signers;

let validators_info = get_validators_info(&api, &rpc, next_signers)
.await
Expand All @@ -84,9 +81,11 @@ pub async fn new_reshare(
.map_err(|e| ValidatorErr::UserError(e.to_string()))?;

let verifying_key_query = entropy::storage().staking_extension().jump_start_progress();
let verifying_key = query_chain(&api, &rpc, verifying_key_query, None)
let parent_key_details = query_chain(&api, &rpc, verifying_key_query, None)
.await?
.ok_or_else(|| ValidatorErr::ChainFetch("Parent verifying key error"))?
.ok_or_else(|| ValidatorErr::ChainFetch("Parent verifying key error"))?;

let verifying_key = parent_key_details
.verifying_key
.ok_or_else(|| ValidatorErr::OptionUnwrapError("Failed to get verifying key".to_string()))?
.0;
Expand All @@ -106,10 +105,11 @@ pub async fn new_reshare(
if !is_proper_signer {
return Ok(StatusCode::MISDIRECTED_REQUEST);
}
// get old key if have it

let my_stash_address = get_stash_address(&api, &rpc, signer.account_id())
.await
.map_err(|e| ValidatorErr::UserError(e.to_string()))?;

let old_holder: Option<OldHolder<KeyParams, PartyId>> =
if data.new_signer == my_stash_address.encode() {
None
Expand All @@ -120,19 +120,19 @@ pub async fn new_reshare(
.ok_or_else(|| ValidatorErr::KvDeserialize("Failed to load KeyShare".into()))?;
Some(OldHolder { key_share: key_share.0 })
};

let party_ids: BTreeSet<PartyId> =
validators_info.iter().cloned().map(|x| PartyId::new(x.tss_account)).collect();

let old_holders_info = get_validators_info(&api, &rpc, signers)
.await
.map_err(|e| ValidatorErr::UserError(e.to_string()))?;
let pruned_old_holders =
prune_old_holders(&api, &rpc, data.new_signer, validators_info.clone()).await?;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah ok right i get it


let new_holder = NewHolder {
verifying_key: decoded_verifying_key,
// TODO: get from chain see #941
old_threshold: party_ids.len(),
old_threshold: parent_key_details.parent_key_threshold as usize,
old_holders,
};
let key_info_query = entropy::storage().parameters().signers_info();
Expand Down Expand Up @@ -316,3 +316,26 @@ pub fn check_forbidden_key(key: &str) -> Result<(), ValidatorErr> {
}
Ok(())
}

/// Filters out new signer from next signers to get old holders
pub async fn prune_old_holders(
api: &OnlineClient<EntropyConfig>,
rpc: &LegacyRpcMethods<EntropyConfig>,
new_signer: Vec<u8>,
validators_info: Vec<ValidatorInfo>,
) -> Result<Vec<ValidatorInfo>, ValidatorErr> {
Ok(if !new_signer.is_empty() {
let address_slice: &[u8; 32] = &new_signer.clone().try_into().unwrap();
let new_signer_address = AccountId32(*address_slice);
let new_signer_info = &get_validators_info(api, rpc, vec![new_signer_address])
.await
.map_err(|e| ValidatorErr::UserError(e.to_string()))?[0];
validators_info
.iter()
.filter(|x| x.tss_account != new_signer_info.tss_account)
.cloned()
.collect()
} else {
validators_info.clone()
})
}
24 changes: 21 additions & 3 deletions crates/threshold-signature-server/src/validator/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ use crate::{
},
validator::get_signer_and_x25519_secret_from_mnemonic,
},
validator::{api::validate_new_reshare, errors::ValidatorErr},
validator::{
api::{prune_old_holders, validate_new_reshare},
errors::ValidatorErr,
},
};
use entropy_kvdb::clean_tests;
use entropy_shared::{
Expand All @@ -55,7 +58,7 @@ async fn test_reshare() {
initialize_test_logger().await;
clean_tests();

let alice = AccountKeyring::Alice;
let alice = AccountKeyring::AliceStash;

let cxt = test_node_process_testing_state(true).await;
let (_validator_ips, _validator_ids) = spawn_testing_validators(true).await;
Expand Down Expand Up @@ -163,6 +166,21 @@ async fn test_reshare_validation_fail_not_in_reshare() {
clean_tests();
}

#[tokio::test]
#[serial]
async fn test_empty_next_signer() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

initialize_test_logger().await;
clean_tests();

let cxt = test_context_stationary().await;
let api = get_api(&cxt.node_proc.ws_url).await.unwrap();
let rpc = get_rpc(&cxt.node_proc.ws_url).await.unwrap();

assert!(prune_old_holders(&api, &rpc, vec![], vec![]).await.is_ok());

clean_tests();
}

async fn setup_for_reshare(
api: &OnlineClient<EntropyConfig>,
rpc: &LegacyRpcMethods<EntropyConfig>,
Expand All @@ -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];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]

for validator_name in validators_names {
let mnemonic = development_mnemonic(&Some(validator_name));
let (tss_signer, _static_secret) =
Expand Down
11 changes: 8 additions & 3 deletions node/cli/src/chain_spec/integration_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,9 +53,15 @@ pub fn integration_tests_config() -> ChainSpec {
crate::chain_spec::authority_keys_from_seed("Alice"),
crate::chain_spec::authority_keys_from_seed("Bob"),
crate::chain_spec::authority_keys_from_seed("Charlie"),
crate::chain_spec::authority_keys_from_seed("Dave"),
],
vec![],
get_account_id_from_seed::<sr25519::Public>("Alice"),
vec![
get_account_id_from_seed::<sr25519::Public>("Alice//stash"),
get_account_id_from_seed::<sr25519::Public>("Bob//stash"),
get_account_id_from_seed::<sr25519::Public>("Charlie//stash"),
],
))
.build()
}
Expand All @@ -72,6 +78,7 @@ pub fn integration_tests_genesis_config(
)>,
initial_nominators: Vec<AccountId>,
root_key: AccountId,
mock_signer_rotate_data: Vec<AccountId>,
) -> serde_json::Value {
// Note that any endowed_accounts added here will be included in the `elections` and
// `technical_committee` genesis configs. If you don't want that, don't push those accounts to
Expand Down Expand Up @@ -208,9 +215,7 @@ pub fn integration_tests_genesis_config(
],
vec![EVE_VERIFYING_KEY.to_vec(), DAVE_VERIFYING_KEY.to_vec()],
),
mock_signer_rotate: (true, initial_authorities.iter().map(|auth| {
auth.0.clone()
}).collect::<Vec<_>>(), vec![get_account_id_from_seed::<sr25519::Public>("Alice")],),
mock_signer_rotate: (true, mock_signer_rotate_data, vec![get_account_id_from_seed::<sr25519::Public>("Alice//stash")],),
},
"elections": ElectionsConfig {
members: endowed_accounts
Expand Down
9 changes: 4 additions & 5 deletions node/cli/src/chain_spec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,15 +75,15 @@ pub mod tss_account_id {
pub static ref CHARLIE: sp_runtime::AccountId32 =
super::hex!["946140d3d5ddb980c74ffa1bb64353b5523d2d77cdf3dc617fd63de9d3b66338"].into();

/// Not sure what mnemonic is used to derive the following `AccountId`.
/// The `DEFAULT_DAVE_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();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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


/// The `DEFAULT_CHARLIE_MNEMONIC` is used to derive the following `AccountId`.
/// The `DEFAULT_EVE_MNEMONIC` is used to derive the following `AccountId`.
/// Mnemonic: "impact federal dish number fun crisp various wedding radio immense whisper glue"
pub static ref EVE: sp_runtime::AccountId32 =
super::hex!["f2b4113735e988f662fe45e97b39770e804ebcd893ad0ab7cd8b7c5b5dcfff22"].into();
super::hex!["ac0d9030598f1722ff7c6a2a3043fa65903448dcc7a23011ec06c1c31cdad120"].into();

}
}
Expand All @@ -97,7 +97,6 @@ pub mod tss_x25519_public_key {
8, 22, 19, 230, 107, 217, 249, 190, 14, 142, 155, 252, 156, 229, 120, 11, 180, 35, 83, 245,
222, 11, 153, 201, 162, 29, 153, 13, 123, 126, 128, 32,
];

/// The `DEFAULT_BOB_MNEMONIC` is used to derive the public key.
/// Mnemonic: "where sight patient orphan general short empower hope party hurt month voice"
pub const BOB: [u8; 32] = [
Expand Down
1 change: 1 addition & 0 deletions node/cli/src/endowed_accounts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,5 +75,6 @@ pub fn endowed_accounts_dev() -> Vec<AccountId> {
crate::chain_spec::tss_account_id::ALICE.clone(),
crate::chain_spec::tss_account_id::BOB.clone(),
crate::chain_spec::tss_account_id::CHARLIE.clone(),
crate::chain_spec::tss_account_id::DAVE.clone(),
]
}
Loading