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: DKG verification pre-key-rotation #1301

Draft
wants to merge 22 commits into
base: feat/require-all-signatures-for-rotate-keys
Choose a base branch
from
Draft
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
10 changes: 10 additions & 0 deletions Cargo.lock

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

1 change: 1 addition & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@ backoff = { version = "0.4.0", default-features = false, features = ["tokio"] }
base64 = { version = "0.22.1", default-features = false, features = ["alloc"] }
bincode = { version = "1.3.3", default-features = false }
bitcoin = { version = "0.32.5", default-features = false, features = ["serde", "rand-std"] }
bitcoinconsensus = { version = "0.106.0", default-features = false }
bitcoincore-rpc = { version = "0.19.0", default-features = false }
bitcoincore-rpc-json = { version = "0.19.0", default-features = false }
bitcoincore-zmq = { version = "1.5.2", default-features = false, features = ["async"] }
Expand Down
1 change: 1 addition & 0 deletions signer/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ testing = ["fake", "mockall", "sbtc/testing"]
aquamarine.workspace = true
axum.workspace = true
bitcoin.workspace = true
bitcoinconsensus.workspace = true
bitcoincore-rpc.workspace = true
bitcoincore-rpc-json.workspace = true
bitcoincore-zmq.workspace = true
Expand Down
75 changes: 75 additions & 0 deletions signer/migrations/0012__dkg_verification_extensions.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,75 @@
-- Enum table for DKG shares status
CREATE TABLE sbtc_signer.dkg_shares_status (
-- The id of the status, not auto-incremented as we want to control the values.
id INTEGER PRIMARY KEY,
-- The name of the status.
key TEXT NOT NULL,
-- Brief description of what the status means.
description TEXT NOT NULL
);

-- Insert the initial entries.
INSERT INTO sbtc_signer.dkg_shares_status (id, key, description) VALUES
(0, 'PENDING', 'DKG round successful, pending verification via signing round'),
(1, 'VERIFIED', 'Successfully verified via signing round'),
(2, 'KEY_REVOKED', 'The DKG key has been revoked and should not be used');

-- Add the new columns to the `dkg_shares` table. We're not adding indexes for
-- now because the table is so small that the overhead likely outweighs the
-- benefits.
ALTER TABLE sbtc_signer.dkg_shares
-- Contains the current
ADD COLUMN dkg_shares_status_id INTEGER DEFAULT 0 REFERENCES sbtc_signer.dkg_shares_status (id),
ADD COLUMN verified_at_bitcoin_block_hash BYTEA DEFAULT NULL,
ADD COLUMN verified_at_bitcoin_block_height BIGINT DEFAULT NULL,
ADD CONSTRAINT fk_dkg_shares_bitcoin_block_hash
FOREIGN KEY (verified_at_bitcoin_block_hash)
REFERENCES sbtc_signer.bitcoin_blocks (block_hash),
ADD CONSTRAINT chk_verified_at
CHECK (
(dkg_shares_status_id = 1 AND verified_at_bitcoin_block_hash IS NOT NULL AND verified_at_bitcoin_block_height IS NOT NULL)
OR (dkg_shares_status_id <> 1 AND verified_at_bitcoin_block_hash IS NULL AND verified_at_bitcoin_block_height IS NULL)
);

-- Set all of the current `dkg_shares` to `3` (revoked) to start with. Confirmed
-- DKG shares will be updated to `1` (verified) in the next step.
UPDATE sbtc_signer.dkg_shares
SET dkg_shares_status_id = 3;

-- Update the `dkg_shares` which have been included in a
-- `rotate_keys_transactions` which can also be tied to a bitcoin block to `1`
-- (verified) and set the `verified_at_*` fields to the bitcoin block
-- hash/height corresponding to the block where these were anchored.
--
-- This update is not fork aware, but at the time of writing there is no forks
-- that should be problematic (i.e. we shouldn't have any rotate-keys events
-- that have been orphaned).
WITH updated_shares AS (
SELECT
s.aggregate_key,
bb.block_hash AS verified_at_bitcoin_block_hash,
bb.block_height AS verified_at_bitcoin_block_height
FROM sbtc_signer.dkg_shares s
INNER JOIN sbtc_signer.rotate_keys_transactions rkt
ON s.aggregate_key = rkt.aggregate_key
INNER JOIN sbtc_signer.stacks_transactions stx
ON rkt.txid = stx.txid
INNER JOIN sbtc_signer.stacks_blocks sb
ON stx.block_hash = sb.block_hash
INNER JOIN sbtc_signer.bitcoin_blocks bb
ON sb.bitcoin_anchor = bb.block_hash
ORDER BY bb.block_height DESC
LIMIT 1
)
UPDATE sbtc_signer.dkg_shares
SET
dkg_shares_status_id = 1,
verified_at_bitcoin_block_hash = updated_shares.verified_at_bitcoin_block_hash,
verified_at_bitcoin_block_height = updated_shares.verified_at_bitcoin_block_height
FROM updated_shares
WHERE sbtc_signer.dkg_shares.aggregate_key = updated_shares.aggregate_key;

-- Make the `dkg_shares_status_id` column `NOT NULL` now that they should all
-- have a value.
ALTER TABLE sbtc_signer.dkg_shares
ALTER COLUMN dkg_shares_status_id SET NOT NULL;
188 changes: 188 additions & 0 deletions signer/src/bitcoin/utxo.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ use std::ops::Deref as _;
use std::sync::LazyLock;

use bitcoin::absolute::LockTime;
use bitcoin::consensus::Encodable;
use bitcoin::hashes::Hash as _;
use bitcoin::sighash::Prevouts;
use bitcoin::sighash::SighashCache;
Expand Down Expand Up @@ -712,6 +713,22 @@ impl SignerUtxo {
}
}

/// A struct for constructing a mock transaction that can be signed. This is
/// used as part of the verification process after a new DKG round has been
/// completed.
///
/// The Bitcoin transaction has the following layout:
/// 1. The first input is spending the signers' UTXO.
/// 2. There is only one output which is an OP_RETURN with the bytes [0x01,
/// 0x02, 0x03] as the data and amount equal to the UTXO's value (i.e. the
/// transaction has a zero-fee).
pub struct UnsignedMockTransaction {
/// The Bitcoin transaction that needs to be signed.
tx: Transaction,
/// The signers' UTXO used as an input to this transaction.
utxo: SignerUtxo,
}

/// Given a set of requests, create a BTC transaction that can be signed.
///
/// This BTC transaction in this struct has correct amounts but no witness
Expand Down Expand Up @@ -802,6 +819,94 @@ impl SignatureHashes<'_> {
}
}

impl UnsignedMockTransaction {
const AMOUNT: u64 = 0;

/// Construct an unsigned mock transaction.
///
/// This will use the provided `aggregate_key` and `amount` to
/// construct a [`Transaction`] with a single input and output.
pub fn new(signer_public_key: XOnlyPublicKey) -> Self {
let utxo = SignerUtxo {
outpoint: OutPoint::null(),
amount: Self::AMOUNT,
public_key: signer_public_key,
};

let tx = Transaction {
version: Version::TWO,
lock_time: LockTime::ZERO,
input: vec![utxo.as_tx_input(&DUMMY_SIGNATURE)],
output: vec![TxOut {
value: Amount::from_sat(Self::AMOUNT),
script_pubkey: ScriptBuf::new_op_return([]),
}],
};

Self { tx, utxo }
}

/// Gets the sighash for the signers' input UTXO which needs to be signed
/// before the transaction can be broadcast.
pub fn compute_sighash(&self) -> Result<TapSighash, Error> {
let prevouts = [self.utxo.as_tx_output()];
let mut sighasher = SighashCache::new(&self.tx);

sighasher
.taproot_key_spend_signature_hash(0, &Prevouts::All(&prevouts), TapSighashType::All)
.map_err(Into::into)
}

/// Tests if the provided taproot [`Signature`] is valid for spending the
/// signers' UTXO. This function will return [`Error::BitcoinConsensus`]
/// error if the signature fails verification, passing the underlying error
/// from [`bitcoinconsensus`].
pub fn verify_signature(&self, signature: &Signature) -> Result<(), Error> {
// Create a copy of the transaction so that we don't modify the
// transaction stored in the struct.
let mut tx = self.tx.clone();

// Set the witness data on the input from the provided signature.
tx.input[0].witness = Witness::p2tr_key_spend(signature);

// Encode the transaction to bytes (needed by the bitcoinconsensus
// library).
let mut tx_bytes: Vec<u8> = Vec::new();
tx.consensus_encode(&mut tx_bytes)
.map_err(Error::BitcoinIo)?;

// Get the prevout for the signers' UTXO.
let prevout = self.utxo.as_tx_output();
let prevout_script_bytes = prevout.script_pubkey.as_script().as_bytes();

// Create the bitcoinconsensus UTXO object.
let prevout_utxo = bitcoinconsensus::Utxo {
script_pubkey: prevout_script_bytes.as_ptr(),
script_pubkey_len: prevout_script_bytes.len() as u32,
value: Self::AMOUNT as i64,
};

// We specify the flags to include all pre-taproot and taproot
// verifications explicitly.
// https://github.com/rust-bitcoin/rust-bitcoinconsensus/blob/master/src/lib.rs
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why include pre-taproot?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had tried it without and it fails without the other flags; it seems like there's some sort of roll-up. They have a method for getting the flags based on block height, where they additively enable each flag per activation block height. This is what they do in their tests, anyway.

let flags = bitcoinconsensus::VERIFY_ALL_PRE_TAPROOT | bitcoinconsensus::VERIFY_TAPROOT;

// Verify that the transaction updated with the provided signature can
// successfully spend the signers' UTXO. Note that the amount is not
// used in the verification process for taproot spends, only the
// signature.
bitcoinconsensus::verify_with_flags(
prevout_script_bytes,
Self::AMOUNT,
&tx_bytes,
Some(&[prevout_utxo]),
0,
flags,
)
.map_err(Error::BitcoinConsensus)
}
}

