[TxIndexer] Integration of transaction indexer (issue-#168) #302

andrewnguyen22 · 2022-10-12T20:01:44Z

Description

Issue-#151 laid the foundation for a proper transaction indexer in isolation.

The integration of the transaction indexer into the M1 lifecycle is pending and required.

Integrate the transaction indexer into the lifecycle of M1

Issue

Fixes #168

Type of change

Please mark the relevant option(s):

List of changes

Added TxIndexer sub-package (previously in Utility Module)
Added TxIndexer to both PersistenceModule and PersistenceContext
Implemented TransactionExists and StoreTransaction
Stores transactions along side blocks during commit
Added current block []TxResult to the module
Moved TxIndexer package to persistence module
Added new proto structure DefaultTxResult
Integrated the TxIndexer into the lifecycle
- Captured TxResult from each played transaction
- Moved the storage of transactions to the Consensus Module
- Returned the TxResults in the ApplyBlock() and GetProposalTransactions()
- AnteHandleMessage() now returns signer
- HandleMessage() now returns messageType and recipient
- ApplyTransaction() returns TxResult
Modified interface for Utility Module ApplyBlock and GetProposalTransactions to return TxResults
Modified interface for Persistence Module StoreTransaction to store the TxResult
Added shared interface TxResult under types.go

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

@andrewnguyen22 Took a first pass with some comments, but I also think it'll be productive to jump and discuss this over a call (@deblasis welcome to join). Specifically, I want to understand if:

We can encapsulate the logic of indexing transaction to a postgres context (which can be rolled back and released)
Avoid modifying utility's external-looking interfaces (it's less intuitive to me now)
Get a better personal understand of the end-to-end flow & purpose of StoreTransaction and as it relates [Persistence] First Implementation of the StateHash (#284) #285.

Note, I will pause all work on #285 until we figure this out.

consensus/CHANGELOG.md

consensus/block.go

consensus/hotstuff_leader.go

utility/doc/CHANGELOG.md

Olshansk · 2022-10-18T20:40:20Z

utility/types/proto/tx_result.proto

+
+option go_package = "github.com/pokt-network/pocket/utility/types";
+
+message DefaultTxResult {


Why do we call it just TxResult in persistence/indexer/proto/transaction_indexer.proto but DefaultTxResult here?

TxResult is the interface DefaultTxResult is the default transaction result implementation

I realise I'm nit picking here, so feel free to push back (or let me know if we should do this in a future commit), but I'm just trying to understanding name naming

When I see DefaultTxResult, I'm expecting there to be a non default too. And given that this is scoped to be within the utility module, does that mean we expect the utility module to have another TxResult at some point?

I would've named the interface to ITxResult and this proto to TxResult, but just wanted to understand the motivation first.

The expectation - is that we could have other implementations in the future. The Default vs NonDefault pattern is found in golang libraries. If you think it's inappropriate in this context I can change it. I don't think it's a big deal IMO.

Yea, not a big deal and not a reason for a blocker, but I wanted to understand.

We can always change it later

persistence/proto/persistence_config.proto

shared/modules/utility_module.go

utility/transaction.go

Co-authored-by: Daniel Olshansky <[email protected]>

andrewnguyen22 · 2022-10-19T14:24:22Z

@Olshansk

Get a better personal understand of the end-to-end flow & purpose of StoreTransaction and as it relates #285.

I can get on a call and explain it to you if you can write up an async summary

Olshansk · 2022-10-20T03:50:28Z

@andrewnguyen22 I believe that we are aligned on next steps for both this and the follow up PR given the details in #315, but let me know if I missed anything or when this PR is simply ready for the next review.

Co-authored-by: Daniel Olshansky <[email protected]>

…#168

andrewnguyen22 · 2022-10-20T20:43:25Z

@Olshansk #315 looks good for a first pass. I'm ready to move forward if you are

Olshansk

Good use of the high priority label. Will prioritize looking at this again tomorrow morning.

Mostly just left a couple nits, a couple questions, and inserted #315 TODO in a couple places to make sure we're aligned.

consensus/hotstuff_leader.go

consensus/module.go

shared/modules/persistence_module.go

shared/modules/utility_module.go

Olshansk · 2022-10-21T03:21:27Z

utility/block.go

-		}
-		if err := u.GetPersistenceContext().StoreTransaction(transactionProtoBytes); err != nil {
-			return nil, err
+		// DISCUSS: currently, the pattern is allowing nil err with an error transaction...


ACK. Will think about it.

Olshansk · 2022-10-21T03:23:19Z

utility/test/transaction_test.go

@@ -133,7 +134,7 @@ func TestUtilityContext_HandleMessage(t *testing.T) {

 func newTestingTransaction(t *testing.T, ctx utility.UtilityContext) (transaction *typesUtil.Transaction, startingAmount, amountSent *big.Int, signer crypto.PrivateKey) {
 	cdc := codec.GetCodec()
-	recipient := GetAllTestingAccounts(t, ctx)[1]
+	recipient := GetAllTestingAccounts(t, ctx)[2]


Not a blocker, but curious why this was changed.

To prevent a collision with the first Validator who is the proposer in tests

utility/transaction.go

utility/types/message.go

Olshansk · 2022-10-21T03:40:38Z

utility/types/proto/tx_result.proto

+
+option go_package = "github.com/pokt-network/pocket/utility/types";
+
+message DefaultTxResult {


I realise I'm nit picking here, so feel free to push back (or let me know if we should do this in a future commit), but I'm just trying to understanding name naming

When I see DefaultTxResult, I'm expecting there to be a non default too. And given that this is scoped to be within the utility module, does that mean we expect the utility module to have another TxResult at some point?

I would've named the interface to ITxResult and this proto to TxResult, but just wanted to understand the motivation first.

Co-authored-by: Daniel Olshansky <[email protected]>

Olshansk

I'm not the biggest fan of the DefaultTxResult naming, but I'll look around what the best practices are in other major Goland libraries.

Please see the two mini edits I have, but otherwise good to go.

utility/doc/CHANGELOG.md

utility/test/transaction_test.go

Olshansk · 2022-10-21T18:07:03Z

utility/types/proto/tx_result.proto

+
+option go_package = "github.com/pokt-network/pocket/utility/types";
+
+message DefaultTxResult {


Yea, not a big deal and not a reason for a blocker, but I wanted to understand.

We can always change it later

Co-authored-by: Daniel Olshansky <[email protected]>

## 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 - [x] **Enable committing of a block to persistence by simply committing the persistence context** - [x] Remove ephemeral block-related state from the consensus module & utility module, and only maintain it in the persistence module - [x] 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 - [x] Code health or cleanup - [x] 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-#168) #302) - Combined creation and application of block in proposer lifecycle ## 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` ## Required 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 - [x] 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](https://mermaid-js.github.io) diagrams in the corresponding README(s) - [ ] I have added, or updated, documentation and [mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*` if I updated `shared/*`README(s)

issue-#168

00c882e

andrewnguyen22 added the core Core infrastructure - protocol related label Oct 12, 2022

andrewnguyen22 added this to the M1: Pocket PoS (Proof of Stake) milestone Oct 12, 2022

andrewnguyen22 requested review from Olshansk and deblasis October 12, 2022 20:01

andrewnguyen22 self-assigned this Oct 12, 2022

andrewnguyen22 requested a review from a team as a code owner October 12, 2022 20:01

andrewnguyen22 mentioned this pull request Oct 18, 2022

[Persistence] First Implementation of the StateHash (#284) #285

Merged

19 tasks

Olshansk requested changes Oct 18, 2022

View reviewed changes

Andrew Nguyen and others added 3 commits October 19, 2022 10:11

Update consensus/CHANGELOG.md

cc39075

Co-authored-by: Daniel Olshansky <[email protected]>

Update utility/doc/CHANGELOG.md

8fbcd36

Co-authored-by: Daniel Olshansky <[email protected]>

Update utility/types/transaction.go

de4fb81

Co-authored-by: Daniel Olshansky <[email protected]>

Olshansk mentioned this pull request Oct 20, 2022

[Persistence] TxIndexer to StateHash Bridge #315

Closed

12 tasks

Andrew Nguyen and others added 4 commits October 20, 2022 16:21

Update consensus/block.go

4b575ab

Co-authored-by: Daniel Olshansky <[email protected]>

olshansk r1

90c8107

merge with origin/main

05a003d

Merge branch 'issue-#168' of github.com:pokt-network/pocket into issue-…

321f312

…#168

andrewnguyen22 requested a review from Olshansk October 20, 2022 20:43

andrewnguyen22 added the priority:high label Oct 20, 2022

Olshansk requested changes Oct 21, 2022

View reviewed changes

Andrew Nguyen and others added 2 commits October 21, 2022 12:08

Apply suggestions from code review

88ecc89

Co-authored-by: Daniel Olshansky <[email protected]>

olshansk r2

3fa9a54

andrewnguyen22 requested a review from Olshansk October 21, 2022 16:16

Olshansk approved these changes Oct 21, 2022

View reviewed changes

Apply suggestions from code review

a51d249

Co-authored-by: Daniel Olshansky <[email protected]>

andrewnguyen22 merged commit 54f244b into main Oct 21, 2022

andrewnguyen22 deleted the issue-#168 branch October 21, 2022 19:03

andrewnguyen22 mentioned this pull request Nov 1, 2022

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

Merged

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[TxIndexer] Integration of transaction indexer (issue-#168) #302

[TxIndexer] Integration of transaction indexer (issue-#168) #302

andrewnguyen22 commented Oct 12, 2022

Olshansk left a comment

Olshansk Oct 18, 2022

andrewnguyen22 Oct 19, 2022

Olshansk Oct 21, 2022

andrewnguyen22 Oct 21, 2022

Olshansk Oct 21, 2022

andrewnguyen22 commented Oct 19, 2022

Olshansk commented Oct 20, 2022

andrewnguyen22 commented Oct 20, 2022

Olshansk left a comment

Olshansk Oct 21, 2022

Olshansk Oct 21, 2022

andrewnguyen22 Oct 21, 2022

Olshansk Oct 21, 2022

Olshansk left a comment

Olshansk Oct 21, 2022


		option go_package = "github.com/pokt-network/pocket/utility/types";

		message DefaultTxResult {

[TxIndexer] Integration of transaction indexer (issue-#168) #302

[TxIndexer] Integration of transaction indexer (issue-#168) #302

Conversation

andrewnguyen22 commented Oct 12, 2022

Description

Issue

Type of change

List of changes

Testing

Required Checklist

If Applicable Checklist

Olshansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewnguyen22 commented Oct 19, 2022

Olshansk commented Oct 20, 2022

andrewnguyen22 commented Oct 20, 2022

Olshansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olshansk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment