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

Consensus module #5

Merged
merged 13 commits into from
Mar 11, 2020
Merged

Consensus module #5

merged 13 commits into from
Mar 11, 2020

Conversation

Gilthoniel
Copy link
Contributor

@Gilthoniel Gilthoniel commented Mar 8, 2020

This extracts the logic of the consensus from the skipchain module and implements the collective signing PBFT from the byzcoin paper.

Note: skipchain to improve later.

@Gilthoniel Gilthoniel added the WIP Work in progress label Mar 8, 2020
@Gilthoniel Gilthoniel self-assigned this Mar 8, 2020
uint32 height = 2;
uint32 baseHeight = 3;
uint32 maximumHeight = 4;
bytes genesisID = 5;
Copy link
Contributor

Choose a reason for hiding this comment

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

what does represent the genesisID?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's the hash of the genesis block which is used as the chain identifier.

blockchain/skipchain/messages.proto Show resolved Hide resolved
DecodeForwardLink(pb proto.Message) (forwardLink, error)
}

// defaultChainFactory is an implementation of the defaultChainFactory interface for forward links.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// defaultChainFactory is an implementation of the defaultChainFactory interface for forward links.
// defaultChainFactory is an implementation of the defaultChainFactory interface
// for forward links.

cosi/mod.go Outdated
Comment on lines 15 to 16
// Hashable is the interface to implement to validate an incoming message
// for a collective signing. It will return the hash that will be signed.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Hashable is the interface to implement to validate an incoming message
// for a collective signing. It will return the hash that will be signed.
// Hashable is the interface to implement to validate an incoming message for a
// collective signing. It will return the hash that will be signed.

// AddressFactory is an implementation of the factory interface.
type AddressFactory struct{}

// FromText returns an instance of an address from a string.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// FromText returns an instance of an address from a string.
// FromText returns an instance of an address from a byte slice.

rpcName = "cosipbft"
)

// Consensus is the implementation of the interface.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Consensus is the implementation of the interface.
// Consensus is the implementation of the consensus.Consensus interface.

return nil, encoding.NewAnyDecodingError(&da, err)
}

// The proposal first need to be validated by the caller of the module
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// The proposal first need to be validated by the caller of the module
// The proposal first needs to be validated by the caller of the module


// LockProposal verifies the prepare signature and stores it. It also locks
// the queue to prevent further committing.
func (q *queue) LockProposal(to Digest, sig crypto.Signature) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

what if q.locked is already true ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, you're right !

Comment on lines 59 to 60
// ReadChain returns the list of forward links to the id. It returns an error
// if the target is never reached.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// ReadChain returns the list of forward links to the id. It returns an error
// if the target is never reached.
// ReadChain returns the list of forward links to the id (which is not
// included). It returns an error if the target is never reached.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm it is actually included in the last forward link which has to point on id.

// Validate should return the proposal decoded from the message or
// an error if it is invalid. It should also return the previous
// proposal.
Validate(message proto.Message) (curr Proposal, prev Proposal, 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.

that feels very tight to the way we make verification now, but we could imagine scenarios where a validation needs something else. What if we make the consensus use on the blockchain abstraction?
That would give something like:

Validate(message proto.Message) (bc Blockchain, blockIndex int, err error)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My point was to build blockchain on top of the consensus but this one should not be aware of what is a blockchain. You could totally build something on top of the consensus that is unrelated to blockchain. The benefit is that the module is responsible to create the crypto links between proposals but a proposal can be anything.

@Gilthoniel
Copy link
Contributor Author

@nkcr thanks for the comments! I answered some of them, if you can have a second look.

@nkcr nkcr merged commit 9e518e2 into master Mar 11, 2020
@nkcr nkcr deleted the consensus_module branch March 11, 2020 07:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants