[TECHDEBT][Consensus] Consensus module techdebt consolidation

Makefile

@@ -347,9 +347,11 @@ benchmark_p2p_addrbook:
 # HACK          - Like TECHDEBT, but much worse. This needs to be prioritized
 # REFACTOR      - Similar to TECHDEBT, but will require a substantial rewrite and change across the codebase
 # CONSIDERATION - A comment that involves extra work but was thoughts / considered as part of some implementation
+# CONSOLIDATE   - We likely have similar implementations/types of the same thing, and we should consolidate them.
+# DEPRECATE     - Code that should be removed in the future
 # DISCUSS_IN_THIS_COMMIT - SHOULD NEVER BE COMMITTED TO MASTER. It is a way for the reviewer of a PR to start / reply to a discussion.
 # TODO_IN_THIS_COMMIT    - SHOULD NEVER BE COMMITTED TO MASTER. It is a way to start the review process while non-critical changes are still in progress
-TODO_KEYWORDS = -e "TODO" -e "TECHDEBT" -e "IMPROVE" -e "DISCUSS" -e "INCOMPLETE" -e "INVESTIGATE" -e "CLEANUP" -e "HACK" -e "REFACTOR" -e "CONSIDERATION" -e "TODO_IN_THIS_COMMIT" -e "DISCUSS_IN_THIS_COMMIT"
+TODO_KEYWORDS = -e "TODO" -e "TECHDEBT" -e "IMPROVE" -e "DISCUSS" -e "INCOMPLETE" -e "INVESTIGATE" -e "CLEANUP" -e "HACK" -e "REFACTOR" -e "CONSIDERATION" -e "TODO_IN_THIS_COMMIT" -e "DISCUSS_IN_THIS_COMMIT" -e "CONSOLIDATE" -e "DEPRECATE"
 
 # How do I use TODOs?
 # 1. <KEYWORD>: <Description of follow up work>;

consensus/CHANGELOG.md

@@ -7,21 +7,51 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
-## [0.0.0.3] - 2022-09-28
+## [0.0.0.4] - 2022-09-28
+
 - `consensusModule` stores block directly to prevent shared structure in the `utilityModule`
 
+## [0.0.0.3] - 2022-09-26
+
+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 `TODO`s 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`
+
 ## [0.0.0.2] - 2022-08-25
+
 **Encapsulate structures previously in shared [#163](github.com/pokt-network/pocket/issues/163)**
+
 - Ensured proto structures implement shared interfaces
 - `ConsensusConfig` uses shared interfaces in order to accept `MockConsensusConfig` in test_artifacts
 - `ConsensusGenesisState` uses shared interfaces in order to accept `MockConsensusGenesisState` in test_artifacts
 - Implemented shared validator interface for `validator_map` functionality
 
 ## [0.0.0.1] - 2021-03-31
 
-HotPocket 1st Iteration (https://github.com/pokt-network/pocket/pull/48)
-
-# Added
+### Added new libraries: HotPocket 1st iteration
 
 - Initial implementation of Basic Hotstuff
 - Initial implementation Hotstuff Pacemaker
@@ -32,9 +62,7 @@ HotPocket 1st Iteration (https://github.com/pokt-network/pocket/pull/48)
 
 ## [0.0.0.0] - 2021-03-31
 
-VRF & Cryptographic Sortition Libraries (https://github.com/pokt-network/pocket/pull/37/files)
-
-### Added
+### Added new libraries: VRF & Cryptographic Sortition Libraries
 
 - Tests with `make test_vrf` and `make test_sortition`
 - Benchmarking via `make benchmark_sortition`

consensus/block.go

@@ -1,107 +1,13 @@
 package consensus
 
 import (
-	"encoding/hex"
+	"log"
 	"unsafe"
 
-	"github.com/pokt-network/pocket/shared/codec"
-
 	typesCons "github.com/pokt-network/pocket/consensus/types"
+	"github.com/pokt-network/pocket/shared/codec"
 )
 
-// TODO(olshansky): Sync with Andrew on the type of validation we need here.
-func (m *ConsensusModule) validateBlock(block *typesCons.Block) error {
-	if block == nil {
-		return typesCons.ErrNilBlock
-	}
-	return nil
-}
-
-// This is a helper function intended to be called by a leader/validator during a view change
-func (m *ConsensusModule) prepareBlockAsLeader() (*typesCons.Block, error) {
-	if m.isReplica() {
-		return nil, typesCons.ErrReplicaPrepareBlock
-	}
-
-	if err := m.refreshUtilityContext(); err != nil {
-		return nil, err
-	}
-
-	txs, err := m.utilityContext.GetProposalTransactions(m.privateKey.Address(), maxTxBytes, lastByzValidators)
-	if err != nil {
-		return nil, err
-	}
-
-	appHash, err := m.utilityContext.ApplyBlock(int64(m.Height), m.privateKey.Address(), txs, lastByzValidators)
-	if err != nil {
-		return nil, err
-	}
-
-	blockHeader := &typesCons.BlockHeader{
-		Height:            int64(m.Height),
-		Hash:              hex.EncodeToString(appHash),
-		NumTxs:            uint32(len(txs)),
-		LastBlockHash:     m.appHash,
-		ProposerAddress:   m.privateKey.Address().Bytes(),
-		QuorumCertificate: []byte("HACK: Temporary placeholder"),
-	}
-
-	block := &typesCons.Block{
-		BlockHeader:  blockHeader,
-		Transactions: txs,
-	}
-
-	return block, nil
-}
-
-// This is a helper function intended to be called by a replica/voter during a view change
-func (m *ConsensusModule) applyBlockAsReplica(block *typesCons.Block) error {
-	if m.isLeader() {
-		return typesCons.ErrLeaderApplyBLock
-	}
-
-	// TODO(olshansky): Add unit tests to verify this.
-	if unsafe.Sizeof(*block) > uintptr(m.MaxBlockBytes) {
-		return typesCons.ErrInvalidBlockSize(uint64(unsafe.Sizeof(*block)), m.MaxBlockBytes)
-	}
-
-	if err := m.refreshUtilityContext(); err != nil {
-		return err
-	}
-
-	appHash, err := m.utilityContext.ApplyBlock(int64(m.Height), block.BlockHeader.ProposerAddress, block.Transactions, lastByzValidators)
-	if err != nil {
-		return err
-	}
-
-	// DISCUSS(drewsky): Is `ApplyBlock` going to return blockHash or appHash?
-	if block.BlockHeader.Hash != hex.EncodeToString(appHash) {
-		return typesCons.ErrInvalidAppHash(block.BlockHeader.Hash, hex.EncodeToString(appHash))
-	}
-
-	return nil
-}
-
-// Creates a new Utility context and clears/nullifies any previous contexts if they exist
-func (m *ConsensusModule) refreshUtilityContext() error {
-	// This is a catch-all to release the previous utility context if it wasn't cleaned up
-	// in the proper lifecycle (e.g. catch up, error, network partition, etc...). Ideally, this
-	// should not be called.
-	if m.utilityContext != nil {
-		m.nodeLog(typesCons.NilUtilityContextWarning)
-		m.utilityContext.ReleaseContext()
-		m.utilityContext = nil
-	}
-
-	utilityContext, err := m.GetBus().GetUtilityModule().NewContext(int64(m.Height))
-	if err != nil {
-		return err
-	}
-
-	m.utilityContext = utilityContext
-	return nil
-}
-
 func (m *ConsensusModule) commitBlock(block *typesCons.Block) error {
 	m.nodeLog(typesCons.CommittingBlock(m.Height, len(block.Transactions)))
 
@@ -130,7 +36,7 @@ func (m *ConsensusModule) commitBlock(block *typesCons.Block) error {
 	m.utilityContext.ReleaseContext()
 	m.utilityContext = nil
 
-	m.appHash = block.BlockHeader.Hash
+	m.lastAppHash = block.BlockHeader.Hash
 
 	return nil
 }
@@ -149,3 +55,49 @@ func (m *ConsensusModule) storeBlock(block *typesCons.Block, blockProtoBytes []b
 	}
 	return nil
 }
+
+// TODO: Add unit tests specific to block validation
+func (m *ConsensusModule) validateBlockBasic(block *typesCons.Block) error {
+	if block == nil && m.Step != NewRound {
+		return typesCons.ErrNilBlock
+	}
+
+	if block != nil && m.Step == NewRound {
+		return typesCons.ErrBlockExists
+	}
+
+	if block != nil && unsafe.Sizeof(*block) > uintptr(m.consGenesis.MaxBlockBytes) {
+		return typesCons.ErrInvalidBlockSize(uint64(unsafe.Sizeof(*block)), m.consGenesis.MaxBlockBytes)
+	}
+
+	// If the current block being processed (i.e. voted on) by consensus is non nil, we need to make
+	// sure that the data (height, round, step, txs, etc) is the same before we start validating the signatures
+	if m.Block != nil {
+		// DISCUSS: The only difference between blocks from one step to another is the QC, so we need
+		//          to determine where/how to validate this
+		if protoHash(m.Block) != protoHash(block) {
+			log.Println("[TECHDEBT][ERROR] The block being processed is not the same as that received by the consensus module ")
+		}
+	}
+
+	return nil
+}
+
+// Creates a new Utility context and clears/nullifies any previous contexts if they exist
+func (m *ConsensusModule) refreshUtilityContext() error {
+	// Catch-all structure to release the previous utility context if it wasn't properly cleaned up.
+	// Ideally, this should not be called.
+	if m.utilityContext != nil {
+		m.nodeLog(typesCons.NilUtilityContextWarning)
+		m.utilityContext.ReleaseContext()
+		m.utilityContext = nil
+	}
+
+	utilityContext, err := m.GetBus().GetUtilityModule().NewContext(int64(m.Height))
+	if err != nil {
+		return err
+	}
+
+	m.utilityContext = utilityContext
+	return nil
+}

consensus/consensus_tests/pacemaker_test.go

@@ -157,22 +157,29 @@ func TestPacemakerCatchupSameStepDifferentRounds(t *testing.T) {
 		Transactions: emptyTxs,
 	}
 
-	leaderConsensusMod := GetConsensusModImplementation(leader)
+	leaderConsensusMod := GetConsensusModElem(leader)
 	leaderConsensusMod.FieldByName("Block").Set(reflect.ValueOf(block))
 
 	// Set all nodes to the same STEP and HEIGHT BUT different ROUNDS
 	for _, pocketNode := range pocketNodes {
-		consensusModImpl := GetConsensusModImplementation(pocketNode)
-		consensusModImpl.FieldByName("Height").SetUint(testHeight)
-		consensusModImpl.FieldByName("Step").SetInt(testStep)
-		consensusModImpl.FieldByName("LeaderId").Set(reflect.Zero(reflect.TypeOf(&leaderId))) // This is re-elected during paceMaker catchup
+		// utilityContext is only set on new rounds, which is skipped in this test
+		utilityContext, err := pocketNode.GetBus().GetUtilityModule().NewContext(int64(testHeight))
+		require.NoError(t, err)
+
+		consensusModElem := GetConsensusModElem(pocketNode)
+		consensusModElem.FieldByName("Height").SetUint(testHeight)
+		consensusModElem.FieldByName("Step").SetInt(testStep)
+		consensusModElem.FieldByName("LeaderId").Set(reflect.Zero(reflect.TypeOf(&leaderId))) // This is re-elected during paceMaker catchup
+
+		consensusModImpl := GetConsensusModImpl(pocketNode)
+		consensusModImpl.MethodByName("SetUtilityContext").Call([]reflect.Value{reflect.ValueOf(utilityContext)})
 	}
 
 	// Set the leader to be in the highest round.
-	GetConsensusModImplementation(pocketNodes[1]).FieldByName("Round").SetUint(uint64(leaderRound - 2))
-	GetConsensusModImplementation(pocketNodes[2]).FieldByName("Round").SetUint(uint64(leaderRound - 3))
-	GetConsensusModImplementation(pocketNodes[leaderId]).FieldByName("Round").SetUint(uint64(leaderRound))
-	GetConsensusModImplementation(pocketNodes[4]).FieldByName("Round").SetUint(uint64(leaderRound - 4))
+	GetConsensusModElem(pocketNodes[1]).FieldByName("Round").SetUint(uint64(leaderRound - 2))
+	GetConsensusModElem(pocketNodes[2]).FieldByName("Round").SetUint(uint64(leaderRound - 3))
+	GetConsensusModElem(pocketNodes[leaderId]).FieldByName("Round").SetUint(uint64(leaderRound))
+	GetConsensusModElem(pocketNodes[4]).FieldByName("Round").SetUint(uint64(leaderRound - 4))
 
 	prepareProposal := &typesCons.HotstuffMessage{
 		Type:          consensus.Propose,