-
Notifications
You must be signed in to change notification settings - Fork 1
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
@@ -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; | ||
|
@@ -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 | ||
|
@@ -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?; | ||
|
||
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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might have got this wrong, but I think that 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
@@ -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() | ||
}) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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::{ | ||
|
@@ -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; | ||
|
@@ -163,6 +166,21 @@ async fn test_reshare_validation_fail_not_in_reshare() { | |
clean_tests(); | ||
} | ||
|
||
#[tokio::test] | ||
#[serial] | ||
async fn test_empty_next_signer() { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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>, | ||
|
@@ -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]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe 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) = | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
|
||
} | ||
} | ||
|
@@ -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] = [ | ||
|
There was a problem hiding this comment.
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.There was a problem hiding this comment.
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