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

Signing flow with derived accounts #990

Merged
merged 19 commits into from
Aug 14, 2024
Merged
Show file tree
Hide file tree
Changes from 15 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
3 changes: 3 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Binary file modified crates/client/entropy_metadata.scale
Binary file not shown.
6 changes: 3 additions & 3 deletions crates/client/src/client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -131,7 +131,7 @@ pub async fn sign(
auxilary_data: Option<Vec<u8>>,
) -> Result<RecoverableSignature, ClientError> {
let message_hash = Hasher::keccak(&message);
let validators_info = get_signers_from_chain(api, rpc).await?;
let validators_info = get_signers_from_chain(api, rpc, false).await?;
tracing::debug!("Validators info {:?}", validators_info);
let block_number = rpc.chain_get_header(None).await?.ok_or(ClientError::BlockNumber)?.number;
let signature_request = UserSignatureRequest {
Expand Down Expand Up @@ -295,9 +295,9 @@ pub async fn put_register_request_on_chain(
rpc: &LegacyRpcMethods<EntropyConfig>,
signature_request_keypair: sr25519::Pair,
deployer: SubxtAccountId32,
program_instance: BoundedVec<ProgramInstance>,
program_instances: BoundedVec<ProgramInstance>,
HCastano marked this conversation as resolved.
Show resolved Hide resolved
) -> Result<(), ClientError> {
let registering_tx = entropy::tx().registry().register(deployer, program_instance);
let registering_tx = entropy::tx().registry().register(deployer, program_instances);

submit_transaction_with_pair(api, rpc, &signature_request_keypair, &registering_tx, None)
.await?;
Expand Down
30 changes: 22 additions & 8 deletions crates/client/src/user.rs
Original file line number Diff line number Diff line change
Expand Up @@ -36,27 +36,39 @@ pub struct UserSignatureRequest {
pub block_number: BlockNumber,
/// Hashing algorithm to be used for signing
pub hash: HashingAlgorithm,
/// The veryfying key for the signature requested
/// The verifying key for the signature requested
pub signature_verifying_key: Vec<u8>,
}

pub async fn get_signers_from_chain(
api: &OnlineClient<EntropyConfig>,
rpc: &LegacyRpcMethods<EntropyConfig>,
with_parent_key: bool,
Copy link
Member

Choose a reason for hiding this comment

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

ehhh maybe a get signers from chain and a get validators from chain function makes more sense

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't disagree, but for the case of this function it is always getting the signers. The difference between both arms here is that the signers are either: queried directly or grabbed from the validator set.

) -> Result<Vec<ValidatorInfo>, SubgroupGetError> {
let all_validators_query = entropy::storage().session().validators();
let mut validators = query_chain(api, rpc, all_validators_query, None)
.await?
.ok_or_else(|| SubgroupGetError::ChainFetch("Get all validators error"))?;
let block_hash = rpc.chain_get_block_hash(None).await?;
let mut handles = Vec::new();
let mut validators = if with_parent_key {
let signer_query = entropy::storage().staking_extension().signers();
query_chain(api, rpc, signer_query, None)
.await?
.ok_or_else(|| SubgroupGetError::ChainFetch("Get all validators error"))?
} else {
let all_validators_query = entropy::storage().session().validators();
let mut validators = query_chain(api, rpc, all_validators_query, None)
.await?
.ok_or_else(|| SubgroupGetError::ChainFetch("Get all validators error"))?;

validators.sort();
validators
};

// TODO #898 For now we use a fix proportion of the number of validators as the threshold
let threshold = (validators.len() as f32 * 0.75) as usize;

// TODO #899 For now we just take the first t validators as the ones to perform signing
validators.sort();
Copy link
Contributor

Choose a reason for hiding this comment

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

Fine for now to not sort - but the reason we have this was because of query results coming back in a different order with polkadot js vs subxt. So the idea was to sort before truncating.

If the plan is that this fn should no longer be called by both the client and the TS server then we need to remove it from entropy-client.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it will still be called by both. Probably safer to keep the sorting

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ameba23 when I sorted the signers coming from the on-chain flow the signing test started failing. Not entirely sure why 🤷

validators.truncate(threshold);

let block_hash = rpc.chain_get_block_hash(None).await?;
let mut handles = Vec::new();

for validator in validators {
let handle: tokio::task::JoinHandle<Result<ValidatorInfo, SubgroupGetError>> =
tokio::task::spawn({
Expand All @@ -77,8 +89,10 @@ pub async fn get_signers_from_chain(
})
}
});

handles.push(handle);
}

let mut all_signers: Vec<ValidatorInfo> = vec![];
for handle in handles {
all_signers.push(handle.await??);
Expand Down
3 changes: 2 additions & 1 deletion crates/threshold-signature-server/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,9 @@ uuid ={ version="1.10.0", features=["v4"] }

# Misc
tokio-tungstenite="0.23.1"
bip39 ={ version="2.0.0", features=["zeroize"] }
bincode ="1.3.3"
bip32 ={ version="0.5.2" }
bip39 ={ version="2.0.0", features=["zeroize"] }
HCastano marked this conversation as resolved.
Show resolved Hide resolved
bytes ={ version="1.7", default-features=false, features=["serde"] }
base64 ="0.22.1"
clap ={ version="4.5.15", features=["derive"] }
Expand Down
6 changes: 4 additions & 2 deletions crates/threshold-signature-server/src/helpers/signing.rs
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ pub async fn do_signing(
app_state: &AppState,
signing_session_info: SigningSessionInfo,
request_limit: u32,
derivation_path: Option<bip32::DerivationPath>,
) -> Result<RecoverableSignature, ProtocolErr> {
tracing::debug!("Preparing to perform signing");

Expand All @@ -60,8 +61,8 @@ pub async fn do_signing(

let account_id = AccountId32(signer.public().0);

// set up context for signing protocol execution
let sign_context = signing_service.get_sign_context(info.clone()).await?;
// Set up context for signing protocol execution
let sign_context = signing_service.get_sign_context(info.clone(), derivation_path).await?;

let tss_accounts: Vec<AccountId32> = user_signature_request
.validators_info
Expand Down Expand Up @@ -89,6 +90,7 @@ pub async fn do_signing(
&x25519_secret_key,
)
.await?;

let channels = {
let ready = timeout(Duration::from_secs(SETUP_TIMEOUT_SECONDS), rx_ready).await?;
let broadcast_out = ready??;
Expand Down
4 changes: 3 additions & 1 deletion crates/threshold-signature-server/src/helpers/substrate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,10 +75,12 @@ pub async fn get_registered_details(
let registered_result = query_chain(api, rpc, registered_info_query, None).await?;

let registration_info = if let Some(old_registration_info) = registered_result {
tracing::debug!("Found user in old `Registered` struct.");

old_registration_info
} else {
// We failed with the old registration path, let's try the new one
tracing::warn!("Didn't find user in old `Registered` struct, trying new one");
tracing::warn!("Didn't find user in old `Registered` struct, trying new one.");

let registered_info_query =
entropy::storage().registry().registered_on_chain(BoundedVec(verifying_key));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,8 @@ pub enum ProtocolErr {
SubstrateClient(#[from] entropy_client::substrate::SubstrateError),
#[error("Listener: {0}")]
Listener(#[from] entropy_protocol::errors::ListenerErr),
#[error("Failed to derive BIP-32 account: {0}")]
Bip32DerivationError(#[from] bip32::Error),
}

impl IntoResponse for ProtocolErr {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -63,18 +63,34 @@ impl<'a> ThresholdSigningService<'a> {
fields(sign_init),
level = tracing::Level::DEBUG
)]
pub async fn get_sign_context(&self, sign_init: SignInit) -> Result<SignContext, ProtocolErr> {
pub async fn get_sign_context(
&self,
sign_init: SignInit,
derivation_path: Option<bip32::DerivationPath>,
) -> Result<SignContext, ProtocolErr> {
tracing::debug!("Getting signing context");
let key_share_and_aux_info_vec = self
.kv_manager
.kv()
.get(&hex::encode(sign_init.signing_session_info.signature_verifying_key.clone()))
.await?;

let verifying_key = if derivation_path.is_some() {
entropy_shared::NETWORK_PARENT_KEY.as_bytes().to_vec()
} else {
sign_init.signing_session_info.signature_verifying_key.clone()
};

let key_share_and_aux_info_vec =
self.kv_manager.kv().get(&hex::encode(verifying_key)).await?;

let (key_share, aux_info): (
ThresholdKeyShare<KeyParams, PartyId>,
AuxInfo<KeyParams, PartyId>,
) = entropy_kvdb::kv_manager::helpers::deserialize(&key_share_and_aux_info_vec)
.ok_or_else(|| ProtocolErr::Deserialization("Failed to load KeyShare".into()))?;

let key_share = if let Some(path) = derivation_path {
key_share.derive_bip32(&path)?
} else {
key_share
};

Ok(SignContext::new(sign_init, key_share, aux_info))
}

Expand All @@ -92,6 +108,7 @@ impl<'a> ThresholdSigningService<'a> {
threshold_signer: &sr25519::Pair,
threshold_accounts: Vec<AccountId32>,
) -> Result<RecoverableSignature, ProtocolErr> {
tracing::debug!("Executing signing session");
tracing::trace!("Signing info {session_id:?}");

let message_hash = if let SessionId::Sign(session_info) = &session_id {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,8 @@ pub async fn open_protocol_connections(
state: &ListenerState,
x25519_secret_key: &x25519_dalek::StaticSecret,
) -> Result<(), ProtocolErr> {
tracing::debug!("Opening protocol connections");

let connect_to_validators = validators_info
.iter()
.filter(|validators_info| {
Expand Down
51 changes: 38 additions & 13 deletions crates/threshold-signature-server/src/user/api.rs
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ pub async fn sign_tx(
.number;

check_stale(user_sig_req.block_number, block_number).await?;

// Probably impossible but block signing from parent key anyways
if string_verifying_key == hex::encode(NETWORK_PARENT_KEY) {
return Err(UserErr::NoSigningFromParentKey);
Expand All @@ -159,7 +160,9 @@ pub async fn sign_tx(
if user_details.programs_data.0.is_empty() {
return Err(UserErr::NoProgramPointerDefined());
}
// handle aux data padding, if it is not explicit by client for ease send through None, error if incorrect length

// Handle aux data padding, if it is not explicit by client for ease send through None, error
// if incorrect length
let auxilary_data_vec;
if let Some(auxilary_data) = user_sig_req.clone().auxilary_data {
if auxilary_data.len() < user_details.programs_data.0.len() {
Expand All @@ -170,6 +173,7 @@ pub async fn sign_tx(
} else {
auxilary_data_vec = vec![None; user_details.programs_data.0.len()];
}

// gets fuel from chain
let max_instructions_per_programs_query =
entropy::storage().parameters().max_instructions_per_programs();
Expand All @@ -186,7 +190,9 @@ pub async fn sign_tx(
runtime.evaluate(&program, &signature_request, Some(&program_info.program_config), None)?;
}

let signers = get_signers_from_chain(&api, &rpc).await?;
let with_parent_key = user_details.derivation_path.is_some();
let signers = get_signers_from_chain(&api, &rpc, with_parent_key).await?;

// Use the validator info from chain as we can be sure it is in the correct order and the
// details are correct
user_sig_req.validators_info = signers;
Expand All @@ -207,22 +213,41 @@ pub async fn sign_tx(
request_author,
};

let _has_key = check_for_key(&string_verifying_key, &app_state.kv_store).await?;
// In the new registration flow we don't store the verifying key in the KVDB, so we only do this
// check if we're using the old registration flow
Copy link
Contributor

Choose a reason for hiding this comment

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

But we could still check here if we are currently in the signing committee. But i guess theres no point as we are about to attempt to get the parent keyshare in get_sign_context anyway

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We weren't checking for inclusion in the signing committee here anyways, so maybe I'm misunderstanding your comment

if user_details.derivation_path.is_none() {
let _has_key = check_for_key(&string_verifying_key, &app_state.kv_store).await?;
}

let derivation_path = if let Some(path) = user_details.derivation_path {
let decoded_path = String::decode(&mut path.as_ref())?;
let path = bip32::DerivationPath::from_str(&decoded_path)?;

Some(path)
} else {
None
};

let (mut response_tx, response_rx) = mpsc::channel(1);

// Do the signing protocol in another task, so we can already respond
tokio::spawn(async move {
let signing_protocol_output =
do_signing(&rpc, user_sig_req, &app_state, signing_session_id, request_limit)
.await
.map(|signature| {
(
BASE64_STANDARD.encode(signature.to_rsv_bytes()),
signer.signer().sign(&signature.to_rsv_bytes()),
)
})
.map_err(|error| error.to_string());
let signing_protocol_output = do_signing(
&rpc,
user_sig_req,
&app_state,
signing_session_id,
request_limit,
derivation_path,
)
.await
.map(|signature| {
(
BASE64_STANDARD.encode(signature.to_rsv_bytes()),
signer.signer().sign(&signature.to_rsv_bytes()),
)
})
.map_err(|error| error.to_string());

// This response chunk is sent later with the result of the signing protocol
if response_tx.try_send(serde_json::to_string(&signing_protocol_output)).is_err() {
Expand Down
2 changes: 2 additions & 0 deletions crates/threshold-signature-server/src/user/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -167,6 +167,8 @@ pub enum UserErr {
SubgroupGet(#[from] entropy_client::user::SubgroupGetError),
#[error("Unknown hashing algorthim - user is using a newer version than us")]
UnknownHashingAlgorithm,
#[error("Failed to derive BIP-32 account: {0}")]
Bip32DerivationError(#[from] bip32::Error),
}

impl From<hkdf::InvalidLength> for UserErr {
Expand Down
Loading
Loading