pokt-network / pocket

Official implementation of the Pocket Network Protocol v1
https://pokt.network
MIT License
61 stars 33 forks source link

[Persistence] Adds atomic Update for TreeStore #861

Closed dylanlott closed 11 months ago

dylanlott commented 1 year ago

Description

Adds atomic savepoint and rollback creation to the TreeStore

Issue

Relates to #562

Type of change

Please mark the relevant option(s):

List of changes

Testing

Required Checklist

If Applicable Checklist

dylanlott commented 11 months ago

First off, amazing work πŸš€ I love it <3

To clarify: this PR does not contain the rollbacks? Only the savepoints?

This PR implements the rollback functionality for block application in utility/unit_of_work/module.go.

Left a few comments my only major one is around the save function, I think we should do the importing during the load method as this will be ran less frequently 🀞🏼

I'm going to politely push back on this for now, with the caveat that I added an OPTIMIZE comment, but I do think this is a premature optimization with unclear benefits when weighed against the added Rollback function complexity. This should likely be examined in an ADR that more clearly discusses the trade-offs.

Olshansk commented 11 months ago

@dylanlott Please add me as a reviewer if/when you need additional feedback on this.

h5law commented 11 months ago

I'm going to politely push back on this for now, with the caveat that I added an OPTIMIZE comment, but I do think this is a premature optimization with unclear benefits when weighed against the added Rollback function complexity. This should likely be examined in an ADR that more clearly discusses the trade-offs.

I see your point, I missed the Rollback function in Trees, I got confused between it and the load world state function you had previously discussed with me offline. I think you are right that at a later date we can look into where the imports should go. I will rereview now and approve these as appart from that everything else was addressed πŸ‘πŸΌ