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

Raft Block Signature #395

Merged
merged 14 commits into from
Oct 4, 2018
Merged

Raft Block Signature #395

merged 14 commits into from
Oct 4, 2018

Conversation

dbryan0516
Copy link
Contributor

This Pull Request implements block signing for the raft consensus protocol. Currently, any node could cheat by rewriting a block and re-build the chain from that block onward, in its local copy of the chain, and there's no telling that cheating has occurred and which node has the original copy. By implementing block signing, no follower nodes would be able to rewrite the content because they don't have the leader's nodeKey

This is implemented similarly to clique, where a 32-byte vanity and a seal is added to the extraData header field. During block creation, the raft ID of the leader is combined with the leader's signature of the block in a struct, which is then rlp encoded for storage into the extraData field after the vanity.

For the signing to work, the nodeKey needs to be passed further along to the minter. This is possible by adding a nodeKey field to the raftService and changing the backend type of the minter to the more specific raftService struct, similar to clique and IBFT.

@jpmsam
Copy link
Contributor

jpmsam commented Jun 5, 2018

Thanks @dbryan0516! This was a todo for us, can you please complete the CLA and email it to [email protected].

@dbryan0516
Copy link
Contributor Author

Hey @jpmsam, I just sent it in!

@fixanoid
Copy link
Contributor

fixanoid commented Jun 8, 2018

@jpmsam CLA received

@joelburget
Copy link
Contributor

Hi @dbryan0516, thanks for submitting this! The biggest question I have is why does the signature use the raft ID? I'd expect it to use the enode id.

@jimthematrix
Copy link
Contributor

@joelburget thanks for the review, the reason we chose to encode the Raft ID into the extraData structure instead of the enode ID is that it felt redundant to include the enode ID because you can already recover the proposer's address from the signature itself, which is functionally equivalent to the enode ID. On the other hand, the proposer's Raft ID is otherwise non-existent on the chain itself, so doing this provides a strong binding b/w the node and the Raft cluster. This could be useful in situations like forensic investigation to trace activities from the Raft cluster logs to the blockchain, and vice versa.

@joelburget
Copy link
Contributor

Hi @jimthematrix -- thanks for the explanation. I'll think about it and get back to you in a day or two.

@dbryan0516
Copy link
Contributor Author

Is there something blocking/preventing this from being merged?

@fixanoid
Copy link
Contributor

fixanoid commented Aug 6, 2018

@dbryan0516 hey, were still reviewing this pull and will decide on the best path forward for raft signing after next release which is coming shortly.

Copy link
Contributor

@trung trung left a comment

Choose a reason for hiding this comment

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

Just a nitpick

Also, please do gofmt and optimize the imports

raft/minter.go Outdated Show resolved Hide resolved
@dbryan0516
Copy link
Contributor Author

Made those requested changes for optimizing imports and changing the pointer logic

@jpmsam jpmsam merged commit db8cc81 into Consensys:master Oct 4, 2018
@jpmsam
Copy link
Contributor

jpmsam commented Oct 4, 2018

Thanks @dbryan0516 and @jimthematrix for your work and contribution!

@dbryan0516 dbryan0516 deleted the raft-signature branch October 12, 2018 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants