txtar for test4 cacheTypes issue #2605

thehowl · 2024-07-18T21:57:04Z

Draft, to share txtar for an issue on test4.

There is a new gnoland restart command to try to verify restarting a node. It doesn't seem to work properly in a "successful" scenario (it panics further down the line); but at least for as far as this issue is concerned, it shows the correct stacktrace we are encountering in production.

My theory on the panic: tx1 fails because of an out-of-gas error, creating one of those inconsistencies between cache* fields in the gno.Store and the underlying database. The type gno.land/p/g17ernafy6ctpcz6uepfsq2js8x2vz0wladh5yc3/zentasktic.Collection is stored in cacheTypes, but not in the store.

gno.land/p/g17ernafy6ctpcz6uepfsq2js8x2vz0wladh5yc3/zentasktic was then redeployed, in tx2. This time, it worked; however, when saving the package, SetType is called and is a no-op, because the type is considered as being already in the store (as it exists in the cache).

func (ds *defaultStore) SetType(tt Type) {
	tid := tt.TypeID()
	// return if tid already known.
	if tt2, exists := ds.cacheTypes[tid]; exists {
		if tt != tt2 {
			// this can happen for a variety of reasons.
			// TODO classify them and optimize.
			return
		}
	}

Thus, already running nodes are fine (they have the zentasktic.Collection from the failing transaction, still in the cache). However, when restarting a node, the type exists in neither cacheTypes nor the underlying database; generating the panic.

Ideas for a solution:

fix(gno.land): make gno store cache respect rollbacks #2319 tries to address the underlying problem, and make the GnoVM store "transactional", and rolled back/committed in the same way as the Tm2 stores.
- However, it almost certainly screws up gas calculations; and as such is unsuitable.
One solution that could be compatible with test4 is to use RunFiles in PreprocessAllFilesAndSaveBlockNodes, so we can have the types eventually saved again in the cache/store.
- However, I'm pretty sure this is a perilous path, riddled with other hacks and caveats, both predictable and unpredictable.
- Especially since this still leads to an inconsistent state with a part of the chain: running nodes use a FrankenType (remember: the zentasktic.Collection in cacheTypes comes from a failing tx, not the "real", deployed zentasktic.Collection). Restarted nodes would instead use the correctly "deployed" zentasktic.Collection. Since the types are the same, this might be fine, but I'm not so sure.
One very simple solution, but which probably leads to the fewest further headaches, is to always clean the database and do a catch-up on the network every time a node is restarted.
- Yep, it's ugly, but somehow seems preferable to the other two.

codecov · 2024-07-18T21:59:36Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 55.02%. Comparing base (447b763) to head (bf9a692).
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2605      +/-   ##
==========================================
- Coverage   55.03%   55.02%   -0.02%     
==========================================
  Files         595      595              
  Lines       79665    79644      -21     
==========================================
- Hits        43843    43822      -21     
+ Misses      32509    32508       -1     
- Partials     3313     3314       +1

Flag	Coverage Δ
contribs/gnodev	`26.00% <ø> (ø)`
contribs/gnofaucet	`14.46% <ø> (-0.86%)`	⬇️
contribs/gnokeykc	`0.00% <ø> (ø)`
contribs/gnomd	`0.00% <ø> (ø)`

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.

deelawn · 2024-07-19T01:40:05Z

Nice job diagnosing the issue 💪 . In regards to the solutions, could we maybe do one of the following:

Don't cache types when a package is created
Use a transient cache for types and move them to the regular cache once addpkg completes without issue

I know this won't fix test4, but it seems like a sane solution going forward. I made a short POC implementation here -- https://github.com/gnolang/gno/compare/master...deelawn:gno:fix/transient-type-cache?expand=1

Let me know what you think, or if I'm missing something, why it won't work 😅

thehowl · 2024-07-24T09:18:43Z

Don't cache types when a package is created

Use a transient cache for types and move them to the regular cache once addpkg completes without issue

I think this is not just a problem for cacheTypes, but also for cacheNodes. And, even then, the code in tm2 commits a tx to store when all of its messages succeed; while your approach would instead commit to store on a "per-message" basis. Which is why I think it's better to do it with something like #2319's approach.

deelawn · 2024-07-24T16:43:13Z

Don't cache types when a package is created

Use a transient cache for types and move them to the regular cache once addpkg completes without issue

I think this is not just a problem for cacheTypes, but also for cacheNodes. And, even then, the code in tm2 commits a tx to store when all of its messages succeed; while your approach would instead commit to store on a "per-message" basis. Which is why I think it's better to do it with something like #2319's approach.

Right, thanks. I forgot this is happening for each message, not each tx.

grepsuzette · 2024-07-25T12:56:33Z

I will react, naively, to the 3 solutions, @thehowl.

fix(gno.land): make gno store cache respect rollbacks #2319 tries to address the underlying problem, and make the GnoVM store "transactional", and rolled back/committed in the same way as the Tm2 stores.

However, it almost certainly screws up gas calculations; and as such is unsuitable.

One solution that could be compatible with test4 is to use RunFiles in PreprocessAllFilesAndSaveBlockNodes, so we can have the types eventually saved again in the cache/store.(...) However, I'm pretty sure this is a perilous path

One very simple solution, but which probably leads to the fewest further headaches, is to always clean the database and do a catch-up on the network every time a node is restarted.

Yep, it's ugly, but somehow seems preferable to the other two.

Solution 2. dicey, agree.

The ideal solution if we omit gas calculations just for this sentence seems solution 1, right?

In what way does solution 1 affect gas calculations? Is it like: Malory volontarily fails tx on low gas so next users pay a lot of gas to reload stuffs that were discarded from memory kind of bad?

If so, before ditching solution 1... in #2319, Jae talked about a short-term solution for rollback issues (you explored a different solution in #2504, consisting of preloading all packages at start):

I think the easiest way to solve this problem right now is to change the implementation of the usage of the type checker so that it doesn't call store.GetMemPackage, but only reads the mempackage through ReadMemPackage().

The type checker doesn't care about gno state, I think it only cares about the source code, so there's no need to run "GetMemPackage" which also runs the mempackage.

Couldn't this work and help here?

For the sake of the discussion, here the txtar in this PR (truncated after 80th column as we don't need the sourcecode):

loadpkg gno.land/p/demo/avl
gnoland start

gnokey sign -tx-path $WORK/tx1.tx -chainid tendermint_test -account-sequence 0 t
! gnokey broadcast $WORK/tx1.tx
stderr 'out of gas'

gnokey sign -tx-path $WORK/tx2.tx -chainid tendermint_test -account-sequence 1 t
gnokey broadcast $WORK/tx2.tx
stdout 'OK!'

gnokey sign -tx-path $WORK/tx3.tx -chainid tendermint_test -account-sequence 2 t
gnokey broadcast $WORK/tx3.tx
stdout 'OK!'

gnoland restart

-- tx1.tx --
{
  "msg": [
    {
      "@type": "/vm.m_addpkg",
      "creator": "g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5",
      "package": {
        "name": "zentasktic",
        "path": "gno.land/p/g17ernafy6ctpcz6uepfsq2js8x2vz0wladh5yc3/zentasktic"
        "files": [
          {
            "name": "README.md",
            "body": "# ZenTasktic Core\n\nA basic, minimalisitc Asess-Decide-Do 
          },
          {
            "name": "collections.gno",
            "body": "// base implementation\npackage zentasktic\n\nimport (\n\t\
          },
          {
            "name": "collections_test.gno",
            "body": "package zentasktic\n\nimport (\n\t\"testing\"\n\n    \"gno.
          },
          {
            "name": "contexts.gno",
            "body": "// base implementation\npackage zentasktic\n\nimport (\n\t\
          },
          {
            "name": "contexts_test.gno",
            "body": "package zentasktic\n\nimport (\n\t\"testing\"\n\n    \"gno.
          },
          {
            "name": "core.gno",
            "body": "// base implementation\npackage zentasktic\n\nimport (\n\t\
          },
          {
            "name": "errors.gno",
            "body": "package zentasktic\n\nimport \"errors\"\n\nvar (\n\tErrTask
          },
          {
            "name": "marshals.gno",
            "body": "package zentasktic\n\nimport (\n\t\"bytes\"\n)\n\n\ntype Co
          },
          {
            "name": "projects.gno",
            "body": "// base implementation\npackage zentasktic\n\nimport (\n\t\
          },
          {
            "name": "projects_test.gno",
            "body": "package zentasktic\n\nimport (\n\t\"testing\"\n\n    \"gno.
          },
          {
            "name": "realms.gno",
            "body": "// base implementation\npackage zentasktic\n\nimport (\n\t
          },
          {
            "name": "tasks.gno",
            "body": "package zentasktic\n\nimport (\n\t\"time\"\n\n\t\"gno.land/
          },
          {
            "name": "tasks_test.gno",
            "body": "package zentasktic\n\nimport (\n\t\"testing\"\n\n    \"gno.
          }
        ]
      },
      "deposit": ""
    }
  ],
  "fee": {
    "gas_wanted": "10000000",
    "gas_fee": "1000000ugnot"
  },
  "signatures": [],
  "memo": ""
}

-- tx2.tx --
{
  "msg": [
    {
      "@type": "/vm.m_addpkg",
      "creator": "g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5",
      "package": {
        "name": "zentasktic",
        "path": "gno.land/p/g17ernafy6ctpcz6uepfsq2js8x2vz0wladh5yc3/zentasktic"
        "files": [
          {
            "name": "README.md",
            "body": "# ZenTasktic Core\n\nA basic, minimalisitc Asess-Decide-Do 
          },
          {
            "name": "collections.gno",
            "body": "// base implementation\npackage zentasktic\n\nimport (\n\t
          },
          {
            "name": "collections_test.gno",
            "body": "package zentasktic\n\nimport (\n\t\"testing\"\n\n    \"gno.
          },
          {
            "name": "contexts.gno",
            "body": "// base implementation\npackage zentasktic\n\nimport (\n
          },
          {
            "name": "contexts_test.gno",
            "body": "package zentasktic\n\nimport (\n\t\"testing\"\n\n    \"gno.
          },
          {
            "name": "core.gno",
            "body": "// base implementation\npackage zentasktic\n\nimport (\n\t
          },
          {
            "name": "errors.gno",
            "body": "package zentasktic\n\nimport \"errors\"\n\nvar (\n\tErrTask
          },
          {
            "name": "marshals.gno",
            "body": "package zentasktic\n\nimport (\n\t\"bytes\"\n)\n\n\ntype Co
          },
          {
            "name": "projects.gno",
            "body": "// base implementation\npackage zentasktic\n\nimport (\n\t
          },
          {
            "name": "projects_test.gno",
            "body": "package zentasktic\n\nimport (\n\t\"testing\"\n\n    \"gno.
          },
          {
            "name": "realms.gno",
            "body": "// base implementation\npackage zentasktic\n\nimport (\n\t
          },
          {
            "name": "tasks.gno",
            "body": "package zentasktic\n\nimport (\n\t\"time\"\n\n\t\"gno.land/
          },
          {
            "name": "tasks_test.gno",
            "body": "package zentasktic\n\nimport (\n\t\"testing\"\n\n    \"gno.
          }
        ]
      },
      "deposit": ""
    }
  ],
  "fee": {
    "gas_wanted": "15000000",
    "gas_fee": "1000000ugnot"
  },
  "signatures": [],
  "memo": ""
}

-- tx3.tx --
{
  "msg": [
    {
      "@type": "/vm.m_addpkg",
      "creator": "g1jg8mtutu9khhfwc4nxmuhcpftf0pajdhfvsqf5",
      "package": {
        "name": "zentasktic_core",
        "path": "gno.land/r/g17ernafy6ctpcz6uepfsq2js8x2vz0wladh5yc3/zentasktic_
        "files": [
          {
            "name": "workable.gno",
            "body": "package zentasktic_core\n\ntype Workable interface {\n\t// 
          },
          {
            "name": "wrapper.gno",
            "body": "package zentasktic_core\n\n\nimport (\n\t\"strconv\"\n\n\t
          }
        ]
      },
      "deposit": ""
    }
  ],
  "fee": {
    "gas_wanted": "15000000",
    "gas_fee": "1000000ugnot"
  },
  "signatures": [],
  "memo": ""
}

thehowl · 2024-07-25T14:55:34Z

The ideal solution if we omit gas calculations just for this sentence seems solution 1, right?

In what way does solution 1 affect gas calculations? Is it like: Malory volontarily fails tx on low gas so next users pay a lot of gas to reload stuffs that were discarded from memory kind of bad?

Well, the txtar is actually already a good example of how fixing this bug breaks gas calculations. This is the output:

        > ! gnokey broadcast $WORK/tx1.tx
        [stderr]
        "gnokey" error: --= Error =--
        Data: transaction failed [...]
        log out of gas, gasWanted: 10000000, gasUsed: 11478554 location: WritePerByte
        Msg Traces:
        --= /Error =--
[...]
        > gnokey broadcast $WORK/tx2.tx
        [stdout]

        OK!
        GAS WANTED: 15000000
        GAS USED:   7851998
        HEIGHT:     87
        EVENTS:     []

These two transactions are identical, except for the gas wanted. But the first panics because it exceeds the 10M gas limit. The second one succeeds.. but note that the gas used is now 7851998; ie. less than 10M.

In fact, you get the same bug if instead of having tx1 and tx2, you only use tx1 (but re-sign it to have a different account-number); the first time around it fails, the second time it succeeds.

This is because on the second time there's already data about the objects it's trying to save to the store, in cacheTypes. So... it hits the store less, and consequently it hits the gas counter less (in tm2/pkg/store/gas/store.go).

Keeping test4 as is while fixing the bug and maintaining the same gas used for all replayed transactions is hard. Which is why we're now looking at doing 1 (with #2319) and probably doing a chain reset / new chain.

If so, before ditching solution 1... in #2319, Jae talked about a short-term solution for rollback issues (you explored a different solution in #2504, consisting of preloading all packages at start):

I think the easiest way to solve this problem right now is to change the implementation of the usage of the type checker so that it doesn't call store.GetMemPackage, but only reads the mempackage through ReadMemPackage().
The type checker doesn't care about gno state, I think it only cares about the source code, so there's no need to run "GetMemPackage" which also runs the mempackage.

Couldn't this work and help here?

the changes have already been merged in #2504 and are already part of the test4 binary (the bug is very related, but with #2504 we made it so at least it's not possible that a failing transaction makes a stdlib package "unimportable"). This is another problem, relating to the lack of rollback functionality in cacheTypes (and probably cacheNodes) as well.

thehowl · 2024-08-05T17:47:12Z

Merging to #2319

txtar for test4 issue

bf9a692

github-actions bot assigned thehowl Jul 18, 2024

github-actions bot added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Jul 18, 2024

thehowl mentioned this pull request Jul 22, 2024

fix(gno.land): make gno store cache respect rollbacks #2319

Merged

Kouteki mentioned this pull request Jul 26, 2024

Minutes: Core Staff Weekly Syncs [every Monday] gnolang/meetings#36

Open

michelleellen mentioned this pull request Jul 31, 2024

Minutes: Public Dev Calls [every Wednesday] gnolang/meetings#37

Open

thehowl closed this Aug 5, 2024

thehowl deleted the dev/morgan/cache-types-test4-mess branch September 3, 2024 10:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

txtar for test4 cacheTypes issue #2605

txtar for test4 cacheTypes issue #2605

thehowl commented Jul 18, 2024

codecov bot commented Jul 18, 2024 •

edited

Loading

deelawn commented Jul 19, 2024

thehowl commented Jul 24, 2024

deelawn commented Jul 24, 2024

grepsuzette commented Jul 25, 2024 •

edited

Loading

thehowl commented Jul 25, 2024

thehowl commented Aug 5, 2024

txtar for test4 cacheTypes issue #2605

txtar for test4 cacheTypes issue #2605

Conversation

thehowl commented Jul 18, 2024

codecov bot commented Jul 18, 2024 • edited Loading

Codecov Report

deelawn commented Jul 19, 2024

thehowl commented Jul 24, 2024

deelawn commented Jul 24, 2024

grepsuzette commented Jul 25, 2024 • edited Loading

thehowl commented Jul 25, 2024

thehowl commented Aug 5, 2024

codecov bot commented Jul 18, 2024 •

edited

Loading

grepsuzette commented Jul 25, 2024 •

edited

Loading