pokt-network / pocket

Official implementation of the Pocket Network Protocol v1
https://pokt.network
MIT License
61 stars 33 forks source link

[core] Protocol Upgrades #948

Closed 0xBigBoss closed 7 months ago

0xBigBoss commented 11 months ago

Description

This PR aims to implement https://github.com/pokt-network/pocket/issues/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:

  1. 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"".

  2. In the file "persistence/trees/trees_test.go", the constant treesHash1 has been updated.

  3. The diff in the file "Dockerfile.localdev" includes changes related to package installation and file copying.

  4. The file "rpc/utils.go" has modifications related to handling the MessageUpgrade messageType and calculating the message fee.

  5. 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.

  6. The file "account.go" has error handling and logging enhancements in the getAccountAmount function.

  7. The file "benchmark_state_test.go" introduces changes related to benchmarking and modifying the persistence context in unit tests.

  8. The file "openapi.yaml" includes changes in schema definitions and a TODO task for a future task.

  9. The diff in the file "handlers.go" includes changes to the PostV1ClientBroadcastTxSync function, specifically returning a response with the transaction hash.

  10. The diff in the file "txmessage_error.go" includes changes related to error codes and error functions.

  11. The diff in the file "defaults.go" includes changes in variable values related to upgrading a message fee.

  12. The diff in the file "Dockerfile.localdev" includes changes related to package installation and file copying.

  13. The diff in the file "message_upgrade_test.go" adds unit tests for the MessageUpgrade struct.

  14. The file "transaction.go" in the utility/unit_of_work directory includes changes in error handling and logging.

  15. In the file "e2e/README.md", there is a change in a scenario command.

  16. The file "upgrades.go" introduces new constants and an update function.

  17. The diff in the file "handlers.go" includes changes related to returning a transaction hash as part of the response.

  18. The file "transaction_errors.go" includes changes related to constants, error functions, and updates to the package imports.

  19. The file "defaults.go" includes changes in variables related to upgrading government parameters.

  20. The changes in the file "Dockerfile.localdev" include updates to package installations and a code removal.

  21. The diff in the file "message_upgrade_test.go" includes changes related to importing packages, added functions, and versions.

  22. The file "upgrade.proto" introduces a new protobuf message called "Upgrade".

  23. 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):

List of changes

Testing

Required Checklist

If Applicable Checklist

Olshansk commented 11 months ago

@adshmh Could you help review this PR?

Olshansk commented 11 months ago

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

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

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

0xBigBoss commented 11 months ago

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

  1. How much larger is this going to be?
  2. Is it fair to assume other functionality will come in subsequent PRs?
  3. 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.

  1. this is a fair assumption.
  2. done.

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

0xBigBoss commented 10 months ago

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:

Olshansk commented 10 months ago

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

Olshansk commented 10 months ago

@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.