impl<'a> UnsignedTransaction<'a> {
/// Construct an unsigned transaction.
///
Expand Down Expand Up @@ -1457,6 +1562,7 @@ mod tests {
use std::str::FromStr;

use super::*;
use bitcoin::key::TapTweak;
use bitcoin::BlockHash;
use bitcoin::CompressedPublicKey;
use bitcoin::Txid;
Expand Down Expand Up @@ -1652,6 +1758,88 @@ mod tests {
}
}

/// This test verifies that our implementation of Bitcoin script
/// verification using [`bitcoinconsensus`] works as expected. This
/// functionality is used in the verification of WSTS signing after a new
/// DKG round has completed.
#[test]
fn mock_signer_utxo_signing_and_spending_verification() {
let secp = secp256k1::Secp256k1::new();

// Generate a key pair which will serve as the signers' aggregate key.
let secret_key = SecretKey::new(&mut OsRng);
let keypair = secp256k1::Keypair::from_secret_key(&secp, &secret_key);
let tweaked = keypair.tap_tweak(&secp, None);
let (aggregate_key, _) = keypair.x_only_public_key();

// Create a new transaction using the aggregate key.
let unsigned = UnsignedMockTransaction::new(aggregate_key);

let tapsig = unsigned
.compute_sighash()
.expect("failed to compute taproot sighash");

// Sign the taproot sighash.
let message = secp256k1::Message::from_digest_slice(tapsig.as_byte_array())
.expect("Failed to create message");

// [1] Verify the correct signature, which should succeed.
let schnorr_sig = secp.sign_schnorr(&message, &tweaked.to_inner());
let taproot_sig = bitcoin::taproot::Signature {
signature: schnorr_sig,
sighash_type: TapSighashType::All,
};
unsigned
.verify_signature(&taproot_sig)
.expect("signature verification failed");

// [2] Verify the correct signature, but with a different sighash type,
// which should fail.
let taproot_sig = bitcoin::taproot::Signature {
signature: schnorr_sig,
sighash_type: TapSighashType::None,
};
unsigned
.verify_signature(&taproot_sig)
.expect_err("signature verification should have failed");

// [3] Verify an incorrect signature with the correct sighash type,
// which should fail. In this case we've created the signature using
// the untweaked keypair.
let schnorr_sig = secp.sign_schnorr(&message, &keypair);
let taproot_sig = bitcoin::taproot::Signature {
signature: schnorr_sig,
sighash_type: TapSighashType::All,
};
unsigned
.verify_signature(&taproot_sig)
.expect_err("signature verification should have failed");

// [4] Verify an incorrect signature with the correct sighash type, which
// should fail. In this case we use a completely newly generated keypair.
let secret_key = SecretKey::new(&mut OsRng);
let keypair = secp256k1::Keypair::from_secret_key(&secp, &secret_key);
let schnorr_sig = secp.sign_schnorr(&message, &keypair);
let taproot_sig = bitcoin::taproot::Signature {
signature: schnorr_sig,
sighash_type: TapSighashType::All,
};
Comment on lines +1821 to +1826
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we also test with tap_tweak here? To be sure this is not failing because of an untweaked 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.

Yup, added in 4642e66

unsigned
.verify_signature(&taproot_sig)
.expect_err("signature verification should have failed");

// [5] Same as [4], but using its tweaked key.
let tweaked = keypair.tap_tweak(&secp, None);
let schnorr_sig = secp.sign_schnorr(&message, &tweaked.to_inner());
let taproot_sig = bitcoin::taproot::Signature {
signature: schnorr_sig,
sighash_type: TapSighashType::All,
};
unsigned
.verify_signature(&taproot_sig)
.expect_err("signature verification should have failed");
}

#[ignore = "For generating the SOLO_(DEPOSIT|WITHDRAWAL)_SIZE constants"]
#[test]
fn create_deposit_only_tx() {
Expand Down
2 changes: 2 additions & 0 deletions signer/src/block_observer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -694,6 +694,7 @@ mod tests {
use crate::keys::PublicKey;
use crate::keys::SignerScriptPubKey as _;
use crate::storage;
use crate::storage::model::DkgSharesStatus;
use crate::testing::block_observer::TestHarness;
use crate::testing::context::*;

Expand Down Expand Up @@ -996,6 +997,7 @@ mod tests {
public_shares: Vec::new(),
signer_set_public_keys: vec![aggregate_key],
signature_share_threshold: 1,
status: DkgSharesStatus::Pending,
};
storage.write_encrypted_dkg_shares(&shares).await.unwrap();

Expand Down
Loading