feat: Cosmos upgrade handler calls swingset #7994

JimLarson · 2023-06-29T21:53:22Z

refs: #7992
closes: #7151

Description

Allow Swingset to perform upgrade-time actions by making a new bridge action that contains the upgrade plan that is called by the Cosmos upgrade handler. The upgrade plan's Info field is a string that by convention contains a JSON-serialized object. We can add additional fields to that object for swingset to use.

Security Considerations

The Swingset upgrade handler runs with significant authority, so it must be written carefully.

Scaling Considerations

The upgrade plan cannot be too large as it must be assayable.

Documentation Considerations

N/A

Testing Considerations

Swingset upgrade handlers must be tested in a full upgrade test.

mhofman

I believe in our discussions we wanted to avoid a bridge message sent during the upgrade handler. This may indeed work, but I'd like to think more about the design (hence the request for changes)

The upgrade plan cannot be too large as it must be assayable.

Could you clarify what you mean?

The Swingset upgrade handler runs with significant authority, so it must be written carefully.

Could you details how this authority is different from other swingset invocations from golang?

mhofman · 2023-06-29T22:17:16Z

golang/cosmos/app/app.go

+func swingsetUpgrade(ctx sdk.Context, plan upgradetypes.Plan, callToController func(ctx sdk.Context, str string) (string, error)) error {
+	action := swingsetUpgradeAction{
+		Type: enactUpgradePlanType,
+		Plan: plan,


Should the whole plan be sent over? The info may contain information unrelated to swingset, maybe it'd be better to reserve a specific field for the swingset module?

The conventional Cosmos use of the Info field is pretty small, just containing a reference to the exact commit. See the "Plan" section of https://bigdipper.live/agoric/proposals/34

If upgrade proposal plans become larger in the future I'd be happy to restrict the information sent to swingset, but for now I don't think there's a need.

Exposing the entire plan is the best choice, IMO.

JimLarson · 2023-06-30T00:13:04Z

By "assayable", I mean that a human voter must be able to look at the raw plan and decide whether it implements the actions described in the prose of the upgrade proposal. For example: https://bigdipper.live/agoric/proposals/34

JimLarson · 2023-06-30T00:18:01Z

The security caution only means to say that we cannot really tell ahead of time what sort of authority an upgrade handler might not need, so they need to be written just as carefully as blockingSend() is.

michaelfig · 2023-07-01T01:04:36Z

I believe in our discussions we wanted to avoid a bridge message sent during the upgrade handler. This may indeed work, but I'd like to think more about the design (hence the request for changes)

I think that this is okay, as long as it can be handled before initializing the kernel. If that's the case, I think it is the best and most flexible design.

michaelfig · 2023-07-01T01:06:22Z

Here is the stack trace from the failed CI:

goroutine 69 [running]:
runtime/debug.Stack()
        /usr/local/go/src/runtime/debug/stack.go:24 +0x65
github.com/tendermint/tendermint/consensus.(*State).receiveRoutine.func2()
        /go/pkg/mod/github.com/agoric-labs/tendermint@v0.34.23-alpha.agoric.3/consensus/state.go:724 +0x4c
panic({0x7f7492c36cc0, 0x7f7493c03530})
        /usr/local/go/src/runtime/panic.go:884 +0x213
github.com/cosmos/cosmos-sdk/x/upgrade.BeginBlocker({{0xc000d7c8f0, 0xd}, 0xc001110d20, {0x7f7492e806d8, 0xc001140810}, {0x7f7492ea1180, 0xc000d84030}, 0xc0011b18c0, {0x7f7492e7ab58, 0xc001064540}, ...}, ...)
        /go/pkg/mod/github.com/agoric-labs/cosmos-sdk@v0.45.11-alpha.agoric.3/x/upgrade/abci.go:38 +0x1014
github.com/cosmos/cosmos-sdk/x/upgrade.AppModule.BeginBlock(...)
        /go/pkg/mod/github.com/agoric-labs/cosmos-sdk@v0.45.11-alpha.agoric.3/x/upgrade/module.go:130
github.com/cosmos/cosmos-sdk/types/module.(*Manager).BeginBlock(_, {{0x7f7492e93ae8, 0xc000132030}, {0x7f7492ea2f60, 0xc001e0f780}, {{0xb, 0x0}, {0xc000c94e00, 0xb}, 0x29b, ...}, ...}, ...)
        /go/pkg/mod/github.com/agoric-labs/cosmos-sdk@v0.45.11-alpha.agoric.3/types/module/module.go:491 +0x1fb
github.com/Agoric/agoric-sdk/golang/cosmos/app.(*GaiaApp).BeginBlocker(...)
        /usr/src/agoric-sdk/golang/cosmos/app/app.go:888
github.com/cosmos/cosmos-sdk/baseapp.(*BaseApp).BeginBlock(_, {{0xc0010bce40, 0x20, 0x20}, {{0xb, 0x0}, {0xc000c94e00, 0xb}, 0x29b, {0xe24203e, ...}, ...}, ...})
        /go/pkg/mod/github.com/agoric-labs/cosmos-sdk@v0.45.11-alpha.agoric.3/baseapp/abci.go:177 +0x97b
github.com/tendermint/tendermint/abci/client.(*committingClient).BeginBlockSync(_, {{0xc0010bce40, 0x20, 0x20}, {{0xb, 0x0}, {0xc000c94e00, 0xb}, 0x29b, {0xe24203e, ...}, ...}, ...})
        /go/pkg/mod/github.com/agoric-labs/tendermint@v0.34.23-alpha.agoric.3/abci/client/committing_client.go:332 +0x1a4
github.com/tendermint/tendermint/proxy.(*appConnConsensus).BeginBlockSync(_, {{0xc0010bce40, 0x20, 0x20}, {{0xb, 0x0}, {0xc000c94e00, 0xb}, 0x29b, {0xe24203e, ...}, ...}, ...})
        /go/pkg/mod/github.com/agoric-labs/tendermint@v0.34.23-alpha.agoric.3/proxy/app_conn.go:81 +0x55
github.com/tendermint/tendermint/state.execBlockOnProxyApp({0x7f7492e94990?, 0xc000d7ff80}, {0x7f7492e9b340, 0xc001208930}, 0xc0000e7c20, {0x7f7492ea2558, 0xc002366018}, 0x29a?)
        /go/pkg/mod/github.com/agoric-labs/tendermint@v0.34.23-alpha.agoric.3/state/execution.go:307 +0x51d
github.com/tendermint/tendermint/state.(*BlockExecutor).ApplyBlock(_, {{{0xb, 0x0}, {0xc002c18c88, 0x8}}, {0xc002c18ca0, 0xb}, 0x1, 0x29a, {{0xc002c6af00, ...}, ...}, ...}, ...)
        /go/pkg/mod/github.com/agoric-labs/tendermint@v0.34.23-alpha.agoric.3/state/execution.go:140 +0x171
github.com/tendermint/tendermint/consensus.(*State).finalizeCommit(0xc001055c00, 0x29b)
        /go/pkg/mod/github.com/agoric-labs/tendermint@v0.34.23-alpha.agoric.3/consensus/state.go:1654 +0xafd
github.com/tendermint/tendermint/consensus.(*State).tryFinalizeCommit(0xc001055c00, 0x29b)
        /go/pkg/mod/github.com/agoric-labs/tendermint@v0.34.23-alpha.agoric.3/consensus/state.go:1563 +0x2ff
github.com/tendermint/tendermint/consensus.(*State).enterCommit.func1()
        /go/pkg/mod/github.com/agoric-labs/tendermint@v0.34.23-alpha.agoric.3/consensus/state.go:1498 +0x94
github.com/tendermint/tendermint/consensus.(*State).enterCommit(0xc001055c00, 0x29b, 0x0)
        /go/pkg/mod/github.com/agoric-labs/tendermint@v0.34.23-alpha.agoric.3/consensus/state.go:1536 +0xccf
github.com/tendermint/tendermint/consensus.(*State).addVote(0xc001055c00, 0xc000f35e00, {0x0, 0x0})
        /go/pkg/mod/github.com/agoric-labs/tendermint@v0.34.23-alpha.agoric.3/consensus/state.go:2150 +0x18dc
github.com/tendermint/tendermint/consensus.(*State).tryAddVote(0xc001055c00, 0xc000f35e00, {0x0?, 0x7f7490dbdfd4?})
        /go/pkg/mod/github.com/agoric-labs/tendermint@v0.34.23-alpha.agoric.3/consensus/state.go:1948 +0x2c
github.com/tendermint/tendermint/consensus.(*State).handleMsg(0xc001055c00, {{0x7f7492e7ba98, 0xc00012d620}, {0x0, 0x0}})
        /go/pkg/mod/github.com/agoric-labs/tendermint@v0.34.23-alpha.agoric.3/consensus/state.go:853 +0x3ff
github.com/tendermint/tendermint/consensus.(*State).receiveRoutine(0xc001055c00, 0x0)
        /go/pkg/mod/github.com/agoric-labs/tendermint@v0.34.23-alpha.agoric.3/consensus/state.go:780 +0x4dc
created by github.com/tendermint/tendermint/consensus.(*State).OnStart
        /go/pkg/mod/github.com/agoric-labs/tendermint@v0.34.23-alpha.agoric.3/consensus/state.go:379 +0x12d

michaelfig

I like the design, but something is wrong with the implementation causing a crash.

michaelfig · 2023-07-01T00:59:56Z

golang/cosmos/app/app.go

+func swingsetUpgrade(ctx sdk.Context, plan upgradetypes.Plan, callToController func(ctx sdk.Context, str string) (string, error)) error {
+	action := swingsetUpgradeAction{
+		Type: enactUpgradePlanType,
+		Plan: plan,


Exposing the entire plan is the best choice, IMO.

michaelfig · 2023-07-01T01:00:33Z

golang/cosmos/app/app.go

-		// business.
-		stdlog.Println("Rebooting SwingSet")
-		return mvm, swingset.BootSwingset(ctx, app.SwingSetKeeper)
+		// Lastly, let Swingset reaction to the upgrade


Suggested change

// Lastly, let Swingset reaction to the upgrade

// Lastly, let SwingSet react to the upgrade

JimLarson · 2023-07-11T08:03:17Z

The previous failure doesn't recur. I think Raph's fix did it.

Oh, I forgot to give a shout-out to @raphdev for #7996.

mhofman · 2023-07-11T13:19:36Z

golang/cosmos/app/app.go

@@ -853,6 +836,7 @@ func (app *GaiaApp) MustInitController(ctx sdk.Context) {
 		VibcPort:    app.vibcPort,
 		VbankPort:   app.vbankPort,
 		LienPort:    app.lienPort,
+		UpgradePlan: app.upgradePlan,


Interesting, placing the upgradePlan in the init message actually makes sense. I have plans to refactor the toSwingSet in chain-main.js which would make this a lot easier and straightforward to handle too!

mhofman · 2023-07-11T13:34:09Z

I believe in our discussions we wanted to avoid a bridge message sent during the upgrade handler. This may indeed work, but I'd like to think more about the design (hence the request for changes)

I think that this is okay, as long as it can be handled before initializing the kernel. If that's the case, I think it is the best and most flexible design.

As we discussed offline, the ability to handle before initializing the kernel was the sticking point if we were going to take the blocking send approach. The PR now allows us to do this, in a clever way by adding the upgrade plan to the init message.

Note for future self: the JS side of cosmic-swingset will need to be careful about hangover inconsistency. It's possible for JS to perform its upgrade steps, commit the swing-store, then for the cosmos side to fail and not commit the cosmos DB. On restart it would result in the init message containing the upgrade plan again. cosmic-swingset needs to detect this case and skip the upgrade work (similarly to how chain sends are simply replayed for normal block processing), at least until we have a better story about swing-store rollbacks (#7963).

@JimLarson, to clarify, the upgrade plan includes the upgrade name, so the cosmic-swingset JS side can use that info if needed?

JimLarson · 2023-07-11T17:30:26Z

Correct - the plan contains the upgrade name. Note that the upgrade Info field will contain a JSON-serialized object which can contain interesting stuff - but it must be explicitly unpacked.

feat: Cosmos upgrade handler calls swingset

feat: Cosmos upgrade handler calls swingset

6757303

JimLarson added enhancement New feature or request cosmic-swingset package: cosmic-swingset agoric-cosmos labels Jun 29, 2023

JimLarson requested a review from michaelfig June 29, 2023 21:53

JimLarson self-assigned this Jun 29, 2023

mhofman requested changes Jun 29, 2023

View reviewed changes

raphdev mentioned this pull request Jun 30, 2023

upgrade-test: upgrade to agoric-upgrade-11 #7996

Closed

michaelfig reviewed Jul 1, 2023

View reviewed changes

JimLarson added 3 commits July 10, 2023 23:41

fix: test upgrade to one-past-vaults

e088215

refactor: put upgrade plan in cosmos init action

e84ebf2

refactor: no longer a need for upgrade function to talk over bridge

8d8580a

mhofman reviewed Jul 11, 2023

View reviewed changes

mhofman approved these changes Jul 11, 2023

View reviewed changes

JimLarson added this pull request to the merge queue Jul 12, 2023

JimLarson mentioned this pull request Jul 12, 2023

perform bundle+install during chain-software upgrade #7992

Closed

Merged via the queue into master with commit bcca849 Jul 12, 2023

JimLarson deleted the 7992-enact-upgrade-plan branch July 12, 2023 02:13

JimLarson mentioned this pull request Jul 20, 2023

Implement upgrade-11 handler #7151

Closed

mhofman mentioned this pull request Jul 26, 2023

swingstore schema migration framework #8089

Open

mhofman pushed a commit that referenced this pull request Aug 7, 2023

Merge pull request #7994 from Agoric/7992-enact-upgrade-plan

e2761bb

feat: Cosmos upgrade handler calls swingset

mhofman pushed a commit that referenced this pull request Aug 7, 2023

Merge pull request #7994 from Agoric/7992-enact-upgrade-plan

09a4046

feat: Cosmos upgrade handler calls swingset

mhofman pushed a commit that referenced this pull request Aug 16, 2023

Merge pull request #7994 from Agoric/7992-enact-upgrade-plan

5c6b254

feat: Cosmos upgrade handler calls swingset

mhofman pushed a commit that referenced this pull request Aug 16, 2023

Merge pull request #7994 from Agoric/7992-enact-upgrade-plan

09be0bb

feat: Cosmos upgrade handler calls swingset

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Cosmos upgrade handler calls swingset #7994

feat: Cosmos upgrade handler calls swingset #7994

JimLarson commented Jun 29, 2023 •

edited by ivanlei

Loading

mhofman left a comment

mhofman Jun 29, 2023

JimLarson Jun 30, 2023

michaelfig Jul 1, 2023

JimLarson commented Jun 30, 2023

JimLarson commented Jun 30, 2023

michaelfig commented Jul 1, 2023

michaelfig commented Jul 1, 2023

michaelfig left a comment

michaelfig Jul 1, 2023

michaelfig Jul 1, 2023

JimLarson Jul 11, 2023

JimLarson commented Jul 11, 2023

mhofman Jul 11, 2023

mhofman commented Jul 11, 2023

JimLarson commented Jul 11, 2023

	// Lastly, let Swingset reaction to the upgrade
	// Lastly, let SwingSet react to the upgrade

feat: Cosmos upgrade handler calls swingset #7994

feat: Cosmos upgrade handler calls swingset #7994

Conversation

JimLarson commented Jun 29, 2023 • edited by ivanlei Loading

Description

Security Considerations

Scaling Considerations

Documentation Considerations

Testing Considerations

mhofman left a comment

Choose a reason for hiding this comment

mhofman Jun 29, 2023

Choose a reason for hiding this comment

JimLarson Jun 30, 2023

Choose a reason for hiding this comment

michaelfig Jul 1, 2023

Choose a reason for hiding this comment

JimLarson commented Jun 30, 2023

JimLarson commented Jun 30, 2023

michaelfig commented Jul 1, 2023

michaelfig commented Jul 1, 2023

michaelfig left a comment

Choose a reason for hiding this comment

michaelfig Jul 1, 2023

Choose a reason for hiding this comment

michaelfig Jul 1, 2023

Choose a reason for hiding this comment

JimLarson Jul 11, 2023

Choose a reason for hiding this comment

JimLarson commented Jul 11, 2023

mhofman Jul 11, 2023

Choose a reason for hiding this comment

mhofman commented Jul 11, 2023

JimLarson commented Jul 11, 2023

JimLarson commented Jun 29, 2023 •

edited by ivanlei

Loading