-
Notifications
You must be signed in to change notification settings - Fork 33
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
[Persistence][Core][Savepoints/Rollbacks] Implement KISS SavePoints - Serialize WorldState from Persistence #327
Comments
@jessicadaugherty Updated the description. Please review |
@jessicadaugherty Is it worth looking into using See: pgx rollback implementation This way we could have I am not super clear on how another form of rollbacks could be implemented as Postgres doesn't support them outside of transactions (I believe). Unless it were to be a DB incremental backup and restore scenario? |
Hi @h5law ! I have received a notification since the issue is assigned to me so I'll try to answer your question on @jessicadaugherty's behalf :) You might be on the right path on the Postgres side of things but I think it's worth clarifying that savepoints and rollbacks have to also consider type persistenceModule struct {
bus modules.Bus
config *configs.PersistenceConfig
genesisState *genesis.GenesisState
blockStore kvstore.KVStore
txIndexer indexer.TxIndexer
stateTrees *stateTrees
// TECHDEBT: Need to implement context pooling (for writes), timeouts (for read & writes), etc...
writeContext *PostgresContext // only one write context is allowed at a time
} Essentially, we need to implement whatever is necessary so that everything we persist can have Savepoints and Rollback functionality. The whole thing has to work in unison across all the datastores. Any ideas or pointers from your point of view are more than welcome. Please let me know if this makes sense, happy to answer any further questions you might have. |
My knowledge of SMTs is very very limited but from a little reading func (s *SMT) Revert(toOldRoot []byte) error { "When Revert is called, the trees to rollback (between the current tree and toOldRoot) are deleted from the database. This process can be slow so for a quick rollback, manually set the Root to the state you want to revert to and reload the cache." Resetting the root seems to be a fast implementation.
If this is correct I'll try to find out more on each of these and see what comes up. |
To add to what @deblasis said, you're definitely on the write part w.r.t the Postgres piece. In type PersistenceWriteContext interface {
// Context Operations
NewSavePoint([]byte) error
RollbackToSavePoint([]byte) error
Release() error
// Commits the current context (height, hash, transactions, etc...) to disk (i.e. finality).
Commit(proposerAddr, quorumCert []byte) error
// ...
} And the last few lines current implementation of func (p PostgresContext) Commit(proposerAddr, quorumCert []byte) error {
//...
// Commit the SQL transaction
ctx := context.TODO()
if err := p.getTx().Commit(ctx); err != nil {
return err
}
if err := p.conn.Close(ctx); err != nil {
log.Println("[TODO][ERROR] Implement connection pooling. Error when closing DB connecting...", err)
}
return nil
} There's still some work to make the postgres part work correctly (if you navigate the code you'll understand what I mean), but most of the piece are setup to enable that. It's "easy" because we can leverage rollbacks in a SQL db, which we use for For In essence, here's what we need to do (pseudo-code):
The hardest part of what I described above is
I haven't thought through the details here deeply so the above is just where I would start thinking/looking if I were to implement it myself. There are still some open questions and we'll need to properly design this piece. |
Likely need to split into multiple issues:
|
…x for params and flags (#654) ## Description While working on #327 (KV Store PR) I realized that the scope was growing and it sounded sensible to create a specific PR with these fixes that I believe are gonna be needed regardless. ## Issue I didn't bother creating an issue since I guess this constitutes bug/techdebt even if it's necessary work for #327 if we move forward with the KV store approach. ## Type of change Please mark the relevant option(s): - [ ] New feature, functionality or library - [x] Bug fix - [x] Code health or cleanup - [ ] Major breaking change - [ ] Documentation - [ ] Other <!-- add details here if it a different type of change --> ## List of changes - Fixed flag/params keying. It was using the hash of the value and not the name, making queries by name impossible ![image](https://user-images.githubusercontent.com/29378614/230366398-6f56cce2-6183-47b0-a259-6c05f17890c7.png) - Updated pools read/write functions to use a real address, not a string that is named address and obviously is not hexadecimal etc, etc. - Added test to describe behaviour and catch future bugs (we already missed a pool that wasn't added to the FriendlyNames: `Pools_POOLS_FISHERMAN_STAKE`) - Updated runtime and tests to reflect the changes ## Testing - [x] `make develop_test`; if any code changes were made - [x] [Docker Compose LocalNet](https://github.com/pokt-network/pocket/blob/main/docs/development/README.md); if any major functionality was changed or introduced - [x] [k8s LocalNet](https://github.com/pokt-network/pocket/blob/main/build/localnet/README.md); if any infrastructure or configuration changes were made <!-- REMOVE this comment block after following the instructions If you added additional tests or infrastructure, describe it here. Bonus points for images and videos or gifs. --> ## Required Checklist - [x] I have performed a self-review of my own code - [x] I have commented my code, particularly in hard-to-understand areas - [x] I have added, or updated, [`godoc` format comments](https://go.dev/blog/godoc) on touched members (see: [tip.golang.org/doc/comment](https://tip.golang.org/doc/comment)) - [x] I have tested my changes using the available tooling - [x] I have updated the corresponding CHANGELOG ### If Applicable Checklist - [ ] I have updated the corresponding README(s); local and/or global - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] I have added, or updated, [mermaid.js](https://mermaid-js.github.io) diagrams in the corresponding README(s) - [ ] I have added, or updated, documentation and [mermaid.js](https://mermaid-js.github.io) diagrams in `shared/docs/*` if I updated `shared/*`README(s) --------- Signed-off-by: Alessandro De Blasis <alex@deblasis.net> Co-authored-by: Daniel Olshansky <olshansky@pokt.network>
Objective
Revisit the TODOs used as placeholders throughout the persistence module codebase related to save points.
Make sure that the
SavePoint
is created when theUtilityContext
(soon to be renamed toUtilityUnitOfWork
) is instantiated.NOTE: This ticket is dependent on #563
Origin Document
Goals
Persistence
so that we have an in-memory representation of it that can be used for the subsequent deliverables, with particular regard to the Change Tracking part ([Utility][Persistence][Savepoints/Rollbacks] Implement change tracking inUtilityUnitOfWork
to allow ephemeral state and remove side effects #564)Deliverable
Non-goals / Non-deliverables
General issue deliverables
Testing Methodology
make test_all
LocalNet
is still functioning correctly by following the instructions at docs/development/README.mdCreator: @jessicadaugherty - rescope: @deblasis
Co-creator: @Olshansk
The text was updated successfully, but these errors were encountered: