[Persistence] PrePersistence deprecation & Block lifecycle improvements (Issue #128 && Issue #105)

[andrewnguyen22]

Objective

closes #128

• Enable the lifecycle of block processing and production by creating a transactional architecture around persistence contexts instead of writing directly to the database on every operation.

closes #105 and closes #104

• Deprecate and remove all PrePersistence related code.

Origin Document

Background

#128
Multiple persistence contexts enable a proper lifecycle for validators and full nodes as they maintain 'ephemeral' states in situations like block validation/processing and block production.

Pretty early on - it was discovered that though ephemeral states are needed, there seems to be little validity in having multiple write contexts in parallel - rather enable write contexts sequentially (only one at a time) and enable multiple read contexts. This is currently not strictly enforced, rather needs discussion on where to delegate this responsibility (Consensus, Utility, or Persistence module).

#105
[Persistence] V1 Persistence Foundation Integration Issue

Goals / Deliverables

• Deliver completed code
• Document the architecture and intended use
• Test the transactional nature within the persistence module
• Deprecation of the PrePersistence Module and all related code, docs, resources, etc...

General milestone checklist

• Update all the relevant CHANGELOGs
• Update all the relevant READMEs
• Update the source code tree explanation
• Add or update a state, sequence or flowchart diagram using mermaid
• Create followup milestones + issues
• Document small TODO along the way

Follow-up Work

All follow-up work is being tracked in #149

---

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 or global README
• If applicable, I have added added new diagrams using mermaid.js
• If applicable, I have added tests that prove my fix is effective or that my feature works

---

Co-authored-by: Daniel Olshansky olshansky.daniel@gmail.com
Co-authored-by: Andrew Nguyen andrewnguyen@Andrews-MacBook-Pro-2.local
Co-authored-by: Andrew Nguyen andrewnguyen@Andrews-MBP-2.lan
Co-authored-by: Daniel Olshansky olshansky@pokt.network

[andrewnguyen22]

A ton of conflicts here. Will work on this in parallel - but wanted to get the PR up to get some eyes on immediately

[Olshansk]

@andrewnguyen22 It's a bit hard to review the changes in isolation because of all the conflicts with master so I think you'll need to do "merge with master" (i.e. I don't suggest a rebase here) and likely spend an hour or two resolvings. I acknowledge this isn't fun, but I think it's worth it and we'll have smaller PRs as the MVP takes shape make these less painful.

I looked at the changelogs and the interface changes and my high level understanding is this: Instead of writing to the postgres DB, we read/write to a transaction and commit it at the very end.

If that's the case, at a high level, it looks good to me and will review in depth once ready.

[andrewnguyen22]

@andrewnguyen22 It's a bit hard to review the changes in isolation because of all the conflicts with master so I think you'll need to do "merge with master" (i.e. I don't suggest a rebase here) and likely spend an hour or two resolvings. I acknowledge this isn't fun, but I think it's worth it and we'll have smaller PRs as the MVP takes shape make these less painful.

I looked at the changelogs and the interface changes and my high level understanding is this: Instead of writing to the postgres DB, we read/write to a transaction and commit it at the very end.

If that's the case, at a high level, it looks good to me and will review in depth once ready.

Understood and agree, working on it now

[andrewnguyen22]

Merged with main, conflicts gone.

Added two follow up issues #148 and #149

[Olshansk]

I took a first pass at this PR, but will do a more detailed one after we address some of the comments and discuss some of the open questions I left inline.

@andrewnguyen22 I also left a couple of open-ended refactoring comments but didn't have time to tackle the changes myself. If you find that it's too much of a waste of time or too out of scope, feel free just to leave a TODO or comment and I can address it in a follow-up PR.

[Olshansk]

@andrewnguyen22 I've spent most of the day going over this PR and the changes have become big enough that I've moved it into a separate branch for now.

I think there are some foundational business logic issues, but wanted to know your preference on:

1. Merging this as is and reviewing / discussing [Persistence] Followup on PrePersistence deprecation & Block lifecycle  #165 separately
2. Waiting for me to fix [Persistence] Followup on PrePersistence deprecation & Block lifecycle  #165 and pushing those changes to this branch

I would personally prefer (2), but am okay with (1) in order to unblock other work in parallel

[andrewnguyen22]

@andrewnguyen22 I've spent most of the day going over this PR and the changes have become big enough that I've moved it into a separate branch for now.

I think there are some foundational business logic issues, but wanted to know your preference on:

1. Merging this as is and reviewing / discussing Issue #128 fixes #165 separately
2. Waiting for me to fix Issue #128 fixes #165 and pushing those changes to this branch

I would personally prefer (2), but am okay with (1) in order to unblock other work in parallel

Let's sync offline to make sure everything goes smoothly. Not closed off to either solution, but 1) does seem attractive given you've already split the PR into two

[Olshansk]

@andrewnguyen22 As discussed offline, I'm working on the follow-ups to this in EDIT: Moving this to #165.

Hoping to approve this by EOD, but just reviewing to make sure I didn't lose any context.

I'm posting some of my previous comments, only one of which I believe needs an answer.

[Olshansk]

I played around with this and cannot understand the intended behaviour.

We only return this default value if err != nil, but if err != nil, then the result should not be used because we should handle the error.

If we want to keep this in place, what we need to do is check the err and do something like (pseudocode):

amount = 0
if err = QueryRow.scan(&amount); err != RowNotFound {
     return "", err
}
return amount, nil      

[andrewnguyen22]

We only return this default value if err != nil, but if err != nil, then the result should not be used because we should handle the error.

The error returned can be row_not_found (pseudo) which should be translated to the default value

[andrewnguyen22]

We can handle this higher in the stack if you prefer

[Olshansk]

EDIT: Moving this to #165.

I'm updating this to use DISTINCT and avoiding the IN query. Even though this probably yields some performance gains (did not benchmark), I think it's more readable / understandable to see what query we're making.

Also going to add some unit tests here too.

[Olshansk]

EDIT: Moving this to #165.

I added a few unit tests and there seemed to be a bug in the previous approach but I didn't dive deep enough to understand the reasoning. Here is the new business logic:

func SelectAccounts(height int64, tableName string) string {
	return fmt.Sprintf(`
			SELECT DISTINCT ON (address) address, balance, height
			FROM %s
			WHERE height<=%d
			ORDER BY address, height DESC
       `, tableName, height)
}

[andrewnguyen22]

Continued conversation here #165 (comment)

[Olshansk]

I think this might be the source of the issues I'm running into in the #128-fixes/improvements.

Can we reuse the same DB connection from Create here?

[Olshansk]

@andrewnguyen22 I'm getting close to finishing #165, so what are your thoughts on merging this in so #139 can start resolving merging conflicts while reviewing #165 in parallel?

Can approve the PR if that sounds good per out offline discussion.

[Olshansk]