feat: DKG verification pre-key-rotation #1301

cylewitruk · 2025-02-04T13:05:15Z

Description

Closes: #1300 (Alternative 3)

⚠️ This PR targets #1285 which in turn targets #1287 and needs to merge prior to either of those.

I'm still actively working on this but feel free to comment

Changes

Testing Information

Checklist:

I have performed a self-review of my code
My changes generate no new warnings
New and existing unit tests pass locally with my changes
Any dependent changes have been merged and published in downstream modules

…mock-signing

djordon

Hey, nice work. Have have some high level comments:

Why add a new table? Why not just a simple is_verified boolean column that is nullable, where NULL means that verification has not been attempted, false means that verification has been attempted and failed and true means that it has been successfully verified. It's much simplier, it's intuitive (to me at least), and gives us the functionality that we need.
Why support revocation of the keys? Once they're verified they'll stay verified. I would think that verification is a one way street, it goes from unknown to either passing or failing verification. Than again, maybe this is a language thing.
I would add column comments in the database instead of a description field, with COMMENT ON COLUMN blah IS 'some comment'. If we do not want to add column comments, then we can just use regular code comments.
Why not use the bitcoin::Transaction::verify_with_flags function in verify_signature? It's much simpler and does the same thing.
I feel like the block hash and block height columns in the dkg_shares should be linked to the started at height, not the ended/verified at. In the future, we should support DKG ending on a block that is different from the block that it started at. If we do that, it's possible for signers to have ending blocks (so different verified at blocks). But, maybe this is really just something to do with naming rather than meaning?

djordon · 2025-02-04T15:28:33Z

signer/src/bitcoin/utxo.rs

+    /// construct a [`Transaction`] with a single input and output.
+    pub fn new(signer_public_key: XOnlyPublicKey) -> Self {
+        let utxo = SignerUtxo {
+            outpoint: OutPoint::new(Txid::all_zeros(), 0),


This is fine, but OutPoint::null() is even simpler.

djordon · 2025-02-04T15:29:06Z

signer/src/bitcoin/utxo.rs

+            input: vec![utxo.as_tx_input(&DUMMY_SIGNATURE)],
+            output: vec![TxOut {
+                value: Amount::from_sat(Self::AMOUNT),
+                script_pubkey: ScriptBuf::new_op_return([0x01, 0x02, 0x03]),


This is fine, but ScriptBuf::new_op_return([]) is even simpler.

djordon · 2025-02-04T15:38:58Z

signer/src/bitcoin/utxo.rs

+
+        // 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
+        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::bitcoin_consensus)


I think we should simplify the above to

// We specify the flags to include all pre-taproot and taproot // verifications explicitly. // https://github.com/rust-bitcoin/rust-bitcoinconsensus/blob/d1a3554fbf031beb725883b932155f8316887769/src/lib.rs let flags = bitcoinconsensus::VERIFY_ALL_PRE_TAPROOT | bitcoinconsensus::VERIFY_TAPROOT; tx.verify_with_flags(|_| Some(self.utxo.as_tx_output()), flags) .map_err(Error::BitcoinConsensus);

or something similar.

Turns out that this doesn't work, as our unit test does not pass with this change. After some digging, we need a later version of bitcoinconsensus in order for the verification to work, and so we need the changes here.

djordon · 2025-02-04T15:54:43Z

signer/src/error.rs

+    /// Convert a bitcoin consensus error to an `error::Error`
+    pub fn bitcoin_consensus(err: bitcoinconsensus::Error) -> Self {
+        Error::BitcoinConsensus(err)
+    }


nit: this seems unnecessary, we can replace Error::bitcoin_consensus with Error::BitcoinConsensus.

pretty sure i tried that and ended up here, but will try it again

matteojug

General approach LGTM, sending some preliminary comments, but I'm also +1 Daniel points that could simplify some scaffolding in here. I would also add (at query level?) that you can set the verify (being successful or failed) only to pending DKG entries.

matteojug · 2025-02-05T11:02:43Z

signer/src/bitcoin/utxo.rs

+        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,
+        };


Can we also test with tap_tweak here? To be sure this is not failing because of an untweaked key

matteojug · 2025-02-05T11:17:16Z

signer/src/error.rs

+    /// The rotate-key frost verification signing round failed for the aggregate
+    /// key.
+    #[error("rotate-key frost verification signing failed for aggregate key: {0}")]
+    DkgVerificationFailed(Box<PublicKeyXOnly>),


how does this differ from DkgVerificationNotSuccessful?

I was about to ask the same :)

I believe I meant this as "The DKG verification is not successful (yet?)" (i.e. when validating a key-rotation contract call), whereas DkgVerificationFailed is hard fail

matteojug · 2025-02-05T11:43:40Z

signer/src/transaction_coordinator.rs

@@ -703,6 +714,78 @@ where
        Ok(())
    }

