[Persistence] TxIndexer to StateHash Bridge (Issue-#315)

[andrewnguyen22]

Description

TL;DR Encapsulate and commit the block and block parts within a persistence context

The objective of this issue is to "bridge" the work being done in #302 (tx indexer integration) and #285 (first state hash implementation.

The interface and diagrams for the state hash computation were being done in #252 while the first implementation began in #285. However, since both of these are dependent on the integration of the tx indexer in #302, some conflicting design decisions arose.

The goal of this ticket is to unblock #302, while also capturing the results of an offline discussion that led to a decision where the "ephemeral block state" should be wholly managed by the persistence context, being a single, "roll-backable" point of reference

• Enable committing of a block to persistence by simply committing the persistence context
• Remove ephemeral block-related state from the consensus module & utility module, and only maintain it in the persistence module
• Simply outward-facing module interfaces to only rely on primitive types
• Remove validatorMap from the consensusModule struct state & access it from the persistence layer (added n the past)

Opened new issue for last one to prevent scope from getting out of hand: #331

Issue

Fixes #315

Type of change

Please mark the relevant option(s):

• New feature, functionality or library
• Bug fix
• Code health or cleanup
• Major breaking change
• Documentation
• Other

List of changes

• Removed apphash and txResults from consensus Module structure
• Modified lifecycle to set the proposal block within a Persistence Context
• Allow block and parts to be committed with the persistence context
• Ported over storing blocks and block parts to persistence module from Consensus and Utility
• Encapsulate TxIndexer logic only inside the persistence context
• Remove TxResult from the utility module interface (added in [TxIndexer] Integration of transaction indexer (issue-[TxIndexer] Integration of transaction indexer #168) [TxIndexer] Integration of transaction indexer (issue-#168) #302)
• Combined creation and application of block in proposer lifecycle

Testing

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

Required 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
• I have updated the corresponding CHANGELOG

If Applicable Checklist

• I have updated the corresponding README(s); local and/or global
• I have added tests that prove my fix is effective or that my feature works
• I have added, or updated, mermaid.js diagrams in the corresponding README(s)
• I have added, or updated, documentation and mermaid.js diagrams in shared/docs/* if I updated shared/*README(s)

[Olshansk]

tl;dr I think we can get this in relatively quickly.

1. I pushed some minor NITS in the changelog files; everything else left as a comment.

2. Business logic-wise, I don't have any real questions/comments. It makes sense and does the job of "bridging" what I needed in the next PR.

3. My only major point of discussion (and there's lots of comments related to it) is naming. I want to bias to using "Proposal" instead of "Latest" and "Previous" instead of "Last" where appropriate. Lmk what you think

[Olshansk]

@andrewnguyen22 Left a few more minor comments, but only have one point of contention.

Understood - we should consolidate this terminology. Do you think that needs to be done in this commit?

I don't think we need to fix/consolidate everything, but I do think that for the methods & variables introduced in this commit, I'd prefer to use Proposal or Previous instead of Latest where appropriate. I personally confuse Latest with Current whenever I read it, and am not sure if it's Proposal related data (uncommitted) ore previously committed data.

[andrewnguyen22]

@Olshansk

don't think we need to fix/consolidate everything, but I do think that for the methods & variables introduced in this commit, I'd prefer to use Proposal or Previous instead of Latest where appropriate. I personally confuse Latest with Current whenever I read it, and am not sure if it's Proposal related data (uncommitted) ore previously committed data.

#332 (comment)

So in this comment, it is communicated that there are non-proposal blocks going through this lifecycle. With this being in mind, I'm not sure your recommendation makes sense.

EDIT: I'm having difficulty switching to previous - as during applyBlock it is the latest information used to commit the block...

WYT about next steps?

[Olshansk]

WYT about next steps?

Reviewed the PR, and this is the last item before we can merge it in.

The Postgres context is attached to a specific height and is meant to be an "ephemeral context" during the application/proposal lifecycle. What if we just drop the latest prefix altogether? We know that it's attached to the context, and whether that context is used for proposal or application depends on the owner of that context.

[andrewnguyen22]

@Olshansk Corrected as specified and conflicts resolved - give it another go when you can

[Olshansk]

Solid. 🚢