pokt-network/pocket

Implement MVC E2E Feature Path F.5: Protocol Upgrades

0xBigBoss opened this issue · 12 comments

0xBigBoss commented

Objective: Implement MVC E2E Feature Path F.5: Protocol Upgrades

Origin Document:

Purpose: The purpose of this feature is to provide an interface for registering and applying blockchain protocol upgrades. It includes mechanisms for version-specific upgrade handlers, submitting upgrade messages, cancelling scheduled upgrades, as well as database schema and data migration during hard forks.

Actors: Check all of the protocol actors involved in the feature:

  • Validator
  • Application
  • Servicer
  • Fisherman
  • Portal

Data Structures:

message MessageUpgradeProtocol {
  bytes signer = 1;
  int64 height = 2;
  string new_version = 3;
}

message MessageCancelUpgradeProtocol {
  bytes signer = 1;
  string version = 3;
}

Interfaces:

type UpgradeModule interface {
 modules.Module

 // ApplyUpgrade applies the upgrade for the specified version.
 ApplyUpgrade(version string) error

 // SubmitUpgradeMessage submits a governance message for an upgrade.
 SubmitUpgradeMessage(message UpgradeMessage) error

 // SubmitCancelUpgradeMessage submits a governance message to cancel a scheduled upgrade.
 SubmitCancelUpgradeMessage(message CancelUpgradeMessage) error
}

Diagram:

---
title: Protocol Upgrades
---
sequenceDiagram
    participant ACL as ACL Owner
    participant B as Node

    ACL->>+B: Submit MessageUpgradeProtocol (valid input)
    Note right of B: Validate the message
    B->>-ACL: Message Submitted Successfully
    Note over B: Wait until activation height
    B->>B: Apply the protocol upgrade
    ACL->>+B: Query the protocol version
    B->>-ACL: Return the updated protocol version
    ACL->>+B: Submit MessageUpgradeProtocol (invalid input)
    Note right of B: Validate the message
    B->>-ACL: Message Rejected (Invalid Input)

---
title: Cancel Protocol Upgrades
---
sequenceDiagram
    participant ACLOwner as ACL Owner
    participant Node as Node
    participant UpgradeModule as Upgrade Module

    Note over ACLOwner,UpgradeModule: ACL Owner decides to cancel a scheduled upgrade

    ACLOwner->>+Node: Submit CancelUpgradeMessage
    Node->>-Node: Cancel the scheduled upgrade if on or before activation height
    ACLOwner->>+Node: Query the cancellation status
    Node->>-ACLOwner: Return the cancellation status

---
title: Jumping Forward Too Many Versions
---
sequenceDiagram
    participant ACL as ACL Owner
    participant B as Node

    ACL->>+B: Submit MessageUpgradeProtocol (version jump)
    Note right of B: Validate the message
    B->>-ACL: Message Rejected (Too Many Versions Ahead)

---
title: Cancel Protocol Upgrade from the Past
---
sequenceDiagram
    participant ACL as ACL Owner
    participant B as Node

    ACL->>+B: Submit MessageCancelUpgradeProtocol (past version)
    Note right of B: Validate the message
    B->>-ACL: Message Rejected (Cannot Cancel Past Upgrade)

User Stories as Tests:

Happy Paths:

  • Happy Path: As an ACL owner, I successfully submitted a protocol upgrade message with valid input and checked the protocol is upgraded after the activation height. The upgrade includes schema changes and consensus rule changes.

  • Sad Path: As an ACL owner, I attempt to submit a protocol upgrade message with invalid input. The system rejects the message.

  • Sad Path: As an ACL owner, I attempt to jump forward too many versions with a protocol upgrade message. The system rejects the message.

  • Happy Path: As a ACL Owner, I submit a CancelUpgrade when I realize an upcoming scheduled upgrade contains issues. The gets accepted before the activation height, and the scheduled upgrade is cancelled.

  • Sad Path: As a ACL Owner, I attempt to cancel a scheduled upgrade after its activation height has passed. The system rejects the CancelUpgrade.

Deliverable

  • POC:
    • A POC SPIKE to be closed out and split out into multiple PRs
  • MVP:
    • A PR that adds or modifies relevant structures and interfaces; such as shared/core/types/proto, shared/modules, etc
    • A PR that materializes an MVP of the feature along with unit tests
    • A PR that introduces a new E2E tests with one happy and one sad path scenarios as described in the origin document (refer to e2e/README.md); this may require additions to the cli
    • A PR that updates all pertinent documentation
  • PROD:
    • One or more subsequent GitHub issues that track future work including, but not limited to:
      • Enhancing test coverage
      • Adding subsequent features
      • Patching hacks or workarounds
      • Enabling your imagination!

General issue deliverables

  • Update the appropriate CHANGELOG(s)
  • Update any relevant local/global README(s)
  • Update relevant source code tree explanations
  • Add or update any relevant or supporting mermaid diagrams

Testing Methodology

  • Task specific tests or benchmarks: make ...
  • New tests or benchmarks: make ...
  • All tests: make test_all
  • LocalNet: verify a LocalNet is still functioning correctly by following the instructions at docs/development/README.md
  • k8s LocalNet: verify a k8s LocalNet is still functioning correctly by following the instructions here

Creator: @0xBigBoss
Co-owner: @Olshansk

Olshansk commented

@0xBigBoss This issue looks great!

Made some small inline nits to the description but also have a few extra questions.

RegisterUpgradeHandler(version uint64, handler UpgradeHandler) error

Rather than registering a handler (which is the async cosmos approach), the v1 repo is simpler in most sense. I believe the module should simply upgrade the protocol rather than needing a handler registration.

As a network participant, I submit a CancelUpgradeProposal through the UpgradeModule when I realize an upcoming scheduled upgrade contains issues. The proposal gets accepted before the activation height, and the scheduled upgrade is cancelled.

I believe this would need to be either 67%+ of the validator consensus or a special ACL owner as well. DO you think any network participant should be able to cancel the transaction?

Extra requests:

  1. Can you add sad/happy paths for trying to jump forward too many versions or cancel something from the instant path?
  2. Can you add a mermaid diagram with a view of the network participant (it'll be dependent on the outcome of the question above)
Olshansk commented

@0xBigBoss Wanted to check in if there are any updates or progress on this?

Olshansk commented

@h5law posted the following question in one of our internal channels. @0xBigBoss, is it fair to assume that our protocol upgrades will be the equivalent of Tendermint's RevisionNumber?

Screenshot 2023-07-17 at 3 07 27 PM

  • 👀1
0xBigBoss commented

hey @Olshansk issue description is updated.

I believe the module should simply upgrade the protocol rather than needing a handler registration.

Need to think about this, probably safe to do it this way as well. I planned on registering them during the upgrade module creation and applying any needed upgrades during the start. I guess these details can be an internal detail though and not necessarily available via interface.

I believe I wanted to keep them separated so we can have a path for upgrading a binary early without necessarily applying them. I think it also makes sense to separate the logic of which upgrades are capable of handling vs actually applying them.

Will move forward with it as a internal or submodule.

DO you think any network participant

sry this was a typo.

is it fair to assume that our protocol upgrades will be the equivalent of Tendermint's RevisionNumber

hey @h5law and @Olshansk , we could include a separate variable, or we could convert the app version string to a number. I don't have much context on the purpose of that height struct though.

h5law commented

hey @h5law and @Olshansk , we could include a separate variable, or we could convert the app version string to a number. I don't have much context on the purpose of that height struct though.

To give context here the Height struct from the image is used as part of the IBC light clients to represent the height of a network. We can define a custom implementation but I think using the revision number-revision height to represent our chain is not super out of reach from how things work currently.

Maybe converting the app version string to a number is good, I was thinking more basic in terms of V0 height 100 would look like 0-100 and V1 height 100 would be 1-100 but this can be changed and worked on as this develops

Olshansk commented

I looked at the docs [1] and here's the key takeaway:

On any reset of the RevisionHeight—for example, when hard-forking a Tendermint chain— the RevisionNumber will get incremented.

tl;dr

  1. Pocket-v1 is fundamentally a different blockchain than pocket-v0 (treat it as a separate chain where the end of v0 is the genesis of v1)
  2. Keep RevisionNumber as a monotonically increasing integer. It only increments on consensus breaking changes
  3. @h5law To unblock your work, assume that RevisionNumber is going to be the "major release" we get from the version string.

This should be enough to unblock both efforts to continue in parallel, but lmk if you have any other questions.

[1] https://ibc.cosmos.network/main/ibc/overview.html

Screenshot 2023-07-18 at 11 18 20 AM

  • 👍2
0xBigBoss commented

I'll be moving and grooving on this issue on this branch 0xbigboss/gov/upgrades-and-params

  • 👍1
Olshansk commented

I'll be moving and grooving on this issue on this branch 0xbigboss/gov/upgrades-and-params

Feel free to create a draft PR and keep pushing changes so we can provide preliminary feedback on specific parts if/when necessary.

Olshansk commented

@0xBigBoss Wanted to check in w/ a few questions:

  1. Are you working on this prior or in parallel to #879?
  2. I know you raised #930. Is this a hard blocker or are you able to continue making progress in the meantime?
  3. Any cool updates to show/share?
  4. Do you have any questions or need any help/guidance?
0xBigBoss commented
  1. Are you working on this prior or in parallel to Implement MVC E2E Feature Path F.1-4: E2E Feature Flags #879?

I am only reading that code path for now. But I don't think I'll include it in the PR other than the TODOs.

  1. I know you raised [bug][rpc] querying unconfirmed transactions fails #930. Is this a hard blocker or are you able to continue making progress in the meantime?

Nope not a blocker at all.

  1. Any cool updates to show/share?

Nothing too crazy so far! Just filling the codebase with a bunch of debug logs lol.

  1. Do you have any questions or need any help/guidance?

Not quite there yet, I may have some changes to the spec once I have the initial version e2e with upgrades, but so far so good. I am still in progress on the utility/unit of work/persistence modules and how they all work together.

  • 👍1
Olshansk commented

@0xBigBoss

Nothing too crazy so far! Just filling the codebase with a bunch of debug logs lol.

I really want to leverage the "beginner's 👀 " as you do this. If you find opportunities to improve the codebase, refactor things, improve documentation, add tooling, etc, PLEASE don't hesitate to submit PRs alongside or create issues to track it. Thanks in advance!

0xBigBoss commented

@Olshansk draft is up #948. Things have been great so far, not too many things that I've been doing differently. Was just a lot more code for me to read to get up to speed.