Skip to content

Libraries for Cryptographic Sortition and VRF#37

Merged
Olshansk merged 87 commits intomilestone/v1-prototypefrom
issue28/prototype/consensus_sortition_lib
Mar 31, 2022
Merged

Libraries for Cryptographic Sortition and VRF#37
Olshansk merged 87 commits intomilestone/v1-prototypefrom
issue28/prototype/consensus_sortition_lib

Conversation

@Olshansk
Copy link
Copy Markdown
Collaborator

@Olshansk Olshansk commented Mar 2, 2022

Description

This PR is meant to partially address the V1 consensus prototype: #28.

It is a set of libraries that are meant to be used for leader election as described in the consensus spec.

Please note that these are just standalone libraries that do not make any changes to the existing prototype and can be reviewed independently from other changes.

Details

Testing

1. Pull changes (if not already local):

$ git clone git@github.com:pokt-network/pocket.git pocket
$ cd pocket
$ gco -b issue28/prototype/consensus_sortition_lib origin/issue28/prototype/consensus_sortition_lib

2. Run unit tests

$ make test_vrf
$ make test_sortition
$ make benchmark_sortition

[Optional] The log statement in TestSortitionBasic can be commented out for some visibility as to what make test_sortition does.

- Support 4 validators + 1 local debugging client
- Debug ports are specified but disabled
@luyzdeleon
Copy link
Copy Markdown
Contributor

luyzdeleon commented Mar 2, 2022

Please note that these are just standalone libraries that do make any changes to the existing prototype.

@Olshansk Did you mean to say "don't make any changes to the existing prototype?"

@Olshansk
Copy link
Copy Markdown
Collaborator Author

Olshansk commented Mar 2, 2022

Yup, thanks for the catch @luyzdeleon! Updated.

Base automatically changed from issue32/prototype/pre2p to milestone/v1-prototype March 4, 2022 02:17
Comment thread .gitignore Outdated
Comment thread Makefile Outdated
Comment thread consensus/leader_election/sortition/sortition.go Outdated
Comment thread consensus/leader_election/sortition/sortition.go Outdated
Comment thread consensus/leader_election/vrf/vrf.go
Comment thread shared/crypto/keys.go Outdated
Comment thread shared/handler.go
)

type EventsChannel chan types.Event
type EventsChannel chan types.PocketEvent
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pocket should not be used as a prefix IMO

Comment thread shared/types/genesis.go
Comment thread shared/types/validator.go
// The key is a hex encoded representation of the validator byte address.
type ValMap map[string]*Validator

type Validator struct {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's going to be a lot of contention around this structure as at this time there's not good justification to have a shared Validator type

It needs to be made more explicit that this is a temp

Copy link
Copy Markdown
Contributor

@luyzdeleon luyzdeleon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this PR is in a good direction, if I might summarize my pending questions are:

  1. Staked amounts being handled as float types when they should be ints (uPOKT vs POKT denominations).

  2. Why is there PreP2P module code included in this PR?

  3. Some shared data structures like the Validator would be more appropiate inside of the consensus_types

Comment thread consensus/leader_election/sortition/sortition.go Outdated
Comment thread consensus/leader_election/sortition/sortition.go
Comment thread consensus/leader_election/sortition/sortition.go
Comment thread consensus/leader_election/sortition/sortition_test.go Outdated
Comment thread consensus/leader_election/sortition/sortition_test.go Outdated
Comment thread consensus/leader_election/sortition/sortition_test.go Outdated
Comment thread consensus/leader_election/sortition/sortition_test.go Outdated
Comment thread consensus/leader_election/vrf/vrf.go
Comment thread p2p/pre2p/handlers.go
}

networkMessage := types.PocketEvent{}
if err := proto.Unmarshal(data, &networkMessage); err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with having a single "Encoder/Decoder" interface and let that use anything it needs underneath.

Comment thread shared/types/genesis.go
Copy link
Copy Markdown
Collaborator Author

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@luyzdeleon @andrewnguyen22 Please take another look when you have a chance. Some things to keep in mind:

  1. Since our git histories are a little messy right now, the original PR extended more than just the base crypto libraries here, but that is mostly resolved and the PR is much smaller.

  2. I refactored the tests and added a lot more documentation in sortition.go and sortition_test.go, so that can be the focus of the re-review.

  3. I added details as to why I chose this VRF library over others, but ultimately I don't have super strong reasoning one way or another.

@Olshansk Olshansk linked an issue Mar 16, 2022 that may be closed by this pull request
9 tasks
Comment thread docs/deps/README.md
Comment thread shared/crypto/ed25519.go Outdated
Comment thread shared/crypto/ed25519.go Outdated
Copy link
Copy Markdown
Collaborator Author

@Olshansk Olshansk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andrewnguyen22 Tended to the last set of comments.

PTAL at the README again as I added some interesting points there.

Comment thread docs/deps/README.md
Comment thread shared/crypto/ed25519.go Outdated
Comment thread shared/crypto/ed25519.go Outdated
@Olshansk Olshansk requested a review from andrewnguyen22 March 18, 2022 23:13
@Olshansk
Copy link
Copy Markdown
Collaborator Author

@luyzdeleon Please take a look again when you have a chance. The two places I'd focus on per your comments are:

  1. The updated README
  2. The changes in the test files

Copy link
Copy Markdown
Contributor

@luyzdeleon luyzdeleon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Habemus approval

@Olshansk Olshansk merged commit 7994b37 into milestone/v1-prototype Mar 31, 2022
@Olshansk Olshansk deleted the issue28/prototype/consensus_sortition_lib branch March 31, 2022 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

consensus Consensus specific changes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

V1 Consensus Module - First Iteration

3 participants