Skip to content

[TECHDEBT][Consensus] Consensus module techdebt consolidation#250

Merged
Olshansk merged 66 commits intomainfrom
issues/249/consensus_techdebt
Oct 5, 2022
Merged

[TECHDEBT][Consensus] Consensus module techdebt consolidation#250
Olshansk merged 66 commits intomainfrom
issues/249/consensus_techdebt

Conversation

@Olshansk
Copy link
Collaborator

@Olshansk Olshansk commented Sep 26, 2022

Description

Consolidate & update techdebt in the consensus module (excluding the pacemaker module which is being modified by #198) in preparation for an end-to-end state hash flow explored in #237.

Issue

Fixes #249

Type of change

  • New feature, functionality or library
  • Bug fix (non-breaking change which fixes an issue)
  • Code health or cleanup
  • Major breaking change
  • Documentation
  • Other: NA

List of changes

Consensus logic

  • Pass in a list of messages to findHighQC instead of a hotstuff step
  • Made CreateProposeMessage and CreateVotemessage accept explicit values, identifying some bugs along the way
  • Made sure to call applyBlock when using highQC from previous round
  • Moved business logic for prepareAndApplyBlock into hotstuff_leader.go
  • Removed MaxBlockBytes and storing the consensus genesis type locally as is

Consensus cleanup

  • Using appropriate getters for protocol types in the hotstuff lifecycle
  • Replaced proto.Marshal with codec.GetCodec().Marshal
  • Reorganized and cleaned up the code in consensus/block.go
  • Consolidated & removed a few TODOs throughout the consensus module
  • Added TECHDEBT and TODOs that will be require for a real block lifecycle
  • Fixed typo in hotstuff_types.proto
  • Moved the hotstuff handler interface to consensus/hotstuff_handler.go

Consensus testing

  • Improved mock module initialization in consensus/consensus_tests/utils_test.go

General

  • Added a diagram for AppHash related ContextInitialization
  • Added Makefile keywords for TODO

Testing

  • make test_all
  • LocalNet w/ all of the steps outlined in the README

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have tested my changes using the available tooling
  • If applicable, I have made corresponding changes to related local or global README
  • If applicable, I have added new diagrams using mermaid.js
  • If applicable, I have added tests that prove my fix is effective or that my feature works

@Olshansk
Copy link
Collaborator Author

@deblasis I still didn't tend to all of your feedback, but please take another look didn't tend to all of your comments and the changes that I did make and the responses I left.

@deblasis
Copy link
Contributor

deblasis commented Oct 3, 2022

@deblasis I still didn't tend to all of your feedback, but please take another look didn't tend to all of your comments and the changes that I did make and the responses I left.

Nice one, most things are solved, I think there are only 2 AI:

@Olshansk Olshansk requested a review from deblasis October 4, 2022 23:58
Copy link
Contributor

@deblasis deblasis left a comment

Choose a reason for hiding this comment

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

LGTM

@Olshansk Olshansk merged commit cb8135a into main Oct 5, 2022
@Olshansk Olshansk deleted the issues/249/consensus_techdebt branch October 5, 2022 16:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

code health Nice to have code improvement consensus Consensus specific changes

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

[Consensus][TECHDEBT] Consensus module techdebt consolidation

2 participants