-
Notifications
You must be signed in to change notification settings - Fork 90
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
One config file to rule them all and in cryptography bind them #147
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I left a lot comments. Happy to discuss outside of GH.
My main concern is naming consistency. Share id, peer id, cosigner id, etc. It would be great to be consistent with this id. From this PR, peer id
seems best to me.
cmd/horcrux/cmd/config_test.go
Outdated
@@ -23,9 +23,8 @@ func TestConfigInitCmd(t *testing.T) { | |||
args: []string{ | |||
"tcp://10.168.0.1:1234", | |||
"-c", | |||
"-p", "tcp://10.168.1.2:2222|2,tcp://10.168.1.3:2222|3", | |||
"-p", "tcp://10.168.1.1:2222|1,tcp://10.168.1.2:2222|2,tcp://10.168.1.3:2222|3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we remove this flag completely. The format is unorthodox and tricky to read. Given the necessary complexity of multiple signers, I'm in favor of forcing a config file. Any thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flags are switched around a bit now.
--peers
/-p
replaced by string slice--cosigner
/-c
. The delimiter is removed. The order in which the-c
flags are used determines shard ID 1->n.args[0]
csv for chain nodes replaced by string slice flag--node
/-n
. This makes it easier to string-n
flags together for multiple chain nodeprivValAddr
.
--mode
(string, either default-threshold
orsingle
) replaces-c
.
- share-id: 1 | ||
p2p-addr: tcp://127.0.0.1:2222 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re UX: I'm wondering if there is any benefit to letting the user set the id.
We could use the index of the array as the id. However, if the user re-orders the array, I'm not sure what would happen.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The name share-id
isn't quite resonating with me. cosigner-id
or signer-id
sits better.
In the code (like error messages) it indicates peer id and in the documentation it indicates cosigner id. The code and docs do not mention a "share id".
I'm good with either but not both. We need consistency across the code, config, and documentation. In the config, I'm in favor of simply id
. And since it's under a peers
object, peer.id
makes the most sense to me. The code also supports peer id over cosigner.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
from our discussion off GH, we talked about having these be shards
instead of peers
, with an id
property within each. After playing around with it some more I changed it to cosigners
instead of peers
, and changed share-id
-> shardID
within. I changed the higher level cosigner
property to thresholdMode
, since I think it better represents the configuration that is specific to using threshold
mode. The grouping of shardID
and p2pAddr
adequately represents what cosigners need to know about each other, the network reachable address and the key shard ID for the ephemeral shares (nonce) and combined signature.
signer/config.go
Outdated
@@ -70,23 +71,21 @@ func (c *Config) ValidateCosignerConfig() error { | |||
// the rest of the checks depend on non-nil c.CosignerConfig | |||
return errors.Join(errs...) | |||
} | |||
if c.CosignerConfig.Threshold <= c.CosignerConfig.Shares/2 { | |||
shares := len(c.CosignerConfig.Peers) | |||
if c.CosignerConfig.Threshold <= shares/2 { | |||
errs = append(errs, fmt.Errorf("threshold (%d) must be greater than number of shares (%d) / 2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Instead of a separate errs slice, you could do:
var err error
err = errors.Join(err, fmt.Errorf("..."))
// repeat the above reassignment as much as needed then:
return err
signer/config.go
Outdated
|
||
url, err := url.Parse(peer.P2PAddr) | ||
if err != nil { | ||
errs = append(errs, fmt.Errorf("failed to parse peer %d p2p address: %w", peer.ShareID, err)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re naming consistency, this error and errors below indicate peer id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
peer now replaced by cosigner throughout codebase. (peer ID is replaced by cosigner shard ID)
signer/local_cosigner_test.go
Outdated
CosignerKeyRSA{ | ||
ID: key.ID, | ||
RSAKey: *rsaKey, | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly for a future PR, and a question I've been meaning to ask is why we need the RSA keys.
I found reference to the RSA keys horcrux/docs/signing.md which illuminates some.
But I think we could expand more explaining why we need them, how to create, and how to manage.
|
||
for _, encryptedSecrets := range encryptedEphemeralSharesThresholdMap { | ||
for _, ephemeralSecretPart := range encryptedSecrets { | ||
// if share is intended for peer, check to make sure source peer is included in threshold | ||
if ephemeralSecretPart.DestinationID == peer.GetID() { | ||
for thresholdPeer := range *encryptedEphemeralSharesThresholdMap { | ||
for thresholdPeer := range encryptedEphemeralSharesThresholdMap { | ||
if thresholdPeer.GetID() == ephemeralSecretPart.SourceID { | ||
// source peer is included in threshold signature, include in sharing | ||
peerEphemeralSecretParts = append(peerEphemeralSecretParts, ephemeralSecretPart) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No change because this is existing code that is already working. But there are 3 for loops (2 nested) and 2 nested if statements. I bet there's a refactor that could make this more readable in the future.
* One config file for all signers * Update docs * Fix test * Fix cosigner cmd * Bump test timeout * handle feedback on naming * lint * fix testdata rsa_keys.json * fix cosigners from flag test * consolidate start command. panic if config is v2 format
Merges
cosigner.p2p-listen
into single list of cosigners. The cosigner shard ID (in bothrsa_keys.json
and{chain_id}_share.json
, which must match), will be used to determine which address should be used for the current cosigner.This enables a single config file to be shared across all cosigners, and also means
horcrux config init
can be used once to generate that shared config.Renames some config properties for clarity, updating
horcrux config migrate
to provide smooth migration path from v2 config.Adds
signMode
to config. This will be used forhorcrux start
, which replaceshorcrux signer start
andhorcrux cosigner start
.Consistent naming convention: horcrux
threshold
mode consists ofcosigners
. Cosigners have keyshards
. Those shards haveID
s.Before:
Cosigner 1
Cosigner 2
Cosigner 3
After
All Cosigners
Resolves #125