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.

Sign up for GitHub

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

Jump to bottom

feat(gnovm)!: store refactor #2655

Closed
wants to merge 11 commits into from
Closed

feat(gnovm)!: store refactor #2655

wants to merge 11 commits into from

Conversation

thehowl
Copy link
Member

@thehowl thehowl commented Aug 2, 2024

This draft PR introduces a re-organized Store interface, with a few goals as compared to the current Store implementation:

  • Split the Store interface into smaller interfaces, and replace usages of the Store interface in the GnoVM codebase to only use the subset they need.
    • This has the main advantage of simplifying unit testing and mocking; allowing to create smaller implementations for what needs to be tested.
    • But furthermore, it makes explicit the extent to which a given function will attempt to access the Store.
  • Split the functionality of existing stores into wrappers with the philosophy of "do one thing and do it well", and allow composition of different stores depending on necessity.
    • As an example, an ObjectStore will eventually be 3 store implementations stacked together:
      1. A "reference manager" store, which when marshaling changes "compacts" many values to RefValues, and many types to RefTypes; and when unmarshaling expands RefTypes to their corresponding types. (This is done currently by functions like copyValueWithRefs / fillTypesOfValue).
      2. A "cache" store, which holds the result of unmarshaling values from the Store, and of new value
      3. The underlying ObjectStore, which retrieves and sets data in the tendermint 2 store.
    • Another type of store will be the "transaction log" store, which would exist for the current cacheTypes and cacheNodes: it keeps the values to be written, until Write() is called and the values are committed to their underlying "cache" maps.
  • Remove any methods which could be functions or options; or other specific implementations
    • For instance, now that PackageGetter is never used on-chain, the Store itself does not need to have a PackageGetter; the TestStore in gnovm/tests can make its own implementation which does the on-the-fly loading of packages and standard libraries.
    • In another instance, instead of setting NativeResolver and PackageInjector we can now set them at startup using StoreOptions - seeing as in our practical usages these are initialised once then never again
  • Ensure that the Store interfaces can be better adapted for implementations in non-blockchain contexts, rather than trying to fit defaultStore for all use-cases.

More development and notes will follow. For now, let's try to merge #2319 as a stepping stone first.

@github-actions github-actions bot added the 📦 🤖 gnovm Issues or PRs gnovm related label Aug 2, 2024
@thehowl thehowl changed the title begin store refactor feat(gnovm)!: store refactor Aug 2, 2024
@thehowl thehowl mentioned this pull request Aug 2, 2024
Merged
@codecov Codecov
Copy link

codecov bot commented Aug 2, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 35.08772% with 111 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
gnovm/pkg/gnolang/store.go 33.79% 91 Missing and 5 partials ⚠️
tm2/pkg/sdk/options.go 0.00% 8 Missing ⚠️
tm2/pkg/sdk/baseapp.go 0.00% 2 Missing and 2 partials ⚠️
gno.land/pkg/sdk/vm/keeper.go 66.66% 2 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@ajnavarro ajnavarro self-requested a review August 27, 2024 09:51
thehowl added a commit that referenced this pull request Sep 5, 2024
@thehowl
fix(gno.land): make gno store cache respect rollbacks (#2319)
c1a3341 
This pull request modifies the current `defaultStore` to support
transactionality in the same way as our tm2 stores. A few new concepts
are introduced:

- A `TransactionStore` interface is added, which extends the
`gnolang.Store` interface to support a Write() method.
- Together with corresponding implementations allowing for
transactionality on its cache maps, this means that the Gno store
retained by the VM keeper is only modified atomically after a
transaction has completed.
- The tm2 `BaseApp` has the new "hooks" `BeginTxHook` and `EndTxHook`.
The former is called right before starting a transaction, and the latter
is called right after finishing it, together with the transaction
result.
- This allows us to plug in the Gno `TransactionalStore` in the
`sdk.Context` through the `BeginTxHook`; and commit the result, if
successful, in the `EndTxHook`.
## Overview of changes

- `gno.land`
- `pkg/gnoland/app.go`: the InitChainer is now additionally responsible
of loading standard libraries. To separate the options related to app
startup globally, and those to genesis, the InitChainer is now a method
of its config struct, `InitChainerConfig`, embedded into the
`AppOptions`.
- `pkg/gnoland/app.go`: `NewAppOptions` is only used in `NewApp`, where
most of its fields were modified, anyway. I replaced it by changing the
`validate()` method to write default values.
- `pkg/gnoland/node_inmemory.go`,
`pkg/integration/testing_integration.go`: these changes were made
necessary to support `gnoland restart` in our txtars, and supporting
fast reloading of standard libraries (`LoadStdlibCached`).
- `pkg/sdk/vm/keeper.go`: removed all the code to load standard
libraries on Initialize, as it's now done in the InitChainer. The hack
introduced in #2568 is no longer necessary as well. Other changes show
how the Gno Store is injected and retrieved from the `sdk.Context`.
- `gnovm/pkg/gnolang/store.go`
- Fork and SwapStores have been removed, in favour of BeginTransaction.
BeginTransaction creates a `TransactionalStore`; the
"transaction-scoped" fields of the defaultStore are swapped with
"transactional" versions (including the allocator, cacheObjects and
cacheTypes/cacheNodes - the latter write to a `txLogMap`).
- ClearObjectCache is still necessary for the case of a transaction with
multiple messages.
- The `Map` interface in `txlog` allows us to have a `txLog` data type
stacked on top of another. This is useful for the cases where we use
`BeginTransaction` in preprocess. (We previously used `Fork`.) See later
in the "open questions" section.
- I added an Iterator method on the `txlog.Map` interface - this will be
compatible with [RangeFunc](https://go.dev/wiki/RangefuncExperiment),
once we switch over to [go1.23](https://go.dev/doc/go1.23).
- `tm2/pkg/sdk`
- As previously mentioned, two hooks were added on the BaseApp to
support injecting application code right before starting a transaction
and then right after ending it; allowing us to inject the code relating
to the Gno.land store while maintaining the modules decoupled
- Other
- `gnovm/pkg/gnolang/machine.go` has a one-line fix for a bug printing
the machine, whereby it would panic if len(blocks) == 0.

## Open questions / notes

- TransactionalStore should be a different implementation altogether;
but decoupling the logic which is used within the stores without doing a
massive copy-and-paste of the entire defaultStore implementation is
non-trivial. See [this
comment](#2319 (comment)),
and [this PR](#2655) for a draft
proposed store refactor, which would render the store more modular and
testable.
- There is an alternative implementation, which in micro-benchmarks
would be somewhat faster, in place of the `txLog`; it's still in
`gnolang/internal/txlog/txlog_test.go` where it also has benchmarks. See
[1347c5f](1347c5f)
for the solution which uses `bufferedTxMap`, without using the
`txlog.Map` interface. The main problem with this approach is that it
does not support stacking bufferedTxMaps, thus there is a different
method to be used in preprocess - `preprocessFork()`.
Base automatically changed from dev/morgan/issue-2283 to master September 5, 2024 10:17
@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 5, 2024

This PR is stale because it has been open 3 months with no activity. Remove stale label or comment or this will be closed in 3 months.

@github-actions github-actions bot added the Stale label Dec 5, 2024
@Gno2D2
Copy link
Collaborator

Gno2D2 commented Dec 5, 2024 •
edited
Loading

I'm a bot that assists the Gno Core team in maintaining this repository. My role is to ensure that contributors understand and follow our guidelines, helping to streamline the development process.

The following requirements must be fulfilled before a pull request can be merged.
Some requirement checks are automated and can be verified by the CI, while others need manual verification by a staff member.

These requirements are defined in this configuration file.

Automated Checks

🔴 Maintainers must be able to edit this pull request
🔴 The pull request head branch must be up-to-date with its base

Manual Checks

No manual checks match this pull request.

Debug
Automated Checks
Maintainers must be able to edit this pull request

If

🟢 Condition met
└── 🟢 On every pull request

Then

🔴 Requirement not satisfied
└── 🔴 Maintainer can modify this pull request
The pull request head branch must be up-to-date with its base

If

🟢 Condition met
└── 🟢 On every pull request

Then

🔴 Requirement not satisfied
└── 🔴 Head branch (dev/morgan/store-refactor) is up to date with base (master): behind by 253 / ahead by 11

@jefft0 jefft0 mentioned this pull request Dec 5, 2024
Open
@github-actions github-actions bot removed the Stale label Dec 6, 2024
@thehowl
Copy link
Member Author

thehowl commented Dec 8, 2024

Let's do this after mainnet

@thehowl thehowl closed this Dec 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ajnavarro ajnavarro Awaiting requested review from ajnavarro

Assignees

@thehowl thehowl

Labels
📦 🤖 gnovm Issues or PRs gnovm related
Projects
💪 Bounties & Worx
Status: Done
🧙‍♂️gno.land core team
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thehowl @Gno2D2