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

Use cosmos-sdk DefaultBaseappOptions instead of replicating #9425

Closed
mhofman opened this issue May 30, 2024 · 2 comments · Fixed by #9563
Closed

Use cosmos-sdk DefaultBaseappOptions instead of replicating #9425

mhofman opened this issue May 30, 2024 · 2 comments · Fixed by #9563
Labels
agoric-cosmos enhancement New feature or request

Comments

@mhofman
Copy link
Member

mhofman commented May 30, 2024

What is the Problem Being Solved?

We recently missed some iavl options that were introduced by an cosmos sdk upgrade because our appCreator didn't add support for these (#9424). Instead of manually adding support for these, we should leverage the cosmos-sdk DefaultBaseappOptions helper in server/util.go.

Description of the Design

Remove custom logic in appCreator.newApp, and adopt DefaultBaseappOptions. Consider adopting in appCreator.appExport as well.

Security Considerations

None

Scaling Considerations

Less manual tracking of upstream changes

Test Plan

TBD

Upgrade Considerations

None

@mhofman
Copy link
Member Author

mhofman commented May 30, 2024

We definitely need some testing to avoid further regressions here.

@gibson042 in #9426 (review)_:

          Wow, that sure is a different kind of architecture we're dealing with. :upside_down_face:

Additional thoughts:

NewAgoricApp is also called by appExport; should that support/send DisableFastNode?

We should have protection against this kind of lapse in the future... maybe a test that imports "github.com/cosmos/cosmos-sdk/server" and uses its functions to see what flags exist:

import (
	"io"
	"os"
	"testing"

	"github.com/cosmos/cosmos-sdk/server"
	servertypes "github.com/cosmos/cosmos-sdk/server/types"
	"github.com/spf13/pflag"
	"github.com/tendermint/tendermint/libs/log"
	dbm "github.com/tendermint/tm-db"

	gaia "github.com/Agoric/agoric-sdk/golang/cosmos/app"
)

type appOptionsFromFn func(string) interface{}

func (fn appOptionsFromFn) Get(name string) interface{} {
	return fn(name)
}

func TestCLIFlags(t *testing.T) {
	expectedFlagNames := map[string]interface{}{
		"abci":                                   "",
		"abci-client-type":                       "",
		"address":                                "",
		"api.address":                            "",
		"api.enable":                             "",
		"api.enabled-unsafe-cors":                "",
		"api.max-open-connections":               "",
		"api.rpc-max-body-bytes":                 "",
		"api.rpc-read-timeout":                   "",
		"api.rpc-write-timeout":                  "",
		"api.swagger":                            "",
		"app-db-backend":                         "",
		"consensus.create_empty_blocks":          "",
		"consensus.create_empty_blocks_interval": "",
		"consensus.double_sign_check_height":     "",
		"cpu-profile":                            "",
		"db_backend":                             "",
		"db_dir":                                 "",
		"fast_sync":                              "",
		"genesis_hash":                           "",
		"grpc-only":                              "",
		"grpc-web.address":                       "",
		"grpc-web.enable":                        "",
		"grpc.address":                           "",
		"grpc.enable":                            "",
		"halt-height":                            "",
		"halt-time":                              "",
		"home":                                   "",
		"iavl-cache-size":                        "",
		"iavl-disable-fastnode":                  "",
		"iavl-lazy-loading":                      "",
		"index-events":                           "",
		"inter-block-cache":                      "",
		"inv-check-period":                       "",
		"min-retain-blocks":                      "",
		"minimum-gas-prices":                     "",
		"moniker":                                "",
		"p2p.external-address":                   "",
		"p2p.laddr":                              "",
		"p2p.persistent_peers":                   "",
		"p2p.pex":                                "",
		"p2p.private_peer_ids":                   "",
		"p2p.seed_mode":                          "",
		"p2p.seeds":                              "",
		"p2p.unconditional_peer_ids":             "",
		"p2p.upnp":                               "",
		"priv_validator_laddr":                   "",
		"proxy_app":                              "",
		"pruning":                                "default",
		"pruning-interval":                       "",
		"pruning-keep-recent":                    "",
		"rpc.grpc_laddr":                         "",
		"rpc.laddr":                              "",
		"rpc.pprof_laddr":                        "",
		"rpc.unsafe":                             "",
		"state-sync.snapshot-interval":           "",
		"state-sync.snapshot-keep-recent":        "",
		"trace":                                  "",
		"trace-store":                            "",
		"transport":                              "",
		"unsafe-skip-upgrades":                   "",
		"with-tendermint":                        "",
	}
	unknownFlagNames := []string{}
	missingFlagNames := map[string]bool{}
	for name, _ := range expectedFlagNames {
		missingFlagNames[name] = true
	}
	readFlag := func(name string) interface{} {
		if defaultValue, found := expectedFlagNames[name]; found {
			delete(missingFlagNames, name)
			return defaultValue
		}
		unknownFlagNames = append(unknownFlagNames, name)
		return nil
	}

	homeDir, err := os.MkdirTemp("", "cosmos-sdk-home")
	if err != nil {
		panic(err)
	}
	defer os.RemoveAll(homeDir)
	dummyAppCreator := func(
		logger log.Logger,
		db dbm.DB,
		traceStore io.Writer,
		appOpts servertypes.AppOptions,
	) servertypes.Application {
		return new(gaia.GaiaApp)
	}
	cmd := server.StartCmd(dummyAppCreator, homeDir)
	flags := cmd.Flags()
	flags.SortFlags = true
	flags.VisitAll(func(flag *pflag.Flag) {
		readFlag(flag.Name)
	})
	readBaseOptions := func() {
		flagsRead := []string{}
		logAndReadFlag := func(name string) interface{} {
			flagsRead = append(flagsRead, name)
			return readFlag(name)
		}
		defer func() {
			if r := recover(); r != nil {
				t.Error("panic after reading flags", flagsRead)
				panic(r)
			}
		}()
		server.DefaultBaseappOptions(appOptionsFromFn(logAndReadFlag))
	}
	readBaseOptions()

	if len(unknownFlagNames) != 0 {
		t.Error(
			"unknown CLI flags in cosmos-sdk; incorporate as needed and update this test",
			unknownFlagNames,
		)
	}
	if len(missingFlagNames) != 0 {
		missing := []string{}
		for name, _ := range missingFlagNames {
			missing = append(missing, name)
		}
		t.Error(
			"expected CLI flags missing from cosmos-sdk; remove from this test",
			missing,
		)
	}
}

mergify bot added a commit that referenced this issue May 31, 2024
closes: #9424

## Description

Plumb the iavl options into the cosmos baseapp.

Copied by comparing to `DefaultBaseappOptions` in cosmos-sdk
`server/util.go`, which we should likely use directly instead (see
#9425), but this is a minimal change we can easily adopt now.

### Security Considerations

None

### Scaling Considerations

None

### Documentation Considerations

None. Change should explicitly be part of the release notes

### Testing Considerations

Unsure about how to test this

### Upgrade Considerations

Chain software, not consensus affecting.
@mhofman
Copy link
Member Author

mhofman commented Jun 22, 2024

NewAgoricApp is also called by appExport; should that support/send DisableFastNode?

My understanding is that this is only needed for operation that write to the DB, but I may be mistaken

@mergify mergify bot closed this as completed in #9563 Jun 23, 2024
@mergify mergify bot closed this as completed in 309c7e1 Jun 23, 2024
mhofman pushed a commit that referenced this issue Jun 23, 2024
closes: #9100 
closes: #9425

## Description
Until we have better layering of the snapshots extensions in cosmos-sdk, this replaces the implementation of the `snapshots export` command to initiate the snapshot creation through our front-running mechanism used for state-sync.

Drive-by cleanup of the root command as well to address #9425

### Security Considerations
None,

### Scaling Considerations
None

### Documentation Considerations
Communicate to validators that we fixed the command and that it's usable as expected with any cosmos chain.

### Testing Considerations
Added an integration test since this only modifies the command line handling

Manually tested with the following
```
cd packages/cosmic-swingset
make scenario2-setup
make scenario2-run-chain
# wait until there are blocks, then kill
agd --home t1/n0 snapshots export
rm -rf t1/n0/data/application.db t1/n0/data/agoric
# the above removes app state without removing cometbft state as that cannot
# be restored easily in a single node chain
agd --home t1/n0 snapshot restore $snapshot_height 2
make scenario2-run-chain
# verify blocks are being produced
```

### Upgrade Considerations
Since we replace the upstream sdk command handling, any future changes upstream would have to be reflected in the override.
mhofman pushed a commit that referenced this issue Jun 23, 2024
closes: #9424

## Description

Plumb the iavl options into the cosmos baseapp.

Copied by comparing to `DefaultBaseappOptions` in cosmos-sdk
`server/util.go`, which we should likely use directly instead (see
#9425), but this is a minimal change we can easily adopt now.

### Security Considerations

None

### Scaling Considerations

None

### Documentation Considerations

None. Change should explicitly be part of the release notes

### Testing Considerations

Unsure about how to test this

### Upgrade Considerations

Chain software, not consensus affecting.
mhofman pushed a commit that referenced this issue Jun 23, 2024
closes: #9100
closes: #9425

Until we have better layering of the snapshots extensions in cosmos-sdk, this replaces the implementation of the `snapshots export` command to initiate the snapshot creation through our front-running mechanism used for state-sync.

Drive-by cleanup of the root command as well to address #9425

None,

None

Communicate to validators that we fixed the command and that it's usable as expected with any cosmos chain.

Added an integration test since this only modifies the command line handling

Manually tested with the following
```
cd packages/cosmic-swingset
make scenario2-setup
make scenario2-run-chain
agd --home t1/n0 snapshots export
rm -rf t1/n0/data/application.db t1/n0/data/agoric
agd --home t1/n0 snapshot restore $snapshot_height 2
make scenario2-run-chain
```

Since we replace the upstream sdk command handling, any future changes upstream would have to be reflected in the override.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
agoric-cosmos enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant