feat!: Implement the version module #1640

evan-forbes · 2023-04-21T04:07:42Z

Overview

This PR adds a very minimal version module, which will update the app version in the header using end block based on a config. The configs are just hardcoded maps atm, but its possible for a user to pass their own via app options. This PR also isn't meant to be a final solution for #1014 or #1568, but more of an exploratory demo of one way they could done.

the majority of #1594

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

codecov-commenter · 2023-04-21T20:48:23Z

Codecov Report

❗ No coverage uploaded for pull request base (v0.13.x@8439399). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             v0.13.x    #1640   +/-   ##
==========================================
  Coverage           ?   49.09%           
==========================================
  Files              ?       84           
  Lines              ?     4532           
  Branches           ?        0           
==========================================
  Hits               ?     2225           
  Misses             ?     2123           
  Partials           ?      184

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

evan-forbes

this was meant as an exploration/hacky module for the incentivized testnet fork, but if we end up liking the mechanism, we can cherry-pick this to main as well.

evan-forbes · 2023-04-21T21:08:03Z

app/end_block.go

+// EndBlock wraps the BaseApp's end block method. This is done to modify the app
+// version if necessary.
+func (app *App) EndBlock(req abci.RequestEndBlock) (res abci.ResponseEndBlock) {
+	res = app.BaseApp.EndBlock(req)
+	ctx := app.GetContextForDeliverTx([]byte{1})
+	return appversion.EndBlocker(ctx, app.VersionKeeper, res)
+}


we have to wrap end block because the base app doesn't actually have a way to modify the app version. It instead relies on the app version being set in BaseApp, and then returned during the Info call on boot. To avoid that, we modify the consensus param updates and pass them here.

I still don't quite get this. Can't you modify the app version in ResponseEndBlock?

Can't you modify the app version in ResponseEndBlock?

that's what we're doing during the version.EndBlocker, but the Baseapp can't modify the app version during endblock afaik

EndBlock
GetConsensusParams

meaning that if we do this in the app's EndBlocker, the version won't actually change

celestia-app/app/app.go

Lines 568 to 571 in 058442d

// EndBlocker application updates every end block

func (app *App) EndBlocker(ctx sdk.Context, req abci.RequestEndBlock) abci.ResponseEndBlock {

return app.mm.EndBlock(ctx, req)

}

Does it get overwritten somewhere later on?

only the events are collected in EndBlock, and GetConsensusParams never checks for version changes.

changing the app version using BaseApp is entirely reliant upon setting the value via SetProtocolVersion, which isn't passed back to tendermint until Info, despite tendermint having the capability to update the version during EndBlock

so yeah its overwritten. nothing happens because only the result of GetConsensusParams is respected, which doesn't check for app vesion

Can't we pass the SetProtocolVersion function to the version module and just call it in the EndBlock :) - Just an idea (It's what the upgrade module does).

BTW, the SDK should really modify the response to hide the fields you can't change to avoid confusion

Ah I think I get what you mean. We have to set the app version in two different areas

evan-forbes · 2023-04-21T21:09:50Z

x/version/abci.go

+	newAppVersion := keeper.GetVersion(ctx)
+	if ctx.BlockHeader().Version.App != newAppVersion {
+		resp.ConsensusParamUpdates.Version = &coretypes.VersionParams{
+			AppVersion: newAppVersion,
+		}
+		// set the version in the application to ensure that tendermint is
+		// passed the correct value upon rebooting
+		keeper.versionSetter.SetProtocolVersion(newAppVersion)
+	}


we have to set the app version in the application, exactly what the upgrade module does, but we also have to update the consensus params in order to avoid having to reboot.

Cherry-pick the following commits to the v0.12.x release branch: 1. ee84e3d 2. 909a566 Will create v0.12.2 after this merges --------- Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>

cmwaters · 2023-04-23T19:38:07Z

A point of clarification. Are we expecting to hardcode the upgrade heights in the binary or get the validators to set them?

Also a design question: why did you choose to have this separate from the modified upgrade module we have

evan-forbes · 2023-04-24T12:57:25Z

@cmwaters note that this is targetting the v0.13.x branch, and is only for incentivized testnet atm. While we might want to reuse some of this for main, this PR is meant to explore what we could do and quickly get a rolling hardfork together.

A point of clarification. Are we expecting to hardcode the upgrade heights in the binary or get the validators to set them?

the validators will eventually set them, although its a good point that we don't yet have that functionality

Also a design question: why did you choose to have this separate from the modified upgrade module we have

the target branch doesn't have a modified upgrade module, so initially I don't think I wanted to wrap or copy paste fork the current upgrade module. This is a good point and we should just stick it in there, should we go to main.

cmwaters · 2023-04-24T14:00:57Z

the validators will eventually set them

If the validators set them in their config then why do we need it to be mapped with the chainID. Can't it just be:

upgrades: { 10, 2 }

rootulp

[no blocking feedback] I was a bit confused by the version map but the tests helped clarify its intended usage

cmd/celestia-appd/cmd/root.go

x/version/READ_ME.md

x/version/README.md

x/version/chains.go

x/version/version_height_mapping_test.go

evan-forbes · 2023-04-25T16:15:04Z

@cmwaters

If the validators set them in their config then why do we need it to be mapped with the chainID. Can't it just be:

sorry, by "validator's setting them" I thought you were referring to signaling, not having a local config. This PR, and therefore the BSR hardfork, will work by the validators choosing a binary that has them preconfigured. I went with the preconfigured approach since its still easy for anyone to change should they formally disagree, but it also attempts to avoid some confusion or miscommunication by requiring a different binary. If users are using the same version/commit of the app, then they will also fork at the same heights.

as I think you were suggesting, since they're preconfigured if we want to support multiple networks without creating multiple releases, then we do need such a mapping.

cmwaters

SGTM.

Excited to try this out

cmwaters · 2023-04-27T13:23:57Z

x/version/version_height_mapping.go

+// getVersion returns the app version for a given block height. It performs a
+// binary search on the ranges slice.
+func getVersion(height int64, ranges []HeightRange) uint64 {
+	// Perform binary search on the ranges slice


lol, good job but I feel this is probably overkill. We might have 10 - 20 versions in our lifetime of this

yeah true good point

cmwaters · 2023-04-27T13:28:54Z

x/version/chains.go

+	return map[string]ChainVersionConfig{
+		MochaChainID:          version0Only,
+		ArabicaChainID:        version0Only,
+		BlockspaceraceChainID: version0Only,


So we should update this when we know the height for the upgrade

cmwaters · 2023-04-27T13:29:20Z

x/version/chains.go

+// ChainVersionConfig. Each hardfork should be added to this map.
+func StandardChainVersions() map[string]ChainVersionConfig {
+	version0Only := NewChainVersionConfig(map[uint64]int64{
+		0: 0,


This is 0 because the current network is set to 0 right?

yeah, hopefully we can stay 1:1 with our major version for simplicity

liamsi

This is pretty cool! Would be curious to get some eyes from the core SDK team on this well.

liamsi · 2023-04-27T13:28:30Z

x/version/README.md

+The state in the version module is abnormal because it can be modified by the
+party building the binary without breaking the app hash. This is because the
+state is not stored in the iavl tree, but rather as a simple predefined golang
+map. It's important to note that even though this state is not in the state


Can you add a sentence where this map is persisted if not in the state? (even if that is explained in more detail below)

evan-forbes · 2023-04-27T13:42:03Z

merging now to not block, but will have a follow up PR for the extra sentence @liamsi

rach-id · 2023-05-03T10:32:44Z

x/version/abci.go

+)
+
+// EndBlocker will modify the app version if the current height is equal to
+// a predefined height at which the app version should be changed.


Late comment to this.

Will this still be activated if more than 2/3rds of the validator set didn't swap the binary before the upgrade height?

nodes that did upgrade will, and nodes that didn't won't. So if 2/3s don't, then they can continue producing blocks while the nodes that did upgrade will halt.

Awesome 👍

evan-forbes added the x/version label Apr 21, 2023

evan-forbes added this to the Mainnet milestone Apr 21, 2023

evan-forbes self-assigned this Apr 21, 2023

evan-forbes changed the base branch from v0.12.x to v0.13.x April 21, 2023 21:13

evan-forbes commented Apr 21, 2023

View reviewed changes

rootulp and others added 8 commits April 21, 2023 16:16

chore: bump NMT to v0.15.0 (#1579)

7fc94f1

Cherry-pick the following commits to the v0.12.x release branch: 1. ee84e3d 2. 909a566 Will create v0.12.2 after this merges --------- Co-authored-by: rene <41963722+renaynay@users.noreply.github.com>

init version module

cafe1aa

chore: test and fix

4ef9166

chore: linter

eec968e

fix: update test

c8146db

docs: add readme plus some minor changes

42bc5bd

chore: update dead link

7096c68

docs: docs

691cd36

evan-forbes force-pushed the evan/itn-rolling-upgrade-sync branch from e155387 to 691cd36 Compare April 21, 2023 21:17

evan-forbes marked this pull request as ready for review April 21, 2023 21:19

evan-forbes requested review from rootulp, mojtaba-esk, cmwaters and rach-id April 21, 2023 21:19

evan-forbes added the itn label Apr 21, 2023

chore: cleanup

999f88d

MSevey requested a review from a team April 24, 2023 13:53

rootulp previously approved these changes Apr 24, 2023

View reviewed changes

docs: typos and more details

e751399

evan-forbes dismissed rootulp’s stale review via e751399 April 25, 2023 18:02

MSevey requested a review from a team April 25, 2023 18:02

rootulp approved these changes Apr 26, 2023

View reviewed changes

cmwaters approved these changes Apr 27, 2023

View reviewed changes

liamsi reviewed Apr 27, 2023

View reviewed changes

evan-forbes merged commit f2a18f4 into v0.13.x Apr 27, 2023

evan-forbes deleted the evan/itn-rolling-upgrade-sync branch April 27, 2023 13:42

rach-id reviewed May 3, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat!: Implement the version module #1640

feat!: Implement the version module #1640

evan-forbes commented Apr 21, 2023 •

edited

Loading

codecov-commenter commented Apr 21, 2023 •

edited

Loading

evan-forbes left a comment

evan-forbes Apr 21, 2023

cmwaters Apr 23, 2023

evan-forbes Apr 24, 2023 •

edited

Loading

evan-forbes Apr 24, 2023 •

edited

Loading

cmwaters Apr 25, 2023

evan-forbes Apr 25, 2023

evan-forbes Apr 25, 2023

cmwaters Apr 27, 2023

cmwaters Apr 27, 2023

evan-forbes Apr 21, 2023

cmwaters commented Apr 23, 2023 •

edited

Loading

evan-forbes commented Apr 24, 2023

cmwaters commented Apr 24, 2023

rootulp left a comment

evan-forbes commented Apr 25, 2023 •

edited

Loading

cmwaters left a comment

cmwaters Apr 27, 2023

evan-forbes Apr 27, 2023

cmwaters Apr 27, 2023

evan-forbes Apr 27, 2023

cmwaters Apr 27, 2023

evan-forbes Apr 27, 2023

liamsi left a comment

liamsi Apr 27, 2023

evan-forbes commented Apr 27, 2023

rach-id May 3, 2023

evan-forbes May 3, 2023

rach-id May 3, 2023

	// EndBlocker application updates every end block
	func (app *App) EndBlocker(ctx sdk.Context, req abci.RequestEndBlock) abci.ResponseEndBlock {
	return app.mm.EndBlock(ctx, req)
	}

feat!: Implement the version module #1640

feat!: Implement the version module #1640

Conversation

evan-forbes commented Apr 21, 2023 • edited Loading

Overview

Checklist

codecov-commenter commented Apr 21, 2023 • edited Loading

Codecov Report

evan-forbes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes Apr 24, 2023 • edited Loading

Choose a reason for hiding this comment

evan-forbes Apr 24, 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

cmwaters commented Apr 23, 2023 • edited Loading

evan-forbes commented Apr 24, 2023

cmwaters commented Apr 24, 2023

rootulp left a comment

Choose a reason for hiding this comment

evan-forbes commented Apr 25, 2023 • edited Loading

cmwaters left a 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

liamsi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes commented Apr 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evan-forbes commented Apr 21, 2023 •

edited

Loading

codecov-commenter commented Apr 21, 2023 •

edited

Loading

evan-forbes Apr 24, 2023 •

edited

Loading

evan-forbes Apr 24, 2023 •

edited

Loading

cmwaters commented Apr 23, 2023 •

edited

Loading

evan-forbes commented Apr 25, 2023 •

edited

Loading