[Core] StateHash Interfaces & Diagrams (#252)

## Description

Interface updates and diagrams to document the cross-module flow for stateHash implementation.

## Issue

Fixes part of #251

## Type of change

Please mark the relevant option(s):

- [x] New feature, functionality or library
- [ ] Bug fix
- [ ] Code health or cleanup
- [x] Major breaking change
- [ ] Documentation
- [ ] Other <!-- add details here if it a different type of change -->

## List of changes

The goal of this change is to show the interface changes and plan of action to implement the state hash implementation.

Documentation and external interface changes required for updating the state hash. Details related to the internals of the persistence module will be done in a follow up commit.

The major change is that the final `quorumCertificate` is only known at the time of committal, not proposal, so the corresponding changes were made as well as interface changes to make the terminology easier to follow with the corresponding documentation.

**General**
- Update some comments / TODOs throughout the code
- Handle errors where appropriate to support interface changes

**UtilityContext interface changes**
- Change `CommitPersistenceContext()` to `Commit(quorumCert)
- Change `ReleaseContext()` to `Release() error`

**PostgresContext interface changes**
- Removed `quorumCert` from the `SetProposalBlock` method signature
- Changed `Commit()` to `Commit(quorumCert)`
- Renamed `ResetContext` to `Release`
- Replace `AppHash` with `UpdateAppHash` 
- Added `ReleaseWriteContext` to the module level interface

**Persistence Module/Interface changes**
- Reduce the cope of the `StoreBlock` function
- Make `storeBlock` accept a `quorumCert`
- Remove `quorumCertificate` from local state and corresponding setters/getters
- Reduce the cope of the `IndexTransactions` function and remove from interface

**Consensus module changes**
- Add error handling where appropriate corresponding to the interface changes

## Testing

- [x] `make develop_test`
- [x] [LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md) w/ all of the steps outlined in the `README`

## Checklist

- [x] I have performed a self-review of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] 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 updated the corresponding CHANGELOG
- [ ] If applicable, I have added tests that prove my fix is effective or that my feature works
- [ ] If applicable, I have added new diagrams using [mermaid.js](https://mermaid-js.github.io)

---

Co-authored-by: Alessandro De Blasis <[email protected]>
Co-authored-by: Irving A.J. Rivas Z. <[email protected]>
Co-authored-by: Andrew Nguyen <[email protected]>
Co-authored-by: Daniel Olshansky <[email protected]>
Co-authored-by: Jason You <[email protected]>

consensus/CHANGELOG.md

@@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+## [0.0.0.8] - 2022-11-15
+
+- Propagate the `quorumCertificate` on `Block` commit to the `Utility` module
+- Slightly improved error handling of the `utilityContext` lifecycle management
+
 ## [0.0.0.7] - 2022-11-01
 
 - Removed `apphash` and `txResults` from `consensusModule` structure

consensus/block.go

@@ -8,14 +8,16 @@ import (
 )
 
 func (m *consensusModule) commitBlock(block *typesCons.Block) error {
-	m.nodeLog(typesCons.CommittingBlock(m.Height, len(block.Transactions)))
-
-	// Commit and release the context
-	if err := m.utilityContext.CommitPersistenceContext(); err != nil {
+	// Commit the context
+	if err := m.utilityContext.Commit(block.BlockHeader.QuorumCertificate); err != nil {
 		return err
 	}
+	m.nodeLog(typesCons.CommittingBlock(m.Height, len(block.Transactions)))
 
-	m.utilityContext.ReleaseContext()
+	// Release the context
+	if err := m.utilityContext.Release(); err != nil {
+		log.Println("[WARN] Error releasing utility context: ", err)
+	}
 	m.utilityContext = nil
 
 	return nil
@@ -55,7 +57,9 @@ func (m *consensusModule) refreshUtilityContext() error {
 	// Ideally, this should not be called.
 	if m.utilityContext != nil {
 		m.nodeLog(typesCons.NilUtilityContextWarning)
-		m.utilityContext.ReleaseContext()
+		if err := m.utilityContext.Release(); err != nil {
+			log.Printf("[WARN] Error releasing utility context: %v\n", err)
+		}
 		m.utilityContext = nil
 	}
 

consensus/consensus_tests/utils_test.go

@@ -362,7 +362,7 @@ func baseUtilityContextMock(t *testing.T) *modulesMock.MockUtilityContext {
 	ctrl := gomock.NewController(t)
 	utilityContextMock := modulesMock.NewMockUtilityContext(ctrl)
 	persistenceContextMock := modulesMock.NewMockPersistenceRWContext(ctrl)
-	persistenceContextMock.EXPECT().SetProposalBlock(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
+	persistenceContextMock.EXPECT().SetProposalBlock(gomock.Any(), gomock.Any(), gomock.Any(), gomock.Any()).Return(nil).AnyTimes()
 	persistenceContextMock.EXPECT().GetPrevAppHash().Return("", nil).AnyTimes()
 
 	utilityContextMock.EXPECT().
@@ -373,13 +373,10 @@ func baseUtilityContextMock(t *testing.T) *modulesMock.MockUtilityContext {
 		ApplyBlock().
 		Return(appHash, nil).
 		AnyTimes()
-	utilityContextMock.EXPECT().CommitPersistenceContext().Return(nil).AnyTimes()
-	utilityContextMock.EXPECT().ReleaseContext().Return().AnyTimes()
+	utilityContextMock.EXPECT().Commit(gomock.Any()).Return(nil).AnyTimes()
+	utilityContextMock.EXPECT().Release().Return(nil).AnyTimes()
 	utilityContextMock.EXPECT().GetPersistenceContext().Return(persistenceContextMock).AnyTimes()
 
-	persistenceContextMock.EXPECT().IndexTransactions().Return(nil).AnyTimes()
-	persistenceContextMock.EXPECT().StoreBlock().Return(nil).AnyTimes()
-
 	return utilityContextMock
 }
 

consensus/debugging.go

@@ -96,6 +96,7 @@ type paceMakerDebug struct {
 	manualMode                bool
 	debugTimeBetweenStepsMsec uint64
 
+	// IMPROVE: Consider renaming to `previousRoundQC`
 	quorumCertificate *typesCons.QuorumCertificate
 }
 

consensus/helpers.go

@@ -82,6 +82,7 @@ func (m *consensusModule) findHighQC(msgs []*typesCons.HotstuffMessage) (qc *typ
 		if m.GetQuorumCertificate() == nil {
 			continue
 		}
+		// TODO: Make sure to validate the "highest QC" first and add tests
 		if qc == nil || m.GetQuorumCertificate().Height > qc.Height {
 			qc = m.GetQuorumCertificate()
 		}

consensus/hotstuff_leader.go

@@ -2,9 +2,10 @@ package consensus
 
 import (
 	"encoding/hex"
-	"github.com/pokt-network/pocket/shared/codec"
 	"unsafe"
 
+	"github.com/pokt-network/pocket/shared/codec"
+
 	consensusTelemetry "github.com/pokt-network/pocket/consensus/telemetry"
 	typesCons "github.com/pokt-network/pocket/consensus/types"
 )
@@ -51,6 +52,7 @@ func (handler *HotstuffLeaderMessageHandler) HandleNewRoundMessage(m *consensusM
 	// TECHDEBT: How do we properly validate `highPrepareQC` here?
 	highPrepareQC := m.findHighQC(m.messagePool[NewRound])
 
+	// TODO: Add test to make sure same block is not applied twice if round is interrupted after being 'Applied'.
 	// TODO: Add more unit tests for these checks...
 	if m.shouldPrepareNewBlock(highPrepareQC) {
 		// Leader prepares a new block if `highPrepareQC` is not applicable
@@ -62,10 +64,8 @@ func (handler *HotstuffLeaderMessageHandler) HandleNewRoundMessage(m *consensusM
 		}
 		m.Block = block
 	} else {
-		// DISCUSS: Do we need to call `validateProposal` here?
 		// Leader acts like a replica if `highPrepareQC` is not `nil`
-		// TODO(olshansky): Add test to make sure same block is not applied twice if round is interrrupted.
-		// been 'Applied'
+		// TODO: Do we need to call `validateProposal` here similar to how replicas does it
 		if err := m.applyBlock(highPrepareQC.Block); err != nil {
 			m.nodeLogError(typesCons.ErrApplyBlock.Error(), err)
 			m.paceMaker.InterruptRound()
@@ -376,7 +376,7 @@ func (m *consensusModule) prepareAndApplyBlock() (*typesCons.Block, error) {
 	}
 
 	// Set the proposal block in the persistence context
-	if err = persistenceContext.SetProposalBlock(blockHeader.Hash, blockProtoBz, blockHeader.ProposerAddress, blockHeader.QuorumCertificate, block.Transactions); err != nil {
+	if err = persistenceContext.SetProposalBlock(blockHeader.Hash, blockProtoBz, blockHeader.ProposerAddress, block.Transactions); err != nil {
 		return nil, err
 	}
 

consensus/hotstuff_replica.go

@@ -3,6 +3,7 @@ package consensus
 import (
 	"encoding/hex"
 	"fmt"
+
 	consensusTelemetry "github.com/pokt-network/pocket/consensus/telemetry"
 	"github.com/pokt-network/pocket/consensus/types"
 	typesCons "github.com/pokt-network/pocket/consensus/types"
@@ -232,7 +233,7 @@ func (m *consensusModule) applyBlock(block *typesCons.Block) error {
 	}
 	persistenceContext := m.utilityContext.GetPersistenceContext()
 	// Set the proposal block in the persistence context
-	if err = persistenceContext.SetProposalBlock(block.BlockHeader.Hash, blockProtoBz, block.BlockHeader.ProposerAddress, block.BlockHeader.QuorumCertificate, block.Transactions); err != nil {
+	if err = persistenceContext.SetProposalBlock(block.BlockHeader.Hash, blockProtoBz, block.BlockHeader.ProposerAddress, block.Transactions); err != nil {
 		return err
 	}
 

consensus/module.go

@@ -50,6 +50,7 @@ type consensusModule struct {
 	//    TODO(#315):  Move the statefulness of `TxResult` to the persistence module
 	TxResults []modules.TxResult // The current block applied transaction results / voted on; it has not been committed to finality
 
+	// IMPROVE: Consider renaming `highPrepareQC` to simply `prepareQC`
 	highPrepareQC *typesCons.QuorumCertificate // Highest QC for which replica voted PRECOMMIT
 	lockedQC      *typesCons.QuorumCertificate // Highest QC for which replica voted COMMIT
 
@@ -71,7 +72,8 @@ type consensusModule struct {
 	// DEPRECATE: Remove later when we build a shared/proper/injected logger
 	logPrefix string
 
-	// TECHDEBT: Move this over to use the txIndexer
+	// TECHDEBT: Rename this to `consensusMessagePool` or something similar
+	//           and reconsider if an in-memory map is the best approach
 	messagePool map[typesCons.HotstuffStep][]*typesCons.HotstuffMessage
 }
 

consensus/pacemaker.go

@@ -227,11 +227,13 @@ func (p *paceMaker) startNextView(qc *typesCons.QuorumCertificate, forceNextView
 	p.consensusMod.clearMessagesPool()
 	// TECHDEBT: This should be avoidable altogether
 	if p.consensusMod.utilityContext != nil {
-		p.consensusMod.utilityContext.ReleaseContext()
+		if err := p.consensusMod.utilityContext.Release(); err != nil {
+			log.Println("[WARN] Failed to release utility context: ", err)
+		}
 		p.consensusMod.utilityContext = nil
 	}
 
-	// TODO(olshansky): This if structure for debug purposes only; think of a way to externalize it...
+	// TECHDEBT: This if structure for debug purposes only; think of a way to externalize it from the main consensus flow...
 	if p.manualMode && !forceNextView {
 		p.quorumCertificate = qc
 		return

persistence/CHANGELOG.md

@@ -9,6 +9,16 @@ TODO: consolidate `persistence/docs/CHANGELOG` and `persistence/CHANGELOG.md`
 
 ## [Unreleased]
 
+## [0.0.0.8] - 2022-11-15
+
+- Rename `GetBlockHash` to `GetBlockHashAtHeight`
+- Reduce visibility scope of `IndexTransactions` to `indexTransactions`
+- Remove `quorumCertificate` from the local context state
+- Remove `LatestQC` and `SetLatestQC`
+- Remove `Latest` prefix from several functions including related to setting context of the proposal block
+- Added `ReleaseWriteContext` placeholder
+- Replaced `ResetContext` with `Release`
+
 ## [0.0.0.7] - 2022-11-01
 
 - Ported over storing blocks and block components to the Persistence module from Consensus and Utility modules

persistence/block.go

@@ -4,9 +4,9 @@ import (
 	"encoding/binary"
 	"encoding/hex"
 	"fmt"
+
 	"github.com/pokt-network/pocket/persistence/kvstore"
 	"github.com/pokt-network/pocket/persistence/types"
-	"github.com/pokt-network/pocket/shared/modules"
 )
 
 // OPTIMIZE(team): get from blockstore or keep in memory
@@ -21,7 +21,7 @@ func (p PostgresContext) GetLatestBlockHeight() (latestHeight uint64, err error)
 }
 
 // OPTIMIZE(team): get from blockstore or keep in cache/memory
-func (p PostgresContext) GetBlockHash(height int64) ([]byte, error) {
+func (p PostgresContext) GetBlockHashAtHeight(height int64) ([]byte, error) {
 	ctx, tx, err := p.getCtxAndTx()
 	if err != nil {
 		return nil, err
@@ -52,12 +52,7 @@ func (p PostgresContext) GetPrevAppHash() (string, error) {
 	if err != nil {
 		return "", fmt.Errorf("error getting block hash for height %d even though it's in the database: %s", height, err)
 	}
-	// IMPROVE/CLEANUP(#284): returning the whole protoblock - should just return hash
-	return hex.EncodeToString(block), nil
-}
-
-func (p PostgresContext) GetTxResults() []modules.TxResult {
-	return p.txResults
+	return hex.EncodeToString(block), nil // TODO(#284): Return `block.Hash` instead of the hex encoded representation of the blockBz
 }
 
 func (p PostgresContext) TransactionExists(transactionHash string) (bool, error) {
@@ -76,9 +71,9 @@ func (p PostgresContext) TransactionExists(transactionHash string) (bool, error)
 	return true, err
 }
 
-func (p PostgresContext) IndexTransactions() error {
+func (p PostgresContext) indexTransactions() error {
 	// TODO: store in batch
-	for _, txResult := range p.GetLatestTxResults() {
+	for _, txResult := range p.GetTxResults() {
 		if err := p.txIndexer.Index(txResult); err != nil {
 			return err
 		}
@@ -89,11 +84,10 @@ func (p PostgresContext) IndexTransactions() error {
 // DISCUSS: this might be retrieved from the block store - temporarily we will access it directly from the module
 //       following the pattern of the Consensus Module prior to pocket/issue-#315
 // TODO(#284): Remove blockProtoBytes from the interface
-func (p *PostgresContext) SetProposalBlock(blockHash string, blockProtoBytes, proposerAddr, qc []byte, transactions [][]byte) error {
+func (p *PostgresContext) SetProposalBlock(blockHash string, blockProtoBytes, proposerAddr []byte, transactions [][]byte) error {
 	p.blockHash = blockHash
 	p.blockProtoBytes = blockProtoBytes
 	p.proposerAddr = proposerAddr
-	p.quorumCertificate = qc
 	p.blockTxs = transactions
 	return nil
 }
@@ -102,7 +96,7 @@ func (p *PostgresContext) SetProposalBlock(blockHash string, blockProtoBytes, pr
 //                 until we include the schema as part of the SQL Store because persistence
 //                 currently has no access to the protobuf schema which is the source of truth.
 // TODO: atomic operations needed here - inherited pattern from consensus module
-func (p PostgresContext) StoreBlock() error {
+func (p PostgresContext) storeBlock(quorumCert []byte) error {
 	if p.blockProtoBytes == nil {
 		// IMPROVE/CLEANUP: HACK - currently tests call Commit() on the same height and it throws a
 		// ERROR: duplicate key value violates unique constraint "block_pkey", because it attempts to
@@ -117,11 +111,11 @@ func (p PostgresContext) StoreBlock() error {
 		return err
 	}
 	// Store in SQL Store
-	if err := p.InsertBlock(uint64(p.Height), p.blockHash, p.proposerAddr, p.quorumCertificate); err != nil {
+	if err := p.InsertBlock(uint64(p.Height), p.blockHash, p.proposerAddr, quorumCert); err != nil {
 		return err
 	}
 	// Store transactions in indexer
-	return p.IndexTransactions()
+	return p.indexTransactions()
 }
 
 func (p PostgresContext) InsertBlock(height uint64, hash string, proposerAddr []byte, quorumCert []byte) error {

persistence/context.go

@@ -5,6 +5,10 @@ import (
 	"log"
 )
 
+func (p PostgresContext) UpdateAppHash() ([]byte, error) {
+	return []byte("TODO(#284): Implement this function."), nil
+}
+
 func (p PostgresContext) NewSavePoint(bytes []byte) error {
 	log.Println("TODO: NewSavePoint not implemented")
 	return nil
@@ -23,21 +27,20 @@ func (p PostgresContext) AppHash() ([]byte, error) {
 func (p *PostgresContext) Reset() error {
 	p.txResults = nil
 	p.blockHash = ""
-	p.quorumCertificate = nil
 	p.proposerAddr = nil
 	p.blockProtoBytes = nil
 	p.blockTxs = nil
 	return nil
 }
 
-func (p PostgresContext) Commit() error {
+func (p PostgresContext) Commit(quorumCert []byte) error {
 	log.Printf("About to commit context at height %d.\n", p.Height)
 
 	ctx := context.TODO()
 	if err := p.GetTx().Commit(context.TODO()); err != nil {
 		return err
 	}
-	if err := p.StoreBlock(); err != nil {
+	if err := p.storeBlock(quorumCert); err != nil {
 		return err
 	}
 	if err := p.conn.Close(ctx); err != nil {

persistence/db.go

@@ -4,9 +4,10 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"github.com/pokt-network/pocket/persistence/indexer"
 	"log"
 
+	"github.com/pokt-network/pocket/persistence/indexer"
+
 	"github.com/pokt-network/pocket/persistence/types"
 
 	"github.com/jackc/pgconn"
@@ -44,21 +45,12 @@ type PostgresContext struct {
 	blockstore kvstore.KVStore
 	txIndexer  indexer.TxIndexer
 	// DISCUSS(#284): this might be retrieved from the block store - temporarily we will access it directly from the module
-	//       following the pattern of the Consensus Module prior to pocket/issue-#315
-	quorumCertificate []byte
-	proposerAddr      []byte
-	blockProtoBytes   []byte
-	blockHash         string
-	blockTxs          [][]byte
-	txResults         []modules.TxResult
-}
-
-func (p PostgresContext) LatestQC() []byte {
-	return p.quorumCertificate
-}
-
-func (p PostgresContext) SetLatestQC(latestQC []byte) {
-	p.quorumCertificate = latestQC
+	//                following the pattern of the Consensus Module prior to pocket/issue-#315
+	proposerAddr    []byte
+	blockProtoBytes []byte
+	blockHash       string
+	blockTxs        [][]byte
+	txResults       []modules.TxResult // Not indexed by `txIndexer` until commit.
 }
 
 func (pg *PostgresContext) getCtxAndTx() (context.Context, pgx.Tx, error) {
@@ -94,27 +86,28 @@ func (pg *PostgresContext) ResetContext() error {
 	return nil
 }
 
-func (p PostgresContext) GetLatestProposerAddr() []byte {
+// DISCUSS: Given that these are context specific setters/getters, is `context.go` a more appropriate location for these than `db.go`?
+func (p PostgresContext) GetProposerAddr() []byte {
 	return p.proposerAddr
 }
 
-func (p PostgresContext) GetLatestBlockProtoBytes() []byte {
+func (p PostgresContext) GetBlockProtoBytes() []byte {
 	return p.blockProtoBytes
 }
 
-func (p PostgresContext) GetLatestBlockHash() string {
+func (p PostgresContext) GetBlockHash() string {
 	return p.blockHash
 }
 
-func (p PostgresContext) GetLatestBlockTxs() [][]byte {
+func (p PostgresContext) GetBlockTxs() [][]byte {
 	return p.blockTxs
 }
 
-func (p PostgresContext) GetLatestTxResults() []modules.TxResult {
+func (p PostgresContext) GetTxResults() []modules.TxResult {
 	return p.txResults
 }
 
-func (p *PostgresContext) SetLatestTxResults(txResults []modules.TxResult) {
+func (p *PostgresContext) SetTxResults(txResults []modules.TxResult) {
 	p.txResults = txResults
 }
 

persistence/debug.go

@@ -45,7 +45,7 @@ func (m *persistenceModule) clearState(_ *messaging.DebugMessage) {
 		log.Printf("Error creating new context: %s \n", err)
 		return
 	}
-	defer context.Commit()
+	defer context.Commit(nil)
 
 	if err := context.(*PostgresContext).DebugClearAll(); err != nil {
 		log.Printf("Error clearing state: %s \n", err)