Refine byzcoin - Githubissues

dedis / cothority

Scalable collective authority

Other

425 stars 106 forks source link

Refine byzcoin #2470

Closed ineiti closed 2 years ago

ineiti commented 2 years ago

This is a collection of patches for byzcoin and skipchain:

solve a race condition where the bftForwardLinkLevel0Ack has been called before bftForwardLinkLevel0 is done
allow storing of new SkipBlocks in existing chains, even if a Client has been registered
changing the byzcoin.Client.GetInstance method to also do the protobuf-decoding
adding a byzcoin.Client.UpdateNodes in case some of the nodes didn't catch up

This has been tested in production and behaves nicer. Of course there are still some bugs left...

sonarcloud[bot] commented 2 years ago

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

No Coverage information
0.0% Duplication

calctopian commented 2 years ago

About changing "Roster.List" to "GetNodes()":

"Roster.List" is used over all the repository, although it doesn't always refer to ByzCoin.Client
There are instances where it does refer to ByzCoin.Client, for example in "eventlog/api.go": 209: if err := c.c.SendProtobuf(c.ByzCoin.Roster.List[0], req, reply); err != nil { 267: blocks, err := c.sc.GetUpdateChainLevel(&c.ByzCoin.Roster, id, 0, -1) These must also be changed to GetNodes()
I haven't reviewed all the codebase, this is just a cursory review: I think there are more places where this must be changed.

ineiti commented 2 years ago

@calctopian - thanks for your comment. The change I did here is really just relevant to the byzcoin.Client. The other Roster.List are very often needed as-is and should not be replaced by the nodes here. The one in line 209 refers to the leader, and should stay like this. Perhaps the one in line 267 should be changed. Are you using the eventlog service? In my mind it is marked as deprecated.

The goal is for a client to know which nodes they can connect to without problems. This is needed because some of the nodes might be faulty. Of course this doesn't help against byzantine nodes. But currently we're more concerned by too slow nodes than anything else :)

The best place for this to be would be probably in the cothority/skipchain/api.go, and then re-use it by the cothority/byzcoin/api.go.

calctopian commented 2 years ago

No, I wasn't using the eventlog service, just finding instances where byzcoin.Client was being used.

OK, I fully agree with you that the priority is to connect to the fastest nodes :)