Skip to content
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

Skipchain leader only #25

Merged
merged 7 commits into from
Mar 25, 2020
Merged

Skipchain leader only #25

merged 7 commits into from
Mar 25, 2020

Conversation

Gilthoniel
Copy link
Contributor

This adds information to the Handler.Process so that network layer info are accessible. It then uses that information to force a skipchain block to be proposed by the leader only.

@Gilthoniel Gilthoniel added enhancement New feature or request mod/mino About the Mino module R4M 🚀 The PR is ready to be reviewed and merged labels Mar 23, 2020
@Gilthoniel Gilthoniel requested a review from nkcr March 23, 2020 16:16
@Gilthoniel Gilthoniel self-assigned this Mar 23, 2020
// and that needs to be processed by the node. It provides some useful
// information about the network layer.
type Request struct {
Address Address
Copy link
Contributor

Choose a reason for hiding this comment

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

I would comment what address this corresponds to

@@ -59,7 +68,7 @@ type RPC interface {
type Handler interface {
// Process handles a single request by producing the response according to
// the request message.
Process(req proto.Message) (resp proto.Message, err error)
Process(req Request) (resp proto.Message, err error)
Copy link
Contributor

Choose a reason for hiding this comment

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

following that, shouldn't we also update Receiver.Recv from Recv(context.Context) (Address, proto.Message, error) to Recv(context.Context) (Request, error) ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, if this is not likely that we add more things to Request, I would find it better to try keeping this interface simple and not use a new struct: Process(req Request, addr Address) (resp proto.Message, err error), as you are already doing with Receiver.Recv

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As discussed, I think it will be useful later.

@Gilthoniel Gilthoniel added R4M 🚀 The PR is ready to be reviewed and merged and removed R4M 🚀 The PR is ready to be reviewed and merged labels Mar 24, 2020
@Gilthoniel Gilthoniel force-pushed the skipchain_leader_only branch from f1414fb to 11b0cc6 Compare March 24, 2020 14:51
@Gilthoniel Gilthoniel added R4M 🚀 The PR is ready to be reviewed and merged and removed R4M 🚀 The PR is ready to be reviewed and merged labels Mar 24, 2020
type Validator interface {
// PayloadProcessor is the interface to implement to validate the generic
// payload stored in the block.
type PayloadProcessor interface {
Copy link
Contributor

Choose a reason for hiding this comment

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

makes me think: we should also add the definition of "payload" to our terminologies.

// propose a block as the leader. It is also responsible for verifying the
// integrity of the players of the chain.
type ViewChange interface {
// Wait returns a non-nil error when the node is allowed to make the
Copy link
Contributor

Choose a reason for hiding this comment

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

  • You return an error when you are allowed to make a proposal?
  • I would use the same word in the comment and the code: in the comment you speak about "proposal" and in the code its "block".
  • It feels strange to suddenly speak about a "node". I would be a bit more precise by saying maybe "...returns a non-nil error when the node running the code is..."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, no it's a nil error when allowed. I changed node by player to be more consistent.

@@ -30,6 +30,25 @@ func ApplyFilters(filters []FilterUpdater) *Filter {
// FilterUpdater is a function to update the filters.
type FilterUpdater func(*Filter)

// RotateFilter is a filter to rotate the indices. When n is above zero, it will
Copy link
Contributor

Choose a reason for hiding this comment

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

so we should update the comment at line 12: "This list is always sorted."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's already the comment ;)

@Gilthoniel Gilthoniel force-pushed the skipchain_leader_only branch from 0981684 to 77ee16f Compare March 25, 2020 07:24
@Gilthoniel Gilthoniel requested a review from nkcr March 25, 2020 07:26
@nkcr nkcr merged commit be542d9 into master Mar 25, 2020
@nkcr nkcr deleted the skipchain_leader_only branch March 25, 2020 07:49
bbjubjub2494 pushed a commit to bbjubjub2494/dela that referenced this pull request Aug 3, 2024
ineiti added a commit that referenced this pull request Oct 10, 2024
Check the source repo of the PR and not the destination repo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request mod/mino About the Mino module R4M 🚀 The PR is ready to be reviewed and merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants