[core] Protocol Upgrades #948

0xBigBoss · 2023-07-29T22:13:58Z

Description

This PR aims to implement #882. It includes so far cli, rpc, and persistence related changes. It is still incomplete according to #882.

This goal of this PR is give the ACL owner the ability to upgrade the protocol.

I am using this script so far to run the commands and develop locally, https://gist.github.com/0xBigBoss/d1a576d857f77dbe42292daa7a32bb2e.

Summary generated by Reviewpad on 09 Aug 23 15:48 UTC

This pull request introduces various changes across multiple files:

The file "validator.feature" has a change related to the scenario "User Wants Help Using The Validator Command". The command "Validator help" has been changed to "the user runs the command with no error "Validator help"".
In the file "persistence/trees/trees_test.go", the constant treesHash1 has been updated.
The diff in the file "Dockerfile.localdev" includes changes related to package installation and file copying.
The file "rpc/utils.go" has modifications related to handling the MessageUpgrade messageType and calculating the message fee.
A new test case has been added in the file "e2e/steps_gov_test.go" to verify the behavior of importing and retrieving an ACL owner key.
The file "account.go" has error handling and logging enhancements in the getAccountAmount function.
The file "benchmark_state_test.go" introduces changes related to benchmarking and modifying the persistence context in unit tests.
The file "openapi.yaml" includes changes in schema definitions and a TODO task for a future task.
The diff in the file "handlers.go" includes changes to the PostV1ClientBroadcastTxSync function, specifically returning a response with the transaction hash.
The diff in the file "txmessage_error.go" includes changes related to error codes and error functions.
The diff in the file "defaults.go" includes changes in variable values related to upgrading a message fee.
The diff in the file "Dockerfile.localdev" includes changes related to package installation and file copying.
The diff in the file "message_upgrade_test.go" adds unit tests for the MessageUpgrade struct.
The file "transaction.go" in the utility/unit_of_work directory includes changes in error handling and logging.
In the file "e2e/README.md", there is a change in a scenario command.
The file "upgrades.go" introduces new constants and an update function.
The diff in the file "handlers.go" includes changes related to returning a transaction hash as part of the response.
The file "transaction_errors.go" includes changes related to constants, error functions, and updates to the package imports.
The file "defaults.go" includes changes in variables related to upgrading government parameters.
The changes in the file "Dockerfile.localdev" include updates to package installations and a code removal.
The diff in the file "message_upgrade_test.go" includes changes related to importing packages, added functions, and versions.
The file "upgrade.proto" introduces a new protobuf message called "Upgrade".
The file "block.go" indicates potential locations for pausing and state migrations.

Please review these changes and let me know if you need further assistance.

Issue

Fixes #882

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

Change prop: the agreed upon repository structure #1
Change Version Number #2
...

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)

Olshansk · 2023-07-31T18:57:57Z

@adshmh Could you help review this PR?

Olshansk · 2023-08-02T21:38:43Z

@0xBigBoss Before I dive into reviewing this PR, I noticed its still a draft and wanted to ask:

How much larger is this going to be?
Is it fair to assume other functionality will come in subsequent PRs?
Can we make it not-a-daft?

My goal is to avoid spending time reviewing documentation and code that will have major changes.

0xBigBoss · 2023-08-03T03:10:49Z

@0xBigBoss Before I dive into reviewing this PR, I noticed its still a draft and wanted to ask:

How much larger is this going to be?

Is it fair to assume other functionality will come in subsequent PRs?

Can we make it not-a-daft?

My goal is to avoid spending time reviewing documentation and code that will have major changes.

Hey @Olshansk the part i am currently in the middle of is investigating the best place to perform the upgrades and writing the upgrade module. Nothing definitive yet for that. Not sure exactly this upgrade module would do now since any database migrations would have to be thought through carefully.

this is a fair assumption.
done.

edit I think it'll be best to update the e2e tests in this PR after your statesync pr has been merged.

Adds servicer pk to validator1 docker compose deployment

0xBigBoss · 2023-08-06T02:05:51Z

Could I get a review on this? @adshmh @Olshansk so far the cli, rpc, consensus, utility and persistence modules have all been updated to include sending upgrade messages.

Not all of the e2e tests will pass due to the error handling and not saving invalid transactions yet.

Some things that I think we should track in #882 or a new issue:

boilerplate upgrade module to house the actual persistence migrations
logic for pausing a node from syncing when not upgraded to the latest version.
logic for pausing modules and performing an upgrade at activation height

Olshansk · 2023-08-08T19:37:06Z

@0xBigBoss Sorry for the delay. On my list for this week. Could you merge with main & resolve conflicts in the meantime?

adshmh

Great work!
Adding unit testing to utility unit of work is a key task IMO.

adshmh · 2023-08-03T12:50:46Z

app/client/cli/gov.go

-				// DISCUSS(#310): define UX for return values - should we return the raw response or a parsed/human readable response? For now, I am simply printing to stdout
+
+				if resp.StatusCode() != 200 {
+					return fmt.Errorf("HTTP status code: %d\n", resp.StatusCode())


Is there anything else in the response we can print? A single status code, e.g. 404 for a bad request, does not help the user understand why a request failed.

Not to my immediate knowledge. I haven't extensively reviewed our rpc error handling nor our openapi spec. In any case, this should be a new opened in a new issue with your requirements here https://github.com/pokt-network/pocket/blob/main/rpc/v1/openapi.yaml#L1

app/client/cli/gov.go

adshmh · 2023-08-03T13:00:05Z

build/Dockerfile.client

@@ -1,17 +1,8 @@
-ARG GOLANG_IMAGE_VERSION=golang:1.20-alpine3.16
+ARG GOLANG_IMAGE_VERSION=golang:1.20-bookworm


Not against this change, but using a bare-minimum image like alpine and adding what we need to it is highly preferable to using a (potentially) bloated image that happens to have the packages we need preinstalled.

heh ur an infra guy too? yeah i made this change to have parity with the localnet since both were using debian. Alpine is great for prod, a bit tough on people in development.

In any case, what is ur action here and let's move it to a new issue as to not slow down protocol work.

adshmh · 2023-08-03T13:02:53Z

build/Dockerfile.localdev

@@ -25,3 +23,5 @@ RUN go install github.com/go-delve/delve/cmd/dlv@latest
 # Needed to make `go install dlv` and `dlv debug` work...
 RUN apk update && apk add --no-cache gcc musl-dev
 RUN go get github.com/go-delve/delve/cmd/dlv@latest
+
+COPY . .


Not sure I understand the reason for this change. It is copying the same location, i.e. current directory. The previous commands should not change the current directory's contents AFAIK.

This step is redundant and unfortunately has cost a lot of us a lot of CPU time. The docker compose file from which this is referenced mounts the same directory as a volume into the container.

persistence/sql/sql.go

adshmh · 2023-08-08T14:59:03Z

persistence/gov.go

+func (p *PostgresContext) SetUpgrade(signer, version string, height int64) error {
+	ctx, tx := p.getCtxAndTx()
+	_, err := tx.Exec(ctx, `
+INSERT INTO upgrades(signer, version, height, created) VALUES ($1, $2, $3, $4)


Considering the importance of any upgrade rows, does it makes sense to verify if such a row already exists and return an error?

Yes, I included that in the schema,

pocket/persistence/types/gov.go

Lines 40 to 52 in 08d55bc

CREATE TABLE IF NOT EXISTS upgrades

(

signer text NOT NULL,

version varchar(10) NOT NULL,

height bigint NOT NULL UNIQUE,

created bigint NOT NULL UNIQUE,

PRIMARY KEY (version)

);

COMMENT ON TABLE upgrades IS 'stores the upgrade history of the network';

COMMENT ON COLUMN upgrades.signer IS 'the address of the signer of the upgrade';

COMMENT ON COLUMN upgrades.version IS 'the semver 2.0 version of the upgrade';

COMMENT ON COLUMN upgrades.height IS 'the activation height of the upgrade';

COMMENT ON COLUMN upgrades.created IS 'the height the upgrade was created';

.

adshmh · 2023-08-08T15:01:27Z

persistence/test/benchmark_state_test.go


 		// Preliminary filter to determine which functions we're interested in trying to call
 		if !isModifierRe.MatchString(methodName) {
 			continue
 		}

+		// IMPROVE: This is a hack to prevent the SetUpgrade function from being called more than once
+		if methodName == "SetUpgrade" && count > 0 {


If SetUpgrade returns an error if called twice with the same arguments (as suggested in the above comment), this would not be needed anymore.

please review the purpose of this test

pocket/persistence/test/state_test.go

Lines 109 to 112 in 580dbfb

// This unit test generates random transactions and creates random state changes, but checks

// that replaying them will result in the same state hash, guaranteeing the integrity of the

// state hash.

It's not intended to validate the inputs or crash.

utility/unit_of_work/tx_message_handler.go

utility/unit_of_work/uow_leader.go

Co-authored-by: Arash <[email protected]>

Olshansk · 2023-08-09T22:01:34Z

@0xBigBoss Just a heads up that we're having an internal sprint to have the entire core team to focus on a specific aspect of Pocket v1, but will follow up afterwards.

Will leave the review up as is for a couple of weeks.

0xBigBoss added the core Core infrastructure - protocol related label Jul 29, 2023

0xBigBoss requested a review from Olshansk July 29, 2023 22:13

0xBigBoss self-assigned this Jul 29, 2023

reviewpad bot added the large Pull request is large label Jul 29, 2023

0xBigBoss mentioned this pull request Jul 29, 2023

Implement MVC E2E Feature Path F.5: Protocol Upgrades #882

Open

20 tasks

Olshansk requested a review from adshmh July 31, 2023 18:57

Olshansk added this to the M3: Pocket RoS (Relay or Slash) milestone Jul 31, 2023

0xBigBoss force-pushed the 0xbigboss/gov/upgrades-and-params branch from aab0217 to 4291b3e Compare August 1, 2023 03:36

0xBigBoss marked this pull request as ready for review August 3, 2023 03:10

reviewpad bot added the waiting-for-review label Aug 3, 2023

0xBigBoss added 16 commits August 3, 2023 21:13

scope: add upgrade feature files and todos

729b558

step: the user is an acl owner

8239913

cleanup

15dc15f

add message upgrade, start cli

68601ba

turn up the debugs

f21945e

show query mempool failing

c555dcb

wip handling upgrade

8a3a246

📝 message upgrade

52ac14f

🐳 docker tweaks

4408e6a

💸 to acl owner

d34715a

📝 persistence

ff0ea16

fix tests, handle when no upgrades

8ff4565

fix tests, handle when no upgrades

f96a3f1

fix flaky test

7a18107

e2e checkpoint

53422aa

speed up local dev dockerfiles

c843e47

0xBigBoss force-pushed the 0xbigboss/gov/upgrades-and-params branch from 4291b3e to c843e47 Compare August 4, 2023 19:55

0xBigBoss added 5 commits August 4, 2023 18:24

wip on fixing e2e

e33bc06

[utility] fix failed to create servicer

45a0f56

Adds servicer pk to validator1 docker compose deployment

fix tests

b784a7e

add missing message_upgrade_fee to localnet

4e6e905

localnet improvements

5c07b9d

0xBigBoss force-pushed the 0xbigboss/gov/upgrades-and-params branch from 7b337ed to 6d1105c Compare August 6, 2023 01:44

add upgrade validations, update e2e tests

2cd8bd8

0xBigBoss force-pushed the 0xbigboss/gov/upgrades-and-params branch from 6d1105c to 2cd8bd8 Compare August 6, 2023 01:58

Merge branch 'main' into 0xbigboss/gov/upgrades-and-params

f637943

adshmh requested changes Aug 9, 2023

View reviewed changes

0xBigBoss and others added 5 commits August 9, 2023 10:49

clearer var names

71b1c51

formatting

3357f2e

test handle message upgrade

580dbfb

Update utility/unit_of_work/uow_leader.go

02a8e4c

Co-authored-by: Arash <[email protected]>

format

08d55bc

0xBigBoss closed this Nov 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Protocol Upgrades #948

[core] Protocol Upgrades #948

0xBigBoss commented Jul 29, 2023 •

edited by reviewpad bot

Loading

Olshansk commented Jul 31, 2023

Olshansk commented Aug 2, 2023

0xBigBoss commented Aug 3, 2023 •

edited

Loading

0xBigBoss commented Aug 6, 2023

Olshansk commented Aug 8, 2023

adshmh left a comment

adshmh Aug 3, 2023

0xBigBoss Aug 9, 2023 •

edited

Loading

adshmh Aug 3, 2023

0xBigBoss Aug 9, 2023

adshmh Aug 3, 2023

0xBigBoss Aug 9, 2023

adshmh Aug 8, 2023

0xBigBoss Aug 9, 2023

adshmh Aug 8, 2023

0xBigBoss Aug 9, 2023

Olshansk commented Aug 9, 2023

		@@ -1,17 +1,8 @@
		ARG GOLANG_IMAGE_VERSION=golang:1.20-alpine3.16
		ARG GOLANG_IMAGE_VERSION=golang:1.20-bookworm

	CREATE TABLE IF NOT EXISTS upgrades
	(
	signer text NOT NULL,
	version varchar(10) NOT NULL,
	height bigint NOT NULL UNIQUE,
	created bigint NOT NULL UNIQUE,
	PRIMARY KEY (version)
	);
	COMMENT ON TABLE upgrades IS 'stores the upgrade history of the network';
	COMMENT ON COLUMN upgrades.signer IS 'the address of the signer of the upgrade';
	COMMENT ON COLUMN upgrades.version IS 'the semver 2.0 version of the upgrade';
	COMMENT ON COLUMN upgrades.height IS 'the activation height of the upgrade';
	COMMENT ON COLUMN upgrades.created IS 'the height the upgrade was created';


	// This unit test generates random transactions and creates random state changes, but checks
	// that replaying them will result in the same state hash, guaranteeing the integrity of the
	// state hash.

[core] Protocol Upgrades #948

[core] Protocol Upgrades #948

Conversation

0xBigBoss commented Jul 29, 2023 • edited by reviewpad bot Loading

Description

Summary generated by Reviewpad on 09 Aug 23 15:48 UTC

Issue

Type of change

List of changes

Testing

Required Checklist

If Applicable Checklist

Olshansk commented Jul 31, 2023

Olshansk commented Aug 2, 2023

0xBigBoss commented Aug 3, 2023 • edited Loading

0xBigBoss commented Aug 6, 2023

Olshansk commented Aug 8, 2023

adshmh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xBigBoss Aug 9, 2023 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Olshansk commented Aug 9, 2023

0xBigBoss commented Jul 29, 2023 •

edited by reviewpad bot

Loading

0xBigBoss commented Aug 3, 2023 •

edited

Loading

0xBigBoss Aug 9, 2023 •

edited

Loading