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

Rotate parent network key takes 2 steps #1018

Merged
merged 13 commits into from
Aug 28, 2024
Binary file modified crates/client/entropy_metadata.scale
Binary file not shown.
3 changes: 3 additions & 0 deletions crates/shared/src/constants.rs
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ pub const DEVICE_KEY_PROXY: &[u8] = include_bytes!("../device_key_proxy.wasm");
/// Network parent key specific size to fit into [u8; 32] to save extra code
pub const NETWORK_PARENT_KEY: &str = "NETWORK_PARENT_KEY_FOR_ENTROPY_";

// Lookup key for the next network parent keyshare ready to be swapped in to new one
pub const NEXT_NETWORK_PARENT_KEY: &str = "NEXT_NETWORK_PARENT_KEY";

/// Total signers on the network with the parent key
pub const TOTAL_SIGNERS: u8 = 3;

Expand Down
3 changes: 2 additions & 1 deletion crates/threshold-signature-server/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -149,7 +149,7 @@ use crate::{
r#unsafe::api::{delete, put, remove_keys, unsafe_get},
signing_client::{api::*, ListenerState},
user::api::*,
validator::api::new_reshare,
validator::api::{new_reshare, rotate_network_key},
};

#[derive(Clone)]
Expand All @@ -171,6 +171,7 @@ pub fn app(app_state: AppState) -> Router {
.route("/user/sign_tx", post(sign_tx))
.route("/signer/proactive_refresh", post(proactive_refresh))
.route("/validator/reshare", post(new_reshare))
.route("/validator/rotate_network_key", post(rotate_network_key))
.route("/attest", post(attest))
.route("/healthz", get(healthz))
.route("/version", get(get_version))
Expand Down
97 changes: 85 additions & 12 deletions crates/threshold-signature-server/src/validator/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ pub use entropy_protocol::{
execute_protocol::{execute_protocol_generic, execute_reshare, Channels, PairWrapper},
KeyParams, KeyShareWithAuxInfo, Listener, PartyId, SessionId, ValidatorInfo,
};
use entropy_shared::{OcwMessageReshare, NETWORK_PARENT_KEY};
use entropy_shared::{OcwMessageReshare, NETWORK_PARENT_KEY, NEXT_NETWORK_PARENT_KEY};
use parity_scale_codec::{Decode, Encode};
use sp_core::Pair;
use std::{collections::BTreeSet, str::FromStr};
Expand Down Expand Up @@ -93,12 +93,9 @@ pub async fn new_reshare(
)
.map_err(|e| ValidatorErr::VerifyingKeyError(e.to_string()))?;

let is_proper_signer = is_signer_or_delete_parent_key(
signer.account_id(),
validators_info.clone(),
&app_state.kv_store,
)
.await?;
let is_proper_signer = validators_info
.iter()
.any(|validator_info| validator_info.tss_account == *signer.account_id());

if !is_proper_signer {
return Ok(StatusCode::MISDIRECTED_REQUEST);
Expand Down Expand Up @@ -176,20 +173,72 @@ pub async fn new_reshare(

let serialized_key_share = key_serialize(&(new_key_share, aux_info))
.map_err(|_| ProtocolErr::KvSerialize("Kv Serialize Error".to_string()))?;
let network_parent_key = hex::encode(NETWORK_PARENT_KEY);
// TODO: should this be a two step process? see # https://github.com/entropyxyz/entropy-core/issues/968
if app_state.kv_store.kv().exists(&network_parent_key).await? {
app_state.kv_store.kv().delete(&network_parent_key).await?
let next_network_parent_key = hex::encode(NEXT_NETWORK_PARENT_KEY);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this need to be encoded as hex?

Copy link
Member Author

Choose a reason for hiding this comment

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

the type that the kv takes needs it, but tbh should probably make the const that


if app_state.kv_store.kv().exists(&next_network_parent_key).await? {
app_state.kv_store.kv().delete(&next_network_parent_key).await?
};

let reservation = app_state.kv_store.kv().reserve_key(network_parent_key).await?;
let reservation = app_state.kv_store.kv().reserve_key(next_network_parent_key).await?;
app_state.kv_store.kv().put(reservation, serialized_key_share.clone()).await?;

// TODO: Error handling really complex needs to be thought about.
confirm_key_reshare(&api, &rpc, &signer).await?;
Ok(StatusCode::OK)
}

/// HTTP POST endpoint called by the off-chain worker (propagation pallet) after a network key reshare.
///
/// This rotates network key, deleting the previous network parent key.
#[tracing::instrument(skip_all)]
pub async fn rotate_network_key(
State(app_state): State<AppState>,
) -> Result<StatusCode, ValidatorErr> {
// validate from chain
let api = get_api(&app_state.configuration.endpoint).await?;
let rpc = get_rpc(&app_state.configuration.endpoint).await?;
validate_rotate_network_key(&api, &rpc).await?;

let (signer, _) = get_signer_and_x25519_secret(&app_state.kv_store)
.await
.map_err(|e| ValidatorErr::UserError(e.to_string()))?;

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 validators_info = get_validators_info(&api, &rpc, signers)
.await
.map_err(|e| ValidatorErr::UserError(e.to_string()))?;

let is_proper_signer = is_signer_or_delete_parent_key(
signer.account_id(),
validators_info.clone(),
&app_state.kv_store,
)
.await?;

if !is_proper_signer {
return Ok(StatusCode::MISDIRECTED_REQUEST);
}

let network_parent_key_heading = hex::encode(NETWORK_PARENT_KEY);
let next_network_parent_key_heading = hex::encode(NEXT_NETWORK_PARENT_KEY);

let new_parent_key = app_state.kv_store.kv().get(&next_network_parent_key_heading).await?;

if app_state.kv_store.kv().exists(&network_parent_key_heading).await? {
app_state.kv_store.kv().delete(&network_parent_key_heading).await?;
};

app_state.kv_store.kv().delete(&next_network_parent_key_heading).await?;

let reservation = app_state.kv_store.kv().reserve_key(network_parent_key_heading).await?;
app_state.kv_store.kv().put(reservation, new_parent_key).await?;
Comment on lines +226 to +238
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean we should get rid of the is_signer_or_delete_parent_key step in the other flow? We want to wait for this to be called before deleting the parent key

Copy link
Member Author

Choose a reason for hiding this comment

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

mmm yes that is a good point, that should be moved to the second step

Ok(StatusCode::OK)
}

// Validates new reshare endpoint
/// Checks the chain for validity of data and block number of data matches current block
pub async fn validate_new_reshare(
Expand Down Expand Up @@ -236,6 +285,30 @@ pub async fn validate_new_reshare(
Ok(())
}

/// Checks the chain that the reshare was completed recently.
///
/// We only care that this happens after a reshare so we just check that message isn't stale
pub async fn validate_rotate_network_key(
api: &OnlineClient<EntropyConfig>,
rpc: &LegacyRpcMethods<EntropyConfig>,
) -> Result<(), ValidatorErr> {
let latest_block_number = rpc
.chain_get_header(None)
.await?
.ok_or_else(|| ValidatorErr::OptionUnwrapError("Failed to get block number".to_string()))?
.number;
Comment on lines +295 to +299
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here it makes sense to check using the finalized header

Copy link
Member Author

Choose a reason for hiding this comment

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

no , I think that would fail as this needs to match when the OCW is fired and that isn't on finalized but on block creation


let rotate_keyshares_info_query = entropy::storage().staking_extension().rotate_keyshares();
let rotate_keyshare_block = query_chain(api, rpc, rotate_keyshares_info_query, None)
.await?
.ok_or_else(|| ValidatorErr::ChainFetch("Rotate Keyshare not in progress"))?;

if latest_block_number > rotate_keyshare_block {
return Err(ValidatorErr::StaleData);
}

Ok(())
}
/// Confirms that a validator has succefully reshared.
pub async fn confirm_key_reshare(
api: &OnlineClient<EntropyConfig>,
Expand Down
84 changes: 82 additions & 2 deletions crates/threshold-signature-server/src/validator/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -101,15 +101,95 @@ async fn test_reshare() {
let (key_share_after, aux_info_after): KeyShareWithAuxInfo =
deserialize(&key_share_and_aux_data_after).unwrap();

// Check key share has not yet changed
assert_eq!(serialize(&key_share_before).unwrap(), serialize(&key_share_after).unwrap());
// Check aux info has not yet changed
assert_eq!(serialize(&aux_info_before).unwrap(), serialize(&aux_info_after).unwrap());

let _ = client
.post(format!("http://127.0.0.1:{}/validator/rotate_network_key", validator_ports[i]))
.send()
.await
.unwrap();

let key_share_and_aux_data_after_rotate =
unsafe_get(&client, hex::encode(NETWORK_PARENT_KEY), validator_ports[i]).await;
let (key_share_after_rotate, aux_info_after_rotate): KeyShareWithAuxInfo =
deserialize(&key_share_and_aux_data_after_rotate).unwrap();

// Check key share has changed
assert_ne!(serialize(&key_share_before).unwrap(), serialize(&key_share_after).unwrap());
assert_ne!(
serialize(&key_share_before).unwrap(),
serialize(&key_share_after_rotate).unwrap()
);
// Check aux info has changed
assert_ne!(serialize(&aux_info_before).unwrap(), serialize(&aux_info_after).unwrap());
assert_ne!(
serialize(&aux_info_before).unwrap(),
serialize(&aux_info_after_rotate).unwrap()
);

// calling twice doesn't do anything
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

let response = client
.post(format!("http://127.0.0.1:{}/validator/rotate_network_key", validator_ports[i]))
.send()
.await
.unwrap();

assert_eq!(response.text().await.unwrap(), "Kv error: Recv Error: channel closed");
let key_share_and_aux_data_after_rotate_twice =
unsafe_get(&client, hex::encode(NETWORK_PARENT_KEY), validator_ports[i]).await;
let (key_share_after_rotate_twice, aux_info_after_rotate_twice): KeyShareWithAuxInfo =
deserialize(&key_share_and_aux_data_after_rotate_twice).unwrap();

// Check key share has not changed
assert_eq!(
serialize(&key_share_after_rotate_twice).unwrap(),
serialize(&key_share_after_rotate).unwrap()
);
// Check aux info has not changed
assert_eq!(
serialize(&aux_info_after_rotate_twice).unwrap(),
serialize(&aux_info_after_rotate).unwrap()
);
}

run_to_block(&rpc, block_number + 7).await;

let response_stale =
client.post("http://127.0.0.1:3001/validator/rotate_network_key").send().await.unwrap();

assert_eq!(response_stale.text().await.unwrap(), "Data is stale");

// TODO #981 - test signing a message with the new keyshare set
clean_tests();
}

#[tokio::test]
#[serial]
async fn test_reshare_none_called() {
initialize_test_logger().await;
clean_tests();

let _cxt = test_node_process_testing_state(true).await;

let add_parent_key_to_kvdb = true;
let (_validator_ips, _validator_ids) = spawn_testing_validators(add_parent_key_to_kvdb).await;

let validator_ports = vec![3001, 3002, 3003];

let client = reqwest::Client::new();

for i in 0..validator_ports.len() {
let response = client
.post(format!("http://127.0.0.1:{}/validator/rotate_network_key", validator_ports[i]))
.send()
.await
.unwrap();

assert_eq!(response.text().await.unwrap(), "Chain Fetch: Rotate Keyshare not in progress");
}
}

#[tokio::test]
#[serial]
async fn test_reshare_validation_fail() {
Expand Down
5 changes: 5 additions & 0 deletions node/cli/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -367,6 +367,11 @@ pub fn new_full_base(
b"reshare_validators",
&format!("{}/validator/reshare", endpoint).into_bytes(),
);
offchain_db.local_storage_set(
sp_core::offchain::StorageKind::PERSISTENT,
b"rotate_keyshares",
&format!("{}/validator/rotate_keyshares", endpoint).into_bytes(),
);
offchain_db.local_storage_set(
sp_core::offchain::StorageKind::PERSISTENT,
b"attest",
Expand Down
48 changes: 48 additions & 0 deletions pallets/propagation/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ pub mod pallet {
let _ = Self::post_reshare(block_number);
let _ = Self::post_proactive_refresh(block_number);
let _ = Self::post_attestation_request(block_number);
let _ = Self::post_rotate_keyshare(block_number);
}

fn on_initialize(_block_number: BlockNumberFor<T>) -> Weight {
Expand All @@ -96,6 +97,10 @@ pub mod pallet {

/// Attestations request message passed
AttestationRequestMessagePassed(OcwMessageAttestationRequest),

/// Key Rotate Message passed to validators
/// parameters. [BlockNumberFor<T>]
Comment on lines +101 to +102
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Key Rotate Message passed to validators
/// parameters. [BlockNumberFor<T>]
/// Key Rotate Message passed to validators parameters. [SOMETHING_MORE_DESCRIPTIVE]

KeyRotatesMessagePassed(BlockNumberFor<T>),
}

#[pallet::call]
Expand Down Expand Up @@ -259,6 +264,49 @@ pub mod pallet {
Ok(())
}

/// Submits a request to rotate parent network key the threshold servers.
pub fn post_rotate_keyshare(block_number: BlockNumberFor<T>) -> Result<(), http::Error> {
let rotate_keyshares = pallet_staking_extension::Pallet::<T>::rotate_keyshares();
if rotate_keyshares != block_number {
return Ok(());
}

let deadline = sp_io::offchain::timestamp().add(Duration::from_millis(2_000));
let kind = sp_core::offchain::StorageKind::PERSISTENT;
let from_local = sp_io::offchain::local_storage_get(kind, b"rotate_keyshares")
.unwrap_or_else(|| b"http://localhost:3001/validator/rotate_keyshares".to_vec());
let url = str::from_utf8(&from_local)
.unwrap_or("http://localhost:3001/validator/rotate_keyshares");

log::warn!("propagation::post rotate keyshare");

let converted_block_number: u32 =
BlockNumberFor::<T>::try_into(block_number).unwrap_or_default();

// We construct the request
// important: the header->Content-Type must be added and match that of the receiving
// party!!
let pending = http::Request::post(url, vec![converted_block_number.encode()])
.deadline(deadline)
.send()
.map_err(|_| http::Error::IoError)?;

// We await response, same as in fn get()
let response =
pending.try_wait(deadline).map_err(|_| http::Error::DeadlineReached)??;

// check response code
if response.code != 200 {
log::warn!("Unexpected status code: {}", response.code);
return Err(http::Error::Unknown);
}
let _res_body = response.body().collect::<Vec<u8>>();

Self::deposit_event(Event::KeyRotatesMessagePassed(block_number));

Ok(())
}

/// Submits a request for a TDX attestation.
pub fn post_attestation_request(
block_number: BlockNumberFor<T>,
Expand Down
11 changes: 11 additions & 0 deletions pallets/propagation/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,6 +64,14 @@ fn knows_how_to_mock_several_http_calls() {
body: [32, 1, 0, 0, 0, 0, 0, 0, 0, 6, 0, 0, 0].to_vec(),
..Default::default()
});
state.expect_request(testing::PendingRequest {
method: "POST".into(),
uri: "http://localhost:3001/validator/rotate_keyshares".into(),
sent: true,
response: Some([].to_vec()),
body: [10, 0, 0, 0].to_vec(),
..Default::default()
});
});

t.execute_with(|| {
Expand Down Expand Up @@ -91,6 +99,9 @@ fn knows_how_to_mock_several_http_calls() {
});
// now triggers
Propagation::post_reshare(7).unwrap();

pallet_staking_extension::RotateKeyshares::<Test>::put(10);
Propagation::post_rotate_keyshare(10).unwrap();
})
}

Expand Down
8 changes: 8 additions & 0 deletions pallets/staking/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,11 @@ pub mod pallet {
#[pallet::getter(fn jump_start_progress)]
pub type JumpStartProgress<T: Config> = StorageValue<_, JumpStartDetails<T>, ValueQuery>;

/// Tell Signers to rotate keyshare
#[pallet::storage]
#[pallet::getter(fn rotate_keyshares)]
pub type RotateKeyshares<T: Config> = StorageValue<_, BlockNumberFor<T>, ValueQuery>;

/// A type used to simplify the genesis configuration definition.
pub type ThresholdServersConfig<T> = (
<T as pallet_session::Config>::ValidatorId,
Expand Down Expand Up @@ -505,6 +510,9 @@ pub mod pallet {
let current_signer_length = signers_info.next_signers.len();
if signers_info.confirmations.len() == (current_signer_length - 1) {
Signers::<T>::put(signers_info.next_signers.clone());
RotateKeyshares::<T>::put(
<frame_system::Pallet<T>>::block_number() + sp_runtime::traits::One::one(),
);
Self::deposit_event(Event::SignersRotation(signers_info.next_signers));
Ok(Pays::No.into())
} else {
Expand Down
Loading