Implement full DkgSign over StackerDB #3911
Conversation
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
… crypto while jude and jacinta fix the networking chicken-egg problem
Codecov Report
@@ Coverage Diff @@
## feat/stacks-signer #3911 +/- ##
======================================================
- Coverage 0.16% 0.16% -0.01%
======================================================
Files 332 339 +7
Lines 288692 290170 +1478
======================================================
Hits 469 469
- Misses 288223 289701 +1478
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
| /// the 32 byte ECDSA private key used to sign blocks, chunks, and transactions | ||
| pub message_private_key: String, | ||
| /// The hex representation of the signer's Stacks private key | ||
| pub stacks_private_key: String, |
There was a problem hiding this comment.
I'm having a hard time understanding the difference between message_private_key and stacks_private_key from just these comments. Can you help me understand why there need to be two private keys?
There was a problem hiding this comment.
stacks_private_key controls money, message_private_key just lets you sign FROST messages. Users might have very different protection strategies for each.
One of my tasks for Nakomoto is to review how we store private keys, and offer various strategies for keeping them safe. Ideally we want to allow users to keep their stacks_private_keys in an HSM; all private keys should at least be trivially encrypted locally.
Having separate private keys from the start means we don't have to change this later.
There was a problem hiding this comment.
I can update the comment on the stacks_private_key as well to indicate what it is used for if that helps (communicating with the stacks node/stacker db contract)?
There was a problem hiding this comment.
Thanks for updating the comment @jferrant!
@xoloki I agree in principle that the keys ought to be separate and have different storage strategies, but I'm curious why the signer needs to have a key that controls large amounts of STX in the first place? When the user stacks their STX in pox-4, they'd additionally set a dedicated signing key for the purposes of signing blocks, chunks, and transactions (i.e. the counterpart to message_private_key). The STX would presumably be managed by the user's wallet, not the signer. Additionally, the PoX rewards would be sent to a (separate) address of the user's choosing, which almost certainly would not be under the control of the signer. Please help me understand what I'm missing?
There was a problem hiding this comment.
Totally possible that I'm making bad assumptions here. I though the way PoX worked was that users stacked significant (~100k) STX for each reward slot, and that the STX address associated with the stacking wallets would be the same STX address that Signers will use to talk to sbtc contracts etc.
If this is a bad assumption, and the STX address the Signer will use with stackerDB is not the address which controls the large STX stake, then yeah we can just use the STX private key for wsts message signing ECDSA too.
There was a problem hiding this comment.
The users don't stack to specific reward slots per se; they just stack a large quantity of STX, and depending on how much STX they stacked relative to everyone else, they are allotted zero or more reward slots by the system at the start of the next reward cycle.
That said, the key that holds the STX and performs the stacking operation would not need to be known to the signer. Instead, the act of stacking from this high-value key would assign a different block-signing key for the signer to use. Depending on what you think about this, it may even be desirable to have the user delegate two keys to the signer: one for signing Stacks blocks, and one for signing Bitcoin sBTC transactions. But in no case would the signer ever possess the key that controls the user's STX.
…e for DKG even y coord incrementation bug Signed-off-by: Jacinta Ferrant <[email protected]>
| ); | ||
| } | ||
| if self.ids_to_await.is_empty() { | ||
| // Calculate the aggregate signature |
There was a problem hiding this comment.
Would it make sense to use a draining iterator for the following? This state won't be used again in this signing round, right? It would save you some clone()s and ensure this signature generation step only happens at most once.
| } | ||
|
|
||
| /// Helper function to determine the slot ID for the provided stacker-db writer id and the message type | ||
| fn slot_id(id: u32, message: &MessageTypes) -> u32 { |
There was a problem hiding this comment.
Linking the comment I had on the other PR here for visibility: #3923 (comment)
I get something similar when building on Alpine. It seems that CI needs to be updated to install |
|
I only have three remaining concerns before approving:
|
Signed-off-by: Jacinta Ferrant <[email protected]>
…st name Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
…imeout Signed-off-by: Jacinta Ferrant <[email protected]>
I'll fix this today.
See my comment above.
We don't need a set of slots per key_id/reward slot, we just need a set of slots per stacking entity/Signer. This will be more like 100, so with like 8 slots per |
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Signed-off-by: Jacinta Ferrant <[email protected]>
Description
This PR builds on the
test_stackerdb_dkgintegration test, but now it useslibstackerdbandlibsigner. It implements a full state-machine basedstacks-signer, with all of the crypto and networking necessary to run a fullDkgSigncommand.Applicable issues
Additional info (benefits, drawbacks, caveats)
Checklist
docs/rpc/openapi.yamlandrpc-endpoints.mdfor v2 endpoints,event-dispatcher.mdfor new events)clarity-benchmarkingrepobitcoin-tests.yml