Multi beacon support on client side #37
Conversation
| } else { | ||
| // trigger the startAutowatch to retry on backoff | ||
| close(wc) | ||
| } | ||
| return wc | ||
| } | ||
|
|
||
| func (c *watchAggregator) Watch(ctx context.Context) <-chan Result { | ||
| func (c *watchAggregator) Watch(ctx context.Context, chainHash []byte) <-chan Result { |
There was a problem hiding this comment.
why not passing this inside the config ? I see later down in the PR that all functions have this added parameters. Hence a very very long PR as well.
It feels to me it would just be simpler to let the client create multiple clients, one for each chain,so the API is the same, simply the initialization is not. As well, for some chain, some config item may make sense but not for others so it would be better to isolate and separate as much as possible here.
| curr, err := g.client.PublicRand(ctx, &drand.PublicRandRequest{Round: round}) | ||
| func (g *grpcClient) Get(ctx context.Context, chainHash []byte, round uint64) (client.Result, error) { | ||
| chainHashToUse := g.chainHash | ||
| if chainHash != nil { |
There was a problem hiding this comment.
If you require to pass a chain hash at the beginning, then why requiring it later in the params ? Both don't make sense to me, we shouldn't have to "overload" the chain hash. and the more I read this PR the more I lean towards putting it inside the config so then the PR shrinks in size by 50% at least and the API is simpler.
|
|
||
| hash, err = hex.DecodeString(c.String(HashFlag.Name)) | ||
| if err != nil { | ||
| return nil, err |
There was a problem hiding this comment.
Just a fmt.Errorf indicating that the hash flag is invalid would be nice - UX is annoying but so important !
| @@ -122,3 +124,20 @@ func (dd *DrandDaemon) AddNewBeaconProcess(beaconID string, store key.Store) (*B | |||
|
|
|||
| return bp, nil | |||
| } | |||
|
|
|||
| func (dd *DrandDaemon) AddNewChainHash(chainHash []byte, beaconID string) { | |||
There was a problem hiding this comment.
I'm quite confused. Could you explain what is your thought on how to do this ?
I thought we were gonna use the beaconID now as the only "ID" to refer to a beacon process/protocol. But now, we are using both chain hash AND beacon ID. If this is right, then I'm afraid this is just gonna lead to more problems down the line. Is there a way to simplify and try to get rid of the chain hash ? Otherwise I'm not sure to see the point of having an explicit beaconID then... I thought both were intended to be used for the same thing, so we should thrive to keep one eventually.
There was a problem hiding this comment.
from the client perspective, chain hash acts as a root of trust. A 'beaconID' does not - there's no cryptographic link that lets a client that's distributed to follow a 'beaconID' know that it's actually following the right key.
From a client's perspective, the root of trust will need (as far as anything I've heard proposed) to continue being the chain hash.
I don't think I care about how it's indexed within the drand node itself
There was a problem hiding this comment.
You're right: the chain hash hashes/includes the public key, so it is what the client needs to store/trust offline, as it is currently the case.
However, now we have two ways to call the methods, two informations we need to put in the metadata depending on which functions we call. The PR does the following:
- For client related public functions (i.e. getting the randomness for a round), metadata is expecting chain hash
- For intra-drand methods (i.e. broadcasting dkg messages, partial signatures etc), metadata is expecting beacon ID
"1" was not needed before because there was only one chain. Now, we need a way to differentiate those, but why not using beacon ID here ? It doesn't change anything since the verification happens on the client side and he is supposed to already have the chain hash (which should include the beacon ID - does it?). Or, even better, on the opposite, why not using chain hash everywhere ? BeaconID is just nice for UX, like we can name the chain/network/protocol we're using, but internally,i.e. on the network, the strongest notion to use is really the chain hash because it's guaranteed to be unique.
Is there any specific reasons for the current design ?
My personal opinion remains the same on this that we should thrive to unify and simplify this: currently it is not documented and if a person were to write a new drand implementation, that would simply be quite hard to know what to use when. We have specs that we should update as well so the simpler / the more unified the protocol, the better. It feels like using chain hash for everything is strictly better but I'm happy to continue discussing (and to change opinion ;) )
| @@ -101,7 +122,14 @@ func (dd *DrandDaemon) PushDKGInfo(ctx context.Context, in *drand.DKGInfoPacket) | |||
| // SyncChain is a inter-node protocol that replies to a syncing request from a | |||
| // given round | |||
| func (dd *DrandDaemon) SyncChain(in *drand.SyncRequest, stream drand.Protocol_SyncChainServer) error { | |||
| bp, _, err := dd.getBeaconProcess(in.GetMetadata()) | |||
| metadata := in.GetMetadata() | |||
| beaconID, err := dd.translateChainHashToBeaconId(metadata) | |||
There was a problem hiding this comment.
could you write bit in the PR desc or a doc what is your vision for the diff and usage of chainhash and beaconID ? I'm a bit lost tbh here... Again, I though we were gonna get rid of chain hash and only use beaconID (not for the current protocol of course) but maybe I misunderstood but then I dont see any difference in usage between the two , so Im questionning is it worth having two identifier ? It feels like a lot more complicated to me for not so clear gains...
emmanuelm41
force-pushed
the
multi-beacon
branch
from
9ffd546
to
4e7427f
Compare
emmanuelm41
force-pushed
the
multi-beacon-client
branch
from
db5529d
to
05a43eb
Compare
|
Close PR as it is not required anymore. |
No description provided.