+    /// Performs verification of of the DKG process by running a FROST signing


Suggested change

/// Performs verification of of the DKG process by running a FROST signing

/// Performs verification of the DKG process by running a FROST signing

matteojug · 2025-02-05T11:45:24Z

signer/src/transaction_coordinator.rs

+    /// [`UnsignedMockTransaction`] using the new aggregate key using the FROST
+    /// coordinator, which requires 100% signing participation vs. FIRE which
+    /// only uses {threshold} signers.


nit: the last part is a repetition of the paragraph above

matteojug · 2025-02-05T11:53:43Z

signer/src/transaction_coordinator.rs

+        .inspect_err(|error| {
+            tracing::warn!(%error, "🔐 failed to assert that all signers have signed with the new aggregate key; aborting");
+        })?;
+        tracing::info!("🔐 all signers have signed with the new aggregate key; proceeding");


nit: should we verify here that the signature is valid as well? To avoid having the coordinator reporting DKG verification successful and attempting to sign the rotate key (that will fail, as the signers would refuse)

Yeah good idea

WSTS will only return OperationResult::SignTaproot if the signature passes verification. But there's no harm in verifying it again, it only takes milliseconds.

matteojug · 2025-02-05T12:04:59Z

signer/src/transaction_signer.rs

-            Some(OperationResult::SignSchnorr(_)) => {
-                tracing::info!("successfully completed pre-rotate-key validation signing round");
-                self.wsts_frost_results.put(id, true);
+            Some(OperationResult::SignSchnorr(sig) | OperationResult::SignTaproot(sig)) => {


Is SignSchnorr still relevant?

Think I just left it since it's what's done in the coordinator, but we're not requesting a SignatureType::Schnorr anymore so it can probably be removed without issue.

matteojug · 2025-02-05T12:14:49Z

signer/src/transaction_signer.rs

@@ -152,7 +154,7 @@ pub struct TxSignerEventLoop<Context, Network, Rng> {
    /// verification of the Stacks rotate-keys transaction.
    pub wsts_frost_state_machines: LruCache<StateMachineId, FrostCoordinator>,
    /// Results of the FROST verification rounds.
-    pub wsts_frost_results: LruCache<StateMachineId, bool>,
+    pub wsts_frost_mock_txs: LruCache<StateMachineId, UnsignedMockTransaction>,


Should we merge this with the above (ie, LruCache<StateMachineId, (FrostCoordinator, UnsignedMockTransaction)>, or wrapping both in a struct)? So that we are sure we always have both available, given that we have an lru and the state machine usage can be decoupled from the mock transaction we may end up in some strange edge case

Yeah I've been wanting to do this, just haven't gotten to it yet -- will try to get it done soon

xoloki

Looks good modulo a few comments and questions.

xoloki · 2025-02-05T17:59:22Z

Cargo.toml

@@ -108,4 +109,4 @@ split-debuginfo = "unpacked"
 [profile.release]
 lto = "thin"
 codegen-units = 16
-overflow-checks = true
+overflow-checks = true


No newline at EOF

xoloki · 2025-02-05T18:12:03Z

signer/src/bitcoin/utxo.rs

+
+        // 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


Why include pre-taproot?

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.

xoloki · 2025-02-05T18:15:28Z

signer/src/error.rs

+    /// The rotate-key frost verification signing round failed for the aggregate
+    /// key.
+    #[error("rotate-key frost verification signing failed for aggregate key: {0}")]
+    DkgVerificationFailed(Box<PublicKeyXOnly>),


I was about to ask the same :)

xoloki · 2025-02-05T20:05:09Z

signer/src/transaction_coordinator.rs

+        .inspect_err(|error| {
+            tracing::warn!(%error, "🔐 failed to assert that all signers have signed with the new aggregate key; aborting");
+        })?;
+        tracing::info!("🔐 all signers have signed with the new aggregate key; proceeding");


WSTS will only return OperationResult::SignTaproot if the signature passes verification. But there's no harm in verifying it again, it only takes milliseconds.

xoloki · 2025-02-05T20:09:05Z

signer/src/transaction_signer.rs

@@ -714,7 +706,7 @@ where
                let (id, aggregate_key) = match msg.id {
                    WstsMessageId::Dkg(_) => {
                        tracing::warn!(
-                            "received nonce-request for DKG round, which is not supported"
+                            "🔐 received nonce-request for DKG round, which is not supported"


Putting emoji in logs still seems bizarre. Welcome to the future!

…rge tool

…mock-signing

cylewitruk added 11 commits February 1, 2025 01:21

pop stash

b0cf9a2

wip

623bda8

working verifications

1526b83

saving

dc4da30

fixing tests

2c3b83d

seems to work

b7bddbf

remove const generic, use 0 amount and add test for random keypair

6a7073e

vet bitcoinconsensus

3af03bb

working on cleaning up

15bc545

Merge branch 'feat/require-all-signatures-for-rotate-keys' into feat/…

3349fba

…mock-signing

migration needs to update block hash/height

2742370

cylewitruk added sbtc signer binary The sBTC Bootstrap Signer. signer coordination The actions executed by the signer coordinator. key rotation The functionality to rotate a private key for a signer in sBTC-v1. breaking-protocol Breaking protocol changes labels Feb 4, 2025

cylewitruk added this to the sBTC: Deposits milestone Feb 4, 2025

cylewitruk self-assigned this Feb 4, 2025

cylewitruk mentioned this pull request Feb 4, 2025

feat: consensus on successful DKG prior to rotate-keys submission #1285

Open

4 tasks

cylewitruk changed the title ~~Feat/mock signing~~ feat: DKG verification pre-key-rotation Feb 4, 2025

djordon reviewed Feb 4, 2025

View reviewed changes

matteojug reviewed Feb 5, 2025

View reviewed changes

Merge branch 'refactor/wsts-cleanup' into feat/mock-signing

978a83e

xoloki approved these changes Feb 5, 2025

View reviewed changes

cylewitruk added 6 commits February 5, 2025 21:23

import some changes manually from parent branch to help a confused me…

abaf93f

…rge tool

more diff-reducing imports

2aff467

reduce diff

15ca645

Merge branch 'feat/require-all-signatures-for-rotate-keys' into feat/…

13595bf

…mock-signing

add validation for nonceresponse + signatureshareresponse

2c46ea5

newline

3fd47be

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: DKG verification pre-key-rotation #1301

feat: DKG verification pre-key-rotation #1301

cylewitruk commented Feb 4, 2025 •

edited

Loading

djordon left a comment

djordon Feb 4, 2025

djordon Feb 4, 2025

djordon Feb 4, 2025

djordon Feb 4, 2025

djordon Feb 4, 2025

cylewitruk Feb 4, 2025

matteojug left a comment

matteojug Feb 5, 2025

matteojug Feb 5, 2025

xoloki Feb 5, 2025

cylewitruk Feb 5, 2025

matteojug Feb 5, 2025

matteojug Feb 5, 2025

matteojug Feb 5, 2025

cylewitruk Feb 5, 2025

xoloki Feb 5, 2025

matteojug Feb 5, 2025

cylewitruk Feb 5, 2025

matteojug Feb 5, 2025

cylewitruk Feb 5, 2025

xoloki left a comment

xoloki Feb 5, 2025

cylewitruk Feb 5, 2025

xoloki Feb 5, 2025

cylewitruk Feb 5, 2025

xoloki Feb 5, 2025

xoloki Feb 5, 2025

xoloki Feb 5, 2025

	/// Performs verification of of the DKG process by running a FROST signing
	/// Performs verification of the DKG process by running a FROST signing

feat: DKG verification pre-key-rotation #1301

Are you sure you want to change the base?

feat: DKG verification pre-key-rotation #1301

Conversation

cylewitruk commented Feb 4, 2025 • edited Loading

Description

Changes

Testing Information

Checklist:

djordon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matteojug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xoloki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cylewitruk commented Feb 4, 2025 •

edited

Loading