Feat: Miner acts as signer set coordinator during block signing #4481
Conversation
|
Is it not possible to just have a fire coordinator within the miner that triggers a sign round exactly as the signers do now? The only thing that would be different is the miner would not include its own key as part of the CoordinatorConfig/if it did, it would be assigned zero key ids. Do we actually need an aggregator? |
That's basically what |
The miner needs those commitments if it wants to be able to check signature shares when aggregation fails. If you can't check signature shares then you can't identify malicious actors. Which means you can't keep signing blocks. |
kantai
force-pushed
the
feat/miner-coordinator
branch
from
b5f92b2
to
19d5710
Compare
|
@kantai Is this ready for review now? |
No, not yet. I'm working on getting the existing nakamoto integrations tests to work with my changes (in branch |
* Replaces msg_id u32 with an enum for message identification * Adds an additional slot for miner messages * Adds a sync channel for listening to StackerDB events * Adds a StackerDBs method for pushing a chunk locally and emitting event * Uses a new message type to store DKG results, to be read by miners to instantiate coordinator * Uses a test signing channel for nakamoto integration tests * Currently builds with a branch of wsts
kantai
force-pushed
the
feat/miner-coordinator
branch
from
dff7cd1
to
207cb69
Compare
kantai
changed the title
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## next #4481 +/- ##
==========================================
- Coverage 83.42% 83.32% -0.10%
==========================================
Files 453 455 +2
Lines 329589 330179 +590
Branches 323 323
==========================================
+ Hits 274944 275135 +191
- Misses 54637 55036 +399
Partials 8 8
... and 25 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
This PR alters the stacks-node miner thread to act as the signing set coordinator for gettings its own blocks signed. There's a couple hacks here that I know are hacks. I added a note about it below (it has to do with figuring out the current miner's key).
Otherwise, this PR does a handful of things in support of the miner-as-coordinator:
[cfg(test)]code block which checks if the blind signing channel is initialized and uses it for the test if so.Hack: Determining the coordinator public key as a signer
If the active miner is acting as the signer, its public key needs to be used as the coordinator public key. Right now, the way this works is that the signer instance checks if its reward cycle is active, and if so, it uses the most recent miner public key it has received.
It fetches this from the stackerdb chunks messages -- using the most recent received event to get the public key. This is mostly safe: the .miners stackerdb really will only allow the current miner and the prior miner to send events. However, this doesn't deal with issues related to the current miner being either non-responsive or malicious. To deal with that, the signer binary needs to be actually tracking some additional state, like the current burn chain view of the node. That way, the signer can have a designated "current trusted miner", that it only moves forward when a miner is selected. This is true regardless of the changes in this PR, though. The right way to handle this is to subscribe to burn block events, and expand that event's information. I don't necessarily think that needs to be done at the same time as this miner-as-coordinator work, though.
Former Hacks!
These all used to be necessary or hacky things in this branch, but are all no longer necessary or have otherwise been fixed.
Reusing the FIRE coordinator library in the miner
This is the correct thing to do, and the FIRE coordinator is instantiated with sufficient state to be able to detect which signers returned bad data so that it can restart signing with malicious signers removed.
As you could tell by inspecting the newsign_coordinatormodule, the wsts fire coordinator is reused by the miner. I don't think this is necessarily so hack-y (in fact, code reuse is A Good Thing), but it's something to think about. The miner isn't actually involved in, say, DKG, and so the fire coordinator may be a over-parameterized from its perspective, as well as theoretically capable of reaching a bunch of bad internal states (if the miner thread's coordinator was ever in a DKG state, e.g.). You can also see that I needed to make some internal state of the coordinator public, as well as implementing an Aggregator that took the aggregate public key as a given.It's worth thinking about whether or not the miner should actually have its own state machine here.Signers continue processing messages on their local coordinator
This was necessary to get the signers integration tests passing (as well as setting the DKG ID in the miner) without changes to them. This had more to do with the tests than anything else, and I updated the tests. I'm still working through fixing all of the signer tests though.
Removing the signature assembled checks from the signers tests
This isn't really a hack, but instead, the change from signers being the coordinator to the miner being a coordinator breaks some of the assumptions made in the signers tests setup. Namely, they no longer have a channel to a coordinator that assembles the final signature. There's definitely ways to fix these tests so that they still can assert the signatures are in place (like, we can add a variable to the tracking globals used by the miner in other tests), so I can just do that.
My fix was to just use the block production events from the event observer, and assert that the signature on the confirmed block matches the expected aggregate key.