feat(gnovm): enforce formatting of added packages

[moul]

RFC: Should we enforce basic code formatting, verify and return an error for bad formatting, or accept bad formatting?

---

This PR introduces the concept of automatically formatting packages during AddPkg. The advantage is that it's relatively inexpensive since we were already parsing the submitted source code AST to apply some verification.

I believe that having a consistent official formatting for the Go language is one of the things that makes it better, simpler, and more readable. I think Gno should strive to be as good as or better than Go in this regard.

Another justification for this approach is that Gno is not just the language but a platform (gno.land), like a PAAS, and AddPkg can be considered the "CD" process, while auto-formatting could be the "CI" component, which often go hand-in-hand in "CI/CD".

The biggest drawback I can see is that in practice, the code content may not be the same on the publisher's computer and on-chain.

[codecov]

Codecov Report

Attention: Patch coverage is 75.00000% with 4 lines in your changes missing coverage. Please review.

Project coverage is 54.81%. Comparing base (b1c1c96) to head (bb4adeb).

Files	Patch %	Lines
gnovm/pkg/gnolang/go2gno.go	75.00%	2 Missing and 1 partial ⚠️
gno.land/pkg/sdk/vm/keeper.go	75.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2464      +/-   ##
==========================================
- Coverage   55.01%   54.81%   -0.20%     
==========================================
  Files         595      593       -2     
  Lines       79765    81442    +1677     
==========================================
+ Hits        43880    44645     +765     
- Misses      32567    33399     +832     
- Partials     3318     3398      +80     

Flag	Coverage Δ
contribs/gnodev	26.00% <ø> (ø)
contribs/gnofaucet	14.46% <ø> (ø)
contribs/gnokeykc	0.00% <ø> (ø)
contribs/gnomd	0.00% <ø> (ø)
gno.land	64.15% <75.00%> (+0.01%)	⬆️
gnovm	60.20% <75.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[thehowl]

I agree to have this. Gno.land's ability to always view the source is a strong response against the obscurity of bytecode blobs; so fighting obfuscated code by simply making all on-chain code gofmt'd makes sense to me.

However, I'd argue that to have this, we'd probably need to have our internal fork of go/format:

Note that formatting of Go source code changes over time, so tools relying on consistent formatting should execute a specific version of the gofmt binary instead of using this package. That way, the formatting will be stable, and the tools won't need to be recompiled each time gofmt changes.

(from the godoc)

ie., compiling Gno with two different versions of Go could lead to a chain fork if there is differing output from the go/format package. :/

We will need to have automated checks that make sure that a Go version upgrade (or any other dependency upgrade) doesn't break the chain, eventually; but this seems obviously in need to fork, as the package doc itself says not to rely on the output.

[moul]

I agree 👍

@mvertes expressed interest in exploring fuzzing-style methods to measure compatibility across various architectures and versions. We should definitely work on this at some point. However, enforcing specific build (GOVERSION) and runtime (GOOS, GOARCH) constraints within the same BFT network is, in my opinion, a reasonable approach in the meantime.

[thehowl]

I discussed with @moul what to do with this, and agreed that at the end of the day we have the same problem with go/types in the typechecker. So requiring a specific version to run gno.land for now seems reasonable.

In the long run, we can look to have our own forks: #2550

Approving.

[deelawn]

I like it ❤️

[thehowl]

Merging. If someone has a good reason to dislike it, please open a PR to revert/change this.

[kristovatlas]

Going to look at this...

[kristovatlas]

This makes sense to me.

I have a slight bias toward rejecting unformatted code from being deployed vs auto-formatting. This is because any time you transform code, you have the potential for bugs to be exploited -- in particular, for underhanded code. Example: https://blog.azuki.vip/backdooring-js/

That said, after reviewing what gofmt does (indentation and alignment, spacing, line breaks, organizing import statements, comments, braces, consistent structure for method/type decls and function signatures, aligning parameter/return types), I think the opportunities to exploit are fairly low. Also, the source code will be available on-chain to review. Perhaps we should provide some mechanism to normalize identifying discrepancies between original source code and on-chain deployed code? Something that @moul and I have discussed is visual trust metrics for realms, so maybe a non-match between original source code (e.g. hosted on GitHub) and on-chain formatted code would be one of those metrics. Alternatively, something simple to check and reflect would be hash mismatch between original source and deployed code; this could also be used in CI/CD checks for devs writing code in gno.

Given the limited attack surface here, I think the practical considerations in favor of auto-formatting we've discussed outweigh the risks.