[Persistence] Update block header #915

h5law · 2023-07-18T18:20:39Z

Description

Summary generated by Reviewpad on 20 Jul 23 20:52 UTC

This pull request includes the following changes:

Added a new function GetTreeHashes in the trees.go file.
Added import statements in various files.
Added functions and tests related to preparing and inserting blocks, getting validator set hashes, and hashing validator set.
Added a test function TestTreeStore_GetTreeHashes in the trees_test.go file.
Made changes to the block.proto file including updating the BlockHeader message, adding the ValidatorSet message, and adding the ValidatorIdentity message.
Added a method GetTreeHashes in the treestore_module.go file.
Made changes to the openapi.yaml file related to the "Block" schema.
Added a new function GetValidatorSet in the actor.go file.
Made changes to the actor_test.go file related to importing packages and adding test functions.
Made changes to the rpc/utils.go file related to adding fields to the BlockHeader struct in the blockToRPCBlock function.
Added a new method GetValidatorSet to the PersistenceReadContext interface in the persistence_module.go file.

Issue

Fixes N/A

Type of change

Please mark the relevant option(s):

List of changes

Adds state tree hashes as a map to the block header
Add next validator set hash to the block header

Testing

make develop_test; if any code changes were made
make test_e2e on k8s LocalNet; if any code changes were made
e2e-devnet-test passes tests on DevNet; if any code was changed
Docker Compose LocalNet; if any major functionality was changed or introduced
k8s LocalNet; if any infrastructure or configuration changes were made

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 added, or updated, godoc format comments on touched members (see: tip.golang.org/doc/comment)
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)

… proto

Olshansk · 2023-07-18T18:21:42Z

@h5law Friendly reminder to please update Issue/PR metdata

h5law · 2023-07-18T18:23:02Z

Friendly reminder to please update Issue/PR metdata

ur too fast

persistence/actor.go

Olshansk · 2023-07-18T21:16:47Z

persistence/trees/trees.go

@@ -101,6 +101,16 @@ func (t *treeStore) GetTree(name string) ([]byte, kvstore.KVStore) {
 	return nil, nil
 }

+// GetTreeHashes returns a map of tree names to their root hashes for all
+// the trees tracked by the treestore, excluding the root tree
+func (t *treeStore) GetTreeHashes() map[string]string {


Can you add a unit test for this?

Not without possible conflicts in #861 - it would require the same boilerplate setup functions to be copied over into trees/trees_test.go I can do this if you think its 100% necissary but this logic is pretty straighforward, I can leave a skip test that can be implemented after #861 is in?

shared/core/types/proto/block.proto

…o definition

shared/core/types/proto/block.proto

persistence/actor.go

persistence/block.go

persistence/test/actor_test.go

Olshansk · 2023-07-19T23:17:55Z

persistence/test/actor_test.go

+	)
+	require.NoError(t, err)
+
+	currValSet, err = db.GetValidatorSet(currHeight - 1)


Can you also update the test to insure the validators are sorted by address?

So the validators are sorted by address and height this means they could look like this:
"aaaaaaa"(height10)
"ccccccc" (height9)
"bbbbbb"(height9)
"zzzzzzz"(height1)
"ddddddd"(height1)
What is important is that although the ordering may not be lexigraphically ordered by address alone it is still deterministic by height. This means the hash will be consistent by height. I think testing they are lexigraphically ordered only passes at genesis and afterwards will not alway be the case

My request is to update the test to verify this explicitly:

For example:

require.Equal(t, "aaaaa", valSet[0].address) // height10 require.Equal(t, "cccc", valSet[0].address) // height9 require.Equal(t, "bbb", valSet[0].address) // height8

Makes it easier for future maintainers of the code and also increase the surface area of bugs

My bad I actually misunderstood the query. They are sorted by address ASC alone as the select is ON DISTINCT it means even if the address is at multiple heights only the most recent is chosen but results are still sorted by address ASC. Updated to test for this.

Olshansk

Just one more request: https://github.com/pokt-network/pocket/pull/915/files#r1268771205

Olshansk · 2023-07-20T20:24:10Z

persistence/test/actor_test.go

+	)
+	require.NoError(t, err)
+
+	currValSet, err = db.GetValidatorSet(currHeight - 1)


My request is to update the test to verify this explicitly:

For example:

require.Equal(t, "aaaaa", valSet[0].address) // height10 require.Equal(t, "cccc", valSet[0].address) // height9 require.Equal(t, "bbb", valSet[0].address) // height8

Makes it easier for future maintainers of the code and also increase the surface area of bugs

Olshansk · 2023-07-20T21:43:15Z

persistence/test/actor_test.go

+		if i == 0 {
+			continue
+		}
+		require.True(t, val.Address > nextValSet.Validators[i-1].Address)


h5law added 2 commits July 18, 2023 19:20

Add state tree hashes and next validator set hash to the block header…

bc0c382

… proto

Add RPC support for updated block header

b7016aa

h5law added persistence Persistence specific changes e2e-devnet-test Runs E2E tests on devnet labels Jul 18, 2023

h5law added this to the M7: Pocket NoS (North Star) milestone Jul 18, 2023

h5law requested review from Olshansk and dylanlott July 18, 2023 18:20

h5law self-assigned this Jul 18, 2023

reviewpad bot added small Pull request is small waiting-for-review labels Jul 18, 2023

Address linter error

25db75c

Olshansk requested changes Jul 18, 2023

View reviewed changes

Address comments

959854a

reviewpad bot added medium Pull request is medium and removed small Pull request is small labels Jul 18, 2023

h5law requested a review from Olshansk July 18, 2023 22:35

h5law added 5 commits July 19, 2023 13:17

GetValidatorSetHash => GetValidatorSet; add custom validator set prot…

9130821

…o definition

Add RPC for current and next validator set hashes

d509039

remove proposer from validator set proto

9aceb1f

add named returns

ed270c2

fix linter errors

9676add

Olshansk requested changes Jul 19, 2023

View reviewed changes

address comments

adbaa39

h5law requested a review from Olshansk July 20, 2023 09:19

h5law added 2 commits July 20, 2023 10:50

add discussion comment

53e0c46

update comments

3915107

Olshansk requested changes Jul 20, 2023

View reviewed changes

test for lexicographical sorting

2d7eedf

h5law requested a review from Olshansk July 20, 2023 20:53

Olshansk approved these changes Jul 20, 2023

View reviewed changes

persistence/test/actor_test.go

if i == 0 {

continue

}

require.True(t, val.Address > nextValSet.Validators[i-1].Address)

Copy link

Member

Olshansk Jul 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clever!

h5law merged commit 66137cd into main Jul 20, 2023
10 checks passed

h5law deleted the persistence/update_block_header branch July 20, 2023 21:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Persistence] Update block header #915

[Persistence] Update block header #915

h5law commented Jul 18, 2023 •

edited by reviewpad bot

Loading

Olshansk commented Jul 18, 2023

h5law commented Jul 18, 2023

Olshansk Jul 18, 2023

h5law Jul 18, 2023

Olshansk Jul 19, 2023

h5law Jul 20, 2023

Olshansk Jul 20, 2023

h5law Jul 20, 2023

Olshansk left a comment

Olshansk Jul 20, 2023

Olshansk Jul 20, 2023

[Persistence] Update block header #915

[Persistence] Update block header #915

Conversation

h5law commented Jul 18, 2023 • edited by reviewpad bot Loading

Description

Summary generated by Reviewpad on 20 Jul 23 20:52 UTC

Issue

Type of change

List of changes

Testing

Required Checklist

If Applicable Checklist

Olshansk commented Jul 18, 2023

h5law commented Jul 18, 2023

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

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

h5law commented Jul 18, 2023 •

edited by reviewpad bot

Loading