feat(gno.land): add go type checking to keeper + tx simulation in gnokey

[thehowl]

Split from #1695 for ease of reviewing. Merge order:

1. feat(stdlibs): remove support for linkedType in native bindings #1700
2. feat(gno.land): add go type checking to keeper + tx simulation in gnokey #1702 (this one!)
3. feat(transpiler): transpile gno standard libraries #1695
   feat(cmd/gno): perform type checking when calling linter #1730

This PR removes TranspileAndCheckMempkg in favour of performing the type checking it was supposed to do using go/types with a custom importer. This importer works together with Gno's Store, and can as such be used to type check Gno packages without ever writing a single file to disk. It is important to note that by "Go type check" I mean a variety of compile-time checks the Go compiler performs; in fact, this is much more powerful than running "gofmt" as we are currently doing.

Additionally, it adds a new flag to gnokey, -simulate, to control transaction simulation before committing a transaction. See this issue comment

Resolves #1661.

Reviewing notes

• transpiler.TranspileAndCheckMempkg has been removed from the gnokey client and gnoclient, in favour of having this step be performed on the vm keeper. This paves the way for clients to not have to include the entire GnoVM, which I call a win.
• Stdlib io had a precompiling error due to an unused variable (remaining); I updated it to the latest code on Go's standard libraries.
• Store changes
  • Store has been changed to have its getPackage method work by detecting import cycles, without risking race conditions (the current implementation is not thread-safe). This is done by creating a new store, importerStore, which contains the previously imported paths in the current chain. Cyclic imports are still (correctly) detected in the tests.
  • GetMemPackage has been changed to return nil when a package cannot be found. This matches its behaviour with GetMemFile, which already did this when the file does not exist.
  • GetMemPackage, if a package is not found in the store, now attempts retrieving it using Store.GetPackage first. The underlying reason is that the Gno importer for the type checker needs to access the source of the standard libraries; however, these are never in any transaction and are not executed "per se" when the blockchain start. As a consequence, they may not exist within the Store; as a solution, when using GetMemPackage, we ensure that a package does not exist by checking if GetPackage does not retrieve it through getMemPackage and save it.

Questions

• I tried adding a new error type to the VM keeper, which deviates slightly from the other kinds, trying to guess what would be best taking a look at the tm2 errors package: https://github.com/gnolang/gno/pull/1702/files#diff-ec768dd7ea534ac768197ae8c6dce4bda1626d1ab95d2324e9d7c12bcf5066d8
  • Does this make sense?
  • Note that, as far as I can tell, the result of the error is stored on the blockchain together with the transaction. I suspect moving the check to ValidateBasic() would allow us to check that the code is invalid before the tx is added to the mempool (don't take my word on this.) Would that make sense? Related discussion on chore: add more validation to MsgRun & MsgAddPackage #1646.

[codecov]

Codecov Report

Attention: Patch coverage is 67.24138% with 38 lines in your changes are missing coverage. Please review.

Project coverage is 54.18%. Comparing base (dc6eb7d) to head (815c573).

Files	Patch %	Lines
tm2/pkg/crypto/keys/client/maketx.go	52.38%	10 Missing ⚠️
gnovm/pkg/gnolang/store.go	46.66%	7 Missing and 1 partial ⚠️
tm2/pkg/crypto/keys/client/broadcast.go	0.00%	8 Missing ⚠️
gno.land/pkg/sdk/vm/errors.go	54.54%	5 Missing ⚠️
gno.land/pkg/sdk/vm/keeper.go	33.33%	3 Missing and 1 partial ⚠️
gnovm/pkg/gnolang/go2gno.go	95.74%	2 Missing ⚠️
tm2/pkg/crypto/keys/client/query.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1702      +/-   ##
==========================================
- Coverage   54.95%   54.18%   -0.78%     
==========================================
  Files         481      520      +39     
  Lines       67407    73038    +5631     
==========================================
+ Hits        37044    39575    +2531     
- Misses      27341    30245    +2904     
- Partials     3022     3218     +196     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[thehowl]

With the latest commits, this PR additionally changes the behaviour of gnokey to apply what was requested in the review meeting by Jae

Essentially, when performing maketx -broadcast, by default gnokey will now attempt to simulate the transaction first, before executing it. This applies to all transactions with maketx, but the specific behaviour can be changed using the flag -simulate.

• -simulate test, the default, will attempt simulating the transaction, return any error, and if it succeeds execute the transaction normally.
• -simulate skip skips the simulation step, and behaves as the current gnokey client does.
• -simulate only allows performing a "dry run" of the transaction, ie. what is currently possible with gnokey broadcast -dry-run.

cc/ @piux2 @jaekwon @moul for a thumbs up on the behaviour and API changes

cc/ @leohhhn re: docs changes

[thehowl]

From Review Meeting:

• @jaekwon to have a review
• I'll revert the changes to the store and clarify that store isn't meant to be shared by multiple goroutines

[thehowl]

@piux2 I've reverted the changes to Store, but I still modified the functioning of Store's import cycle detection to work using a slice rather than a map.

When I had import cycles happen in real-life scenarios, I found the "dependency path" that Go showed me quite useful, as I could pinpoint not just what package was being recursively imported, but also which other package was importing it.

This is possible only with slices, because maps don't guarantee the order of their keys.

[thehowl]

after a few hours of painful debugging; I remembered the other reason why I'd made the store changes. (cc/ @piux2)

Consider this code:

gno/gno.land/pkg/sdk/vm/builtins.go

Lines 15 to 44 in a03eeb3

	func (vm *VMKeeper) initBuiltinPackagesAndTypes(store gno.Store) {
	// NOTE: native functions/methods added here must be quick operations,
	// or account for gas before operation.
	// TODO: define criteria for inclusion, and solve gas calculations.
	getPackage := func(pkgPath string) (pn *gno.PackageNode, pv *gno.PackageValue) {
	// otherwise, built-in package value.
	// first, load from filepath.
	stdlibPath := filepath.Join(vm.stdlibsDir, pkgPath)
	if !osm.DirExists(stdlibPath) {
	// does not exist.
	return nil, nil
	}
	memPkg := gno.ReadMemPackage(stdlibPath, pkgPath)
	if memPkg.IsEmpty() {
	// no gno files are present, skip this package
	return nil, nil
	}
	m2 := gno.NewMachineWithOptions(gno.MachineOptions{
	PkgPath: "gno.land/r/stdlibs/" + pkgPath,
	// PkgPath: pkgPath,
	Output: os.Stdout,
	Store: store,
	})
	defer m2.Release()
	return m2.RunMemPackage(memPkg, true)
	}
	store.SetPackageGetter(getPackage)
	store.SetNativeStore(stdlibs.NativeStore)
	}

This is used to initialize the VM store:

gno/gno.land/pkg/sdk/vm/keeper.go

Lines 79 to 80 in a03eeb3

	vm.gnoStore = gno.NewStore(alloc, baseSDKStore, iavlSDKStore)
	vm.initBuiltinPackagesAndTypes(vm.gnoStore)

So, what happens in getGnoStore?

gno/gno.land/pkg/sdk/vm/keeper.go

Lines 104 to 128 in a03eeb3

	case sdk.RunTxModeDeliver:
	// swap sdk store of existing store.
	// this is needed due to e.g. gas wrappers.
	baseSDKStore := ctx.Store(vm.baseKey)
	iavlSDKStore := ctx.Store(vm.iavlKey)
	vm.gnoStore.SwapStores(baseSDKStore, iavlSDKStore)
	// clear object cache for every transaction.
	// NOTE: this is inefficient, but simple.
	// in the future, replace with more advanced caching strategy.
	vm.gnoStore.ClearObjectCache()
	return vm.gnoStore
	case sdk.RunTxModeCheck:
	// For query??? XXX Why not RunTxModeQuery?
	simStore := vm.gnoStore.Fork()
	baseSDKStore := ctx.Store(vm.baseKey)
	iavlSDKStore := ctx.Store(vm.iavlKey)
	simStore.SwapStores(baseSDKStore, iavlSDKStore)
	return simStore
	case sdk.RunTxModeSimulate:
	// always make a new store for simulate for isolation.
	simStore := vm.gnoStore.Fork()
	baseSDKStore := ctx.Store(vm.baseKey)
	iavlSDKStore := ctx.Store(vm.iavlKey)
	simStore.SwapStores(baseSDKStore, iavlSDKStore)
	return simStore

The problem is the following: at the time being, when the GnoVM fails to get a package, it will call the package getter... but it will always use the same store (in this case, vm.gnoStore.) Hence, this will ignore whether the store we're currently using is a fork or not.

Thus, this is actually problematic (I just didn't remember it, because it's been a while since I did the changes); when importing packages that don't exist in the store, the GnoVM will load things up from vm.gnoStore, which is only meant to be used directly when actually executing transactions.

This created an issue in the CI: https://github.com/gnolang/gno/actions/runs/9022495035/job/24792161138

The "std" package was attempted to be resolved, but it was only being loaded in the top-level store, and not the fork, thus creating the "import not found".

Which is why we need to change the signature of the PackageGetter, and make sure that it gets the up-to-date store and not pollute the global store.

[thehowl]

@jaekwon as we agreed last thursday, with the goal of unblocking further work depending on this I'm merging this PR

I'm happy to re-discuss any further changes to be made if you still want to make a review

[jefft0]

Hi @thehowl . This PR is a welcome improvement, but it is effectively a breaking change. I am happily going through my realm and test code, fixing compiler errors and trying to get it to load. You may want to warn people.