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.

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

WiP: Move NiPoST state into db #5202

Closed
wants to merge 4 commits into from
Closed

Conversation

fasmat
Copy link
Member

@fasmat fasmat commented Oct 26, 2023

Motivation

Closes #5206 - adds local db and moves nipost state into it

This is a prerequisite for making multiple identities possible with the ATX builder. The state that is persisted by the activation.Builder and activation.NIPostBuilder is moved into the DB.

Changes

  • add nipost table
  • add code that migrates existing builder state into DB
    • TODO: execute this code on startup
  • update activation.Builder to use new source for state

Test Plan

  • additional tests for new code
  • existing tests pass

TODO

  • Explain motivation or link existing issue(s)
  • Test changes and document test plan
  • Update documentation as needed
  • Update changelog as needed

@fasmat fasmat self-assigned this Oct 26, 2023
@fasmat fasmat requested review from dshulyak and poszu as code owners October 26, 2023 21:51
@fasmat fasmat marked this pull request as draft October 26, 2023 21:55
@fasmat fasmat force-pushed the 5149-nipost-state-in-db branch from 8e7cf7c to 8221bb1 Compare October 26, 2023 21:56
@fasmat fasmat force-pushed the 5149-nipost-state-in-db branch from 5524478 to 1747cc6 Compare October 27, 2023 00:22
Copy link
Contributor

@dshulyak dshulyak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i don't think that this is a good idea. db stores generic data and can be copied to other machines safely right now. lets please preserve that

i suggest to keep several files. or if you really want, create another sqlite instance to store smesher specific data

@fasmat
Copy link
Member Author

fasmat commented Oct 27, 2023

i don't think that this is a good idea. db stores generic data and can be copied to other machines safely right now. lets please preserve that

i suggest to keep several files. or if you really want, create another sqlite instance to store smesher specific data

The data added to the db is still safe to copy to another machine. The state is stored contains information about the node id and the publish epoch it belongs to -> another node with different identities will first ignore and later prune those when their publish epoch passes. (DELETE FROM nipost where epoch < (current_epoch) - not implemented yet)

The reason why I want to to get rid of the blob data on disk is because:

  • it is stored in the PoST data directory which doesn't exist on nodes that have no supervised post service -> configuration needs to be updated and existing configuration migrated
  • it is less convenient to use files than DB entries - first the directory has to be scanned for files with the node id in their name, then those files need to be scale decoded and only then can be checked if they are still relevant or not

Some of the currently failing tests fail because the state that is loaded from disk hasn't been checked before - e.g. publish fails, time advances to next epoch, node retries and publishes ATX with same nipost again <- should in my opinion not happen, since public epoch passed already. EDIT: this could also be just a bug in my current implementation 😅

@fasmat
Copy link
Member Author

fasmat commented Oct 27, 2023

Superceded by #5207

@fasmat fasmat closed this Oct 27, 2023
@fasmat fasmat deleted the 5149-nipost-state-in-db branch October 27, 2023 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add second database for local state
2 participants