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

feat/modify rotate key to ask for all signatures #1315

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 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
8 changes: 8 additions & 0 deletions signer/src/stacks/contracts.rs
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,14 @@ pub enum StacksTx {
SmartContract(SmartContract),
}

impl StacksTx {
/// Returns true if the transaction is a rotate keys transaction, regardless
/// of the version.
pub fn is_rotate_keys(&self) -> bool {
matches!(self, StacksTx::ContractCall(ContractCall::RotateKeysV1(_)))
}
}
Comment on lines +207 to +212
Copy link
Collaborator

Choose a reason for hiding this comment

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

Enum variant name RotateKeysV1 and "regardless of the version" comment looks a bit confusing. Maybe something like

/// Returns true if the transaction is a rotate keys transaction, regardless
/// of it's contens.


impl AsTxPayload for StacksTx {
fn tx_payload(&self) -> TransactionPayload {
match self {
Expand Down
5 changes: 5 additions & 0 deletions signer/src/stacks/wallet.rs
Original file line number Diff line number Diff line change
Expand Up @@ -345,6 +345,11 @@ impl MultisigTx {
&self.tx
}

/// Return the message digest associated with the transaction
pub fn digest(&self) -> &Message {
&self.digest
}

/// Return the total number of signatures that have been received so
/// far for this transaction.
pub fn num_signatures(&self) -> u16 {
Expand Down
40 changes: 39 additions & 1 deletion signer/src/transaction_coordinator.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ use crate::metrics::Metrics;
use crate::metrics::BITCOIN_BLOCKCHAIN;
use crate::metrics::STACKS_BLOCKCHAIN;
use crate::network;
use crate::signature::RecoverableEcdsaSignature as _;
use crate::signature::TaprootSignature;
use crate::stacks::api::FeePriority;
use crate::stacks::api::GetNakamotoStartHeight;
Expand Down Expand Up @@ -900,6 +901,22 @@ where
) -> Result<StacksTransaction, Error> {
let txid = req.txid;

// Determine the number of signatures required for the transaction. If
// the transaction is a rotate keys transaction, then we require all
// signers to sign the transaction to ensure that all signers have
// successfully finished DKG. Otherwise, we only need the number of
// signatures required by the wallet.
//
// Note: this check is naive and assumes that signers are correctly
// validating the new aggregate key. A malicious/faulty signer can
// still sign the transaction as the signer simply signs the transaction
// using their configured private key.
let signatures_required = if req.contract_tx.is_rotate_keys() {
wallet.num_signers()
} else {
wallet.signatures_required()
} as usize;

// We ask for the signers to sign our transaction (including
// ourselves, via our tx signer event loop)
self.send_message(req, chain_tip).await?;
Expand All @@ -913,7 +930,15 @@ where
tokio::pin!(signal_stream);

let future = async {
while multi_tx.num_signatures() < wallet.signatures_required() {
let mut pending_signers = wallet.public_keys().clone();

// This serves as a "super-condition" relative to `multi_tx.num_signatures() < wallet.signatures_required()`:
// - We start with a full set of expected signers `pending_signers`.
// - Each valid signature is verified using `recover_ecdsa(multi_tx.digest())`, ensuring that only the
// actual signers of the expected transaction digest can remove themselves from `pending_signers`.
// - We stop collecting signatures once we have enough, but keep tracking responses from remaining signers
// for key rotation transactions.
while wallet.public_keys().len() - pending_signers.len() < signatures_required {
matteojug marked this conversation as resolved.
Show resolved Hide resolved
matteojug marked this conversation as resolved.
Show resolved Hide resolved
// If signal_stream.next() returns None then one of the
// underlying streams has closed. That means either the
// network stream, the internal message stream, or the
Expand All @@ -938,6 +963,19 @@ where
_ => continue,
};

let enough_signatures = multi_tx.num_signatures() >= wallet.signatures_required();
let recovered_key = sig.signature.recover_ecdsa(multi_tx.digest());

if let Ok(key) = recovered_key {
pending_signers.remove(&key);
}

// Stop collecting signatures once we have enough, but keep tracking responses
// from remaining signers for key rotation transactions
if enough_signatures {
continue;
}

if let Err(error) = multi_tx.add_signature(sig.signature) {
tracing::warn!(
%txid,
Expand Down