Add Beacon ID support #832

emmanuelm41 · 2021-10-14T13:50:33Z

No description provided.

Go version update

core/drand_control.go

nikkolasg · 2021-10-14T14:59:13Z

client/client.go

@@ -248,6 +250,14 @@ func WithSchemeID(schID string) Option {
 	}
 }

+// WithBeaconID indicates the id for the randomness generation process which will be queried
+func WithBeaconID(beaconID string) Option {


We need to think of the long term idea here:

What does this mean to give this ID for the client specifically ?

For example, what happens if the ID is not one of the protocols the endpoint is running ?

What happens if we don't provide one ? What is considered "default" ?

I think we need have the feature of fetching the ID remotely from the endpoint. Currently since there is stil only one ID it's simple.

Why is it not in the chain info ? It'd be simpler to unite all informations there, that's what it is made for. As well the SCHEME ID (not sure if it's there or not anymore). In that way, we dont need this additional information since there is already a WithChainInfo method and client can already fetch it automatically, it's already coded.

I think we are still saying that a given beacon is not going to change it's group info / key over it's lifetime. As such, i agree that the root of trust at this point remains the public key / hash of key that is currently the pinned point of reference.

That means that the beacon ID as a piece of metadata used in calculating hashes and identifying the chain acts a lot like the genesis time of the chain. It should be learned from the info endpoint, rather than manually exposed to the client.

Yep - so we should remove this option and just let the client knows it via the chain info, do we agree ?

cmd/client/lib/cli.go

core/drand.go

core/drand_control.go

protobuf/drand/common.proto

chain/beacon/cache.go

chain/beacon/node_test.go

willscott · 2021-10-18T14:52:29Z

chain/beacon/sync.go

 	last, err := s.store.Last()
 	if err != nil {
 		return false
 	}
+
+	beaconID := s.info.ID
+	metadata := common.NewMetadata(utils.Version{Major: 0, Minor: 0, Patch: 0}.ToProto()) // FIXME We should set node version here


set version?

I left the comment to fix this in another PR, as you don't want me to mix topics in PRs. I will fix that one on another PR after merging this one!

Can you open an issue here so we dont forget and track this please ? (maybe you already done it - just making sure !)

Yes, of course we can!

Done here! #833

willscott · 2021-10-18T14:54:22Z

chain/convert.go

 	}, nil
 }

 // ToProto returns the protobuf description of the chain info
 func (c *Info) ToProto() *drand.ChainInfoPacket {
 	buff, _ := c.PublicKey.MarshalBinary()
+
+	metadata := common.Metadata{BeaconID: c.ID} // FIXME Add node version here too


I left the comment to fix this in another PR, as you don't want me to mix topics in PRs. I will fix that one on another PR after merging this one!

willscott · 2021-10-18T14:58:10Z

client/client.go

@@ -248,6 +250,14 @@ func WithSchemeID(schID string) Option {
 	}
 }

+// WithBeaconID indicates the id for the randomness generation process which will be queried
+func WithBeaconID(beaconID string) Option {


I think we are still saying that a given beacon is not going to change it's group info / key over it's lifetime. As such, i agree that the root of trust at this point remains the public key / hash of key that is currently the pinned point of reference.

That means that the beacon ID as a piece of metadata used in calculating hashes and identifying the chain acts a lot like the genesis time of the chain. It should be learned from the info endpoint, rather than manually exposed to the client.

core/drand.go

willscott · 2021-10-18T15:01:15Z

core/drand_control.go

@@ -374,7 +391,7 @@ func (d *Drand) setupAutomaticDKG(_ context.Context, in *drand.InitDKGPacket) (*
 	if err != nil {
 		return nil, err
 	}
-	return finalGroup.ToProto(), nil
+	return finalGroup.ToProto(d.version), nil


this is a different change? is this a bug fix from the versioning before?

Yes, this is not a change related to beacon id feature. If you want, I will leave a FIXME comment and change it on another PR. It is so an small change I thought we can include it here.

core/drand_control.go

willscott · 2021-10-18T15:04:08Z

core/drand_public.go

 	response := chain.NewChainInfo(d.group).ToProto()
-	response.Metadata = metadata
+	response.Metadata.NodeVersion = d.version.ToProto()


this ideally would be separated out into a follow-up on the node versioning PR.
Can you add a test to show that this is now passing version information on chain info requests as expected?

This is not a bug fix from node versioning PR. I change the way we set metadata here so we can handle beacon id too.

willscott · 2021-10-18T15:07:01Z

protobuf/common/common.proto

@@ -15,4 +15,5 @@ message NodeVersion {

 message Metadata {
    NodeVersion node_version = 1;
+    string beaconID = 2;


beacon_id to use consistent casing with node_version above?

If I use beacon_id as name, Protocol will set the name as BeaconId and linter will complain. We had the same issue with schemeID.

Then beacon_ID ?

Nop. If I set so, the filed name would be Beacon_ID. I just tested it.

nikkolasg

A few comments but i think after that we're good to merge !

nikkolasg · 2021-10-19T09:05:23Z

chain/beacon/sync.go

 	last, err := s.store.Last()
 	if err != nil {
 		return false
 	}
+
+	beaconID := s.info.ID
+	metadata := common.NewMetadata(utils.Version{Major: 0, Minor: 0, Patch: 0}.ToProto()) // FIXME We should set node version here


Can you open an issue here so we dont forget and track this please ? (maybe you already done it - just making sure !)

nikkolasg · 2021-10-19T09:07:42Z

client/client.go

@@ -248,6 +250,14 @@ func WithSchemeID(schID string) Option {
 	}
 }

+// WithBeaconID indicates the id for the randomness generation process which will be queried
+func WithBeaconID(beaconID string) Option {


Yep - so we should remove this option and just let the client knows it via the chain info, do we agree ?

core/drand_control.go

nikkolasg · 2021-10-19T09:14:33Z

core/drand_control.go

 	if err != nil {
 		return fmt.Errorf("invalid hash info hex: %v", err)
 	}
 	if !bytes.Equal(info.Hash(), hash) {
 		return errors.New("invalid chain info hash")
 	}
+	if beaconID != info.ID {


This would be nice to test as well - if realistically possible

I am not sure if this would be change in the future, or not. StartFollowChain fetch chain info from other nodes without any specific value. If we have more than one chain info (on multi beacon), we need to be able to choose among them.

I mean you're right to check this here we need to do this even more in the case of multi beacons - so maybe it's enough to test this in your multi beacon PR

key/group.go

willscott and others added 21 commits October 5, 2020 07:51

v1.2.0

42df740

Merge branch 'master' into release/v1.2

44b55ad

Merge branch 'master' into release/v1.2

3bc0526

Merge branch 'master' into release/v1.2

91f2aeb

Merge remote-tracking branch 'origin/master' into release/v1.2

df6cee3

Merge branch 'master' of github.com:drand/drand into release/v1.2

1d6486e

Merge branch 'master' into release/v1.2

277e1e9

Merge branch 'master' into release/v1.2

a5ee729

Merge branch 'drand:master' into master

6fe1929

Merge branch 'drand:master' into master

da7cfe6

Merge branch 'drand:master' into master

ceb1b75

Merge branch 'drand:master' into master

95ee48e

update go version to 1.17

a5980d4

update go and protobuf version

018aba5

Merge pull request #30 from Zondax/go-version-update

e391685

Go version update

Merge branch 'drand:master' into master

51628a2

Merge branch 'drand:master' into master

4e01dbd

Merge branch 'drand:master' into master

f2004d7

Merge branch 'drand:master' into master

9c64cca

Merge branch 'drand:master' into master

3ae2a71

Merge branch 'drand:master' into master

0226449

emmanuelm41 requested a review from nikkolasg October 14, 2021 13:50

nikkolasg reviewed Oct 14, 2021

View reviewed changes

Merge branch 'drand:master' into master

cac168e

emmanuelm41 force-pushed the beacon-id branch from 0dff8c2 to 5aca171 Compare October 15, 2021 18:10

emmanuelm41 marked this pull request as ready for review October 18, 2021 14:33

emmanuelm41 requested review from willscott and nikkolasg October 18, 2021 14:42

willscott reviewed Oct 18, 2021

View reviewed changes

emmanuelm41 requested a review from willscott October 18, 2021 15:34

nikkolasg reviewed Oct 19, 2021

View reviewed changes

emmanuelm41 added 20 commits October 19, 2021 17:02

Merge branch 'drand:master' into master

661f733

start to add beacon id from starting point

b3feb92

add beacon id to client cmd

f38039f

add beacon id on more functions of the code

d1865c9

add beacon id on more functions in the code

d2038f9

fix lint and test issues

218e3ab

add beacon id to beacon packets

c0bf573

fix test

8abb388

add beacon id to Orchestrator tests

3741750

add beacon id on tests scenarios

6083a12

add beacon id on FollowChain cmd on CLI

bb2e30a

add beacon id on chain info to proto conversion

d159c86

apply fixes required by linter

3c20784

apply some style improvements to tests

d53c217

apply some improvements on StartFollowChain func

080090c

add beacon id to info and group hash generation

3dfc0c0

change way beacon id is set on chain info struct

94cb818

fix some lint issues

279ef77

remove beacon id from client

196e192

add test for resharing using beacon id

a8153bc

emmanuelm41 force-pushed the beacon-id branch from b4ed1dc to a8153bc Compare October 19, 2021 20:08

nikkolasg approved these changes Oct 20, 2021

View reviewed changes

add beacon id to some extra line logs

b6faa66

willscott merged commit f2ee959 into drand:master Oct 20, 2021

emmanuelm41 deleted the beacon-id branch October 20, 2021 23:57

emmanuelm41 mentioned this pull request Nov 1, 2021

Multi beacon support on client side Zondax/drand#37

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Beacon ID support #832

Add Beacon ID support #832

emmanuelm41 commented Oct 14, 2021

nikkolasg Oct 14, 2021

willscott Oct 18, 2021

nikkolasg Oct 19, 2021

willscott Oct 18, 2021

emmanuelm41 Oct 18, 2021

nikkolasg Oct 19, 2021

emmanuelm41 Oct 19, 2021

emmanuelm41 Oct 19, 2021

willscott Oct 18, 2021

emmanuelm41 Oct 18, 2021

willscott Oct 18, 2021

willscott Oct 18, 2021

emmanuelm41 Oct 18, 2021

willscott Oct 18, 2021

emmanuelm41 Oct 18, 2021

willscott Oct 18, 2021

emmanuelm41 Oct 18, 2021

nikkolasg Oct 19, 2021

emmanuelm41 Oct 19, 2021

nikkolasg left a comment

nikkolasg Oct 19, 2021

nikkolasg Oct 19, 2021

nikkolasg Oct 19, 2021

emmanuelm41 Oct 19, 2021

nikkolasg Oct 20, 2021

Add Beacon ID support #832

Add Beacon ID support #832

Conversation

emmanuelm41 commented Oct 14, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikkolasg 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