feat: bootstrapping distributed key generation (stale) #632

djordon · 2024-10-08T18:42:42Z

Description

Closes #590

Changes

Run DKG is we need to
Expect the public keys of the signers in the config. Not sure if this one will last.

There are still some things that need to be done,

Submitting the rotate-keys transaction. Though I'm not entirely sure that we want to do that.
Change the code to fallback on using the dkg_shares table for the current aggregate key if there is no rotate_keys_transaction in the database.

Testing Information

This also needs tests.

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

instead of that lon error function

transaction coordinator

cylewitruk

Looks good, assuming that the actual logic in wsts_state_machine and wsts is correct :)

cylewitruk · 2024-10-08T18:49:55Z

signer/src/config/default.toml

@@ -132,6 +132,17 @@ deployer = "SN2V7WTJ7BHR03MPHZ1C9A9ZR6NZGR4WM8HT4V67Y"
 # Environment: SIGNER_SIGNER__DB_ENDPOINT
 db_endpoint = "postgresql://postgres:postgres@localhost:5432/signer"

+# The public keys of known signers who are approved to be in the signer
+# set.
+# The signer database endpoint (pgsql connection string)


Snuck in your copy-paste ;)

lol, 🙏🏽 thank you

cylewitruk · 2024-10-08T18:50:55Z

signer/src/config/default.toml

+#
+# Required: true
+# Environment: SIGNER_SIGNER__PEER_PUBLIC_KEYS
+peer_public_keys = [


Yeah, we should try to get rid of this for release, but for now is fine

Yeah. I wasn't sure if we needed this for v1 or not (under the current design). I think the alternative is to have some other mechanism to get this information from their peers.

signer/src/error.rs

signer/src/transaction_coordinator.rs

cylewitruk · 2024-10-08T19:03:16Z

signer/src/transaction_coordinator.rs

+    /// Check whether or not we need to run DKG
+    /// 
+    /// This function checks for the existence of a
+    /// [`RotateKeysTransaction`] in the database, and one does not exist


Suggested change

/// Check whether or not we need to run DKG

///

/// This function checks for the existence of a

/// [`RotateKeysTransaction`] in the database, and one does not exist

/// Check whether or not we need to run DKG

///

/// This function first checks for the existence of a

/// [`RotateKeysTransaction`] row in the database. If one exists, then DKG

/// has already been completed.

///

/// If not, it then proceeds to check for DKG shares in the database for

/// the current signer aggregate key. If DKG shares exist but there is no

/// [`RotateKeysTransaction`] then one needs to be created, otherwise if

/// no shares exist then DKG needs to be run.

perfect, yeah I forgot to update this.

cylewitruk · 2024-10-08T19:05:32Z

signer/src/transaction_coordinator.rs

@@ -553,6 +619,15 @@ where
        PublicKey::from_private_key(&self.private_key)
    }

+    /// This function provides a deterministic 32-byte identifier for the
+    /// signer. This should probably deterministically too.


"This should probably deterministically too" ... ? 😆

lol will update.

matteojug · 2024-10-08T18:52:30Z

signer/src/config/default.toml

@@ -132,6 +132,17 @@ deployer = "SN2V7WTJ7BHR03MPHZ1C9A9ZR6NZGR4WM8HT4V67Y"
 # Environment: SIGNER_SIGNER__DB_ENDPOINT
 db_endpoint = "postgresql://postgres:postgres@localhost:5432/signer"

+# The public keys of known signers who are approved to be in the signer
+# set.
+# The signer database endpoint (pgsql connection string)


nit: leftover line

signer/src/transaction_coordinator.rs

matteojug · 2024-10-08T19:04:31Z

signer/src/transaction_coordinator.rs

@@ -169,6 +177,12 @@ where
            .await?
            .ok_or(Error::NoChainTip)?;

+        if self.needs_dkg(&bitcoin_chain_tip).await? == DkgState::NeedsDkg {


Since this is not guarded by some self.is_coordinator condition, do we risk to have concurrent DKGs?

Oh, I just forgot to add that in.

matteojug · 2024-10-08T19:24:58Z

signer/src/transaction_coordinator.rs

+        &mut self,
+        chain_tip: &model::BitcoinBlockHash,
+    ) -> Result<PublicKey, Error> {
+        let mut state_machine = CoordinatorStateMachine::new([], self.threshold, self.private_key);


sanity check: is the [ ] just a wip thing?

yeah, forgot to change that.

matteojug · 2024-10-08T19:26:49Z

signer/src/transaction_coordinator.rs

@@ -553,6 +619,15 @@ where
        PublicKey::from_private_key(&self.private_key)
    }

+    /// This function provides a deterministic 32-byte identifier for the
+    /// signer. This should probably deterministically too.


nit: This should probably deterministically too I guess a word is missing somewhere, or is a repetition of the deterministic above

lol yeah typo.

along with the associated public key.

shares from the database, use in some key places

Thanks you

the signing set

is no key roation transaction yet.

test private key

djordon · 2024-10-12T05:42:18Z

This has been superseded by #662 and #655.

djordon added 5 commits October 8, 2024 00:32

Use an error function implementation

b1fd36d

instead of that lon error function

Add a timeout field for running dkg

c1606d1

Add a function for running DGK into the

c703c51

transaction coordinator

Expect seed public keys

a563f76

Condition DKG on whether it's needed

9de35c8

djordon changed the title ~~590 bootstrapping distributed key generation~~ feat: bootstrapping distributed key generation Oct 8, 2024

cylewitruk reviewed Oct 8, 2024

View reviewed changes

matteojug reviewed Oct 8, 2024

View reviewed changes

djordon added 3 commits October 8, 2024 16:25

Add the signing set as a column,

d740f97

along with the associated public key.

Sort the keys, why not

e29610c

Add a new query for fetching encrypted

142823a

shares from the database, use in some key places

AshtonStephens force-pushed the 590-bootstrapping-distributed-key-generation branch from 142823a to e4a2b7d Compare October 9, 2024 11:56

Apply Cyle's fix for the integration test.

2c28d6e

Thanks you

djordon force-pushed the 590-bootstrapping-distributed-key-generation branch from a5fa87d to 2c28d6e Compare October 9, 2024 13:35

djordon added 7 commits October 10, 2024 10:54

Merge branch 'main' into 590-bootstrapping-distributed-key-generation

a3ae793

match testing config details after noting

db1a27f

the signing set

fixup

8032c07

Updates after signature change

30a49a0

Fallback to the bootstrap wallet if there

4913e77

is no key roation transaction yet.

code quality clean up

da0c6ab

Forgot to update the test fixtures and

b13ed09

test private key

djordon changed the title ~~feat: bootstrapping distributed key generation~~ feat: bootstrapping distributed key generation (stale) Oct 12, 2024

djordon closed this Oct 12, 2024

djordon deleted the 590-bootstrapping-distributed-key-generation branch October 12, 2024 05:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: bootstrapping distributed key generation (stale) #632

feat: bootstrapping distributed key generation (stale) #632

djordon commented Oct 8, 2024 •

edited

Loading

cylewitruk left a comment

cylewitruk Oct 8, 2024

djordon Oct 8, 2024

cylewitruk Oct 8, 2024

djordon Oct 8, 2024

cylewitruk Oct 8, 2024

djordon Oct 8, 2024

cylewitruk Oct 8, 2024

djordon Oct 8, 2024

matteojug Oct 8, 2024

matteojug Oct 8, 2024

djordon Oct 8, 2024

matteojug Oct 8, 2024

djordon Oct 8, 2024

matteojug Oct 8, 2024

djordon Oct 8, 2024

djordon commented Oct 12, 2024

-    /// Check whether or not we need to run DKG
-    ///
-    /// This function checks for the existence of a
-    /// [`RotateKeysTransaction`] in the database, and one does not exist
+    /// Check whether or not we need to run DKG
+    ///
+    /// This function first checks for the existence of a
+    /// [`RotateKeysTransaction`] row in the database. If one exists, then DKG
+    /// has already been completed.
+    ///
+    /// If not, it then proceeds to check for DKG shares in the database for
+    /// the current signer aggregate key. If DKG shares exist but there is no
+    /// [`RotateKeysTransaction`] then one needs to be created, otherwise if
+    /// no shares exist then DKG needs to be run.

feat: bootstrapping distributed key generation (stale) #632

feat: bootstrapping distributed key generation (stale) #632

Conversation

djordon commented Oct 8, 2024 • edited Loading

Description

Changes

Testing Information

Checklist:

cylewitruk 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djordon commented Oct 12, 2024

djordon commented Oct 8, 2024 •

edited

Loading