Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs: ADR 018 Network Upgrades #1562

Merged
merged 10 commits into from
Sep 15, 2023
Merged

docs: ADR 018 Network Upgrades #1562

merged 10 commits into from
Sep 15, 2023

Conversation

evan-forbes
Copy link
Member

@evan-forbes evan-forbes commented Mar 29, 2023

Overview

ADR for social upgrades, still a work in progress in that we still need to finalize a few decisions before moving to the implementation detail phase, but this ADR is ready for the first round of reviews.

Related to ADR 016

rendered

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@evan-forbes evan-forbes added the ADR item is directly relevant to writing or modifying an ADR label Mar 29, 2023
@evan-forbes evan-forbes self-assigned this Mar 29, 2023
@MSevey MSevey requested a review from a team March 29, 2023 16:50
rootulp
rootulp previously approved these changes Mar 29, 2023
docs/architecture/adr-018-social-upgrades.md Outdated Show resolved Hide resolved
docs/architecture/adr-018-social-upgrades.md Outdated Show resolved Hide resolved
docs/architecture/adr-018-social-upgrades.md Outdated Show resolved Hide resolved
docs/architecture/adr-018-social-upgrades.md Outdated Show resolved Hide resolved
docs/architecture/adr-018-social-upgrades.md Outdated Show resolved Hide resolved
docs/architecture/adr-018-social-upgrades.md Outdated Show resolved Hide resolved
@MSevey MSevey requested a review from a team March 29, 2023 20:21
@evan-forbes
Copy link
Member Author

per sync discussion, instead of halting the chain, we can encode a rule where after a specific height we increase the community tax to 100%

@evan-forbes evan-forbes mentioned this pull request Mar 30, 2023
4 tasks
@evan-forbes evan-forbes changed the title ADR 018: Social Upgrades docs: ADR 018 Social Upgrades Apr 2, 2023
@evan-forbes evan-forbes marked this pull request as draft April 3, 2023 10:58
@evan-forbes
Copy link
Member Author

converting to a draft and might close, as there's not that much useful info in the ADR atm. Single binary syncs will cover upgrades, we are not using any form of halt height. We want to use some signaling mechanism, but don't know what that will look like.

perhaps after we add more detail on the signalling mechanism this will be worth merging.

@rootulp rootulp added this to the Post-mainnet milestone Jul 12, 2023
@MSevey MSevey removed their request for review July 24, 2023 13:03
@cmwaters cmwaters self-assigned this Aug 29, 2023
@codecov-commenter
Copy link

codecov-commenter commented Aug 30, 2023

Codecov Report

Merging #1562 (b008e3b) into main (a0a6e4b) will decrease coverage by 0.04%.
Report is 29 commits behind head on main.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1562      +/-   ##
==========================================
- Coverage   20.65%   20.61%   -0.04%     
==========================================
  Files         131      131              
  Lines       15241    15270      +29     
==========================================
  Hits         3148     3148              
- Misses      11791    11820      +29     
  Partials      302      302              

see 5 files with indirect coverage changes

@cmwaters cmwaters marked this pull request as ready for review August 30, 2023 12:39
@cmwaters cmwaters changed the title docs: ADR 018 Social Upgrades docs: ADR 018 Network Upgrades Aug 30, 2023
@cmwaters
Copy link
Contributor

Have updated this PR and it's ready for review now. I think the major remaining point of discussion is on phase 2: https://github.com/celestiaorg/celestia-app/blob/evan/adr-018-social-upgrades/docs/architecture/adr-018-network-upgrades.md#phase-2-signaled-upgrade-height

docs/architecture/adr-018-network-upgrades.md Outdated Show resolved Hide resolved

Given this, a node can at any time spin up a v2 binary which will immediately be able to continue validating and executing v1 blocks as if it were a v1 machine.

The mechanism that dictates which versioned block to agree upon, begins with the app in `EndBlock` of the previous height. There, as a `VersionParams`, the application indicates the version they expect the network to have agreed upon. The proposer of the following height then proposes a block with the new app version. If a validator has the same app version and everything else is correct, they will vote for it else if they are on a different version they will PREVOTE and PRECOMMIT nil, signalling to move to the next round. If less than 2/3+ validators have upgraded, the network will be unable to reach consensus. If the upgrade has failed, then the validators that upgraded can simply downgrade and continue to produce blocks on the original version (even if they are still running the binary of the latest version).
Copy link
Collaborator

Choose a reason for hiding this comment

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

[question] do validators need to explicitly downgrade or can the mechanism fallback to producing blocks of version v1 even though a v2 binary is used.

Asking b/c if validators need to explicitly downgrade, then I don't see what incentive they have to upgrade ahead of time b/c their validator will fail to produce blocks until 2/3 of the network upgrades.

Copy link
Contributor

Choose a reason for hiding this comment

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

They are not changing the binary but are just accepting and producing blocks of the prior version

Copy link
Contributor

Choose a reason for hiding this comment

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

It could be possible to implement something whereby a node produces v2 blocks after a height but still accepts v1 blocks. I'm not sure if this is a good idea. I haven't thought it through yet

The following decisions have been made:

1. All upgrades (barring social hard forks) are to be rolling upgrades. That is node operators will be able to restart their node ahead of the upgrade height. The node will continue running the current version of the upgrade but will be capable of validating and executing transactions of the version being upgraded to. This makes sense given the decision to have single binary syncs (i.e. support for all prior versions). As validators are likely to be running nodes all around the world, it reduces the burden of coordinating a single time for all operators to be online. It also reduces the likelihood of failed upgrade and automates the process meaning generally less downtime between versions.
2. Upgrade coordination will be rolled out in two phases. The first (v1 -> v2) will rely on a configured height to move from one version to the next. The binary will be released with a default height which can be modified later by validators in the event that it needs to be pushed back (or forward). The second phase (v2 -> v3) will use a signalling mechanism whereby validators who are now running on the latest binary will signal that they are ready to shift to the next version.
Copy link
Collaborator

Choose a reason for hiding this comment

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

in the event that it needs to be pushed back (or forward)

For the scenario above, I can think of two resolutions:

Option A: Maintainers of celestia-app modify the default config of upgrade heights. Cut a new release. Ask validators to upgrade.
Option B: Ask validators to manually configured the file of upgrade heights.

I expect Option A to be easier to execute so I'm currently of the opinion that the config file doesn't need to be exposed and can be hard-coded in the binary. Out of curiosity, are there examples of blockchains that have successfully maintain a configurable file for: block height -> version?

Copy link
Contributor

Choose a reason for hiding this comment

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

You're basically saying that it's easier to cut a new release with the new height then to get validators to manually change the default height in their config?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yep, that's what I predict.

Copy link
Contributor

Choose a reason for hiding this comment

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

Keep in mind every single time we'd cut a new release with a different height would constitute a breaking change and a major bump to the version.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could add a CLI command to modify the upgrade height. That may be more convenient for people than downloading a new binary

docs/architecture/adr-018-network-upgrades.md Outdated Show resolved Hide resolved
docs/architecture/adr-018-network-upgrades.md Outdated Show resolved Hide resolved
Copy link
Member Author

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

I can't approve cause I originally opened, but I hereby approve

thanks for cleaning this up @cmwaters

docs/architecture/adr-018-network-upgrades.md Outdated Show resolved Hide resolved
docs/architecture/adr-018-network-upgrades.md Outdated Show resolved Hide resolved
@celestia-bot celestia-bot requested a review from a team September 13, 2023 11:39
cmwaters
cmwaters previously approved these changes Sep 13, 2023
@celestia-bot celestia-bot requested a review from a team September 14, 2023 09:51
Copy link
Member Author

@evan-forbes evan-forbes left a comment

Choose a reason for hiding this comment

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

my re-approve ✔️ lol

@rootulp rootulp requested a review from cmwaters September 15, 2023 16:14
@cmwaters cmwaters merged commit b59e876 into main Sep 15, 2023
@cmwaters cmwaters deleted the evan/adr-018-social-upgrades branch September 15, 2023 17:01
@staheri14 staheri14 mentioned this pull request Nov 14, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ADR item is directly relevant to writing or modifying an ADR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants