feat: mine new overlay #4685
feat: mine new overlay #4685
Conversation
There was a problem hiding this comment.
in order to achieve overlay change during runtime, i think you have to save the previous target-neighborhood in the statestore and only nuke the dirs (and change the overlay) if the target-neighborhood of the config option is different from the statestore one.
Couldn't you just check the saved overlay (if there is one) to see if it is already in the specified target neighborhood? |
You can also do this, yes. |
|
Thanks for the comments regarding the missed use-case when user reloads the client with the mining param. I corrected that and it also sets |
pkg/node/node.go
Outdated
| const ( | ||
| localstore = "localstore" | ||
| kademlia = "kademlia-metrics" | ||
| statestore = "statestore" |
There was a problem hiding this comment.
TLDR The cleanest way is instead to remove directories, it would be better for these services to implement its own internal nuke methods (that handle file descriptors and internal storage reinitalizations) and call them here, instead to remove their files.
Statestore directory is removed, but a few lines bellow statestore is passed to the setOverlay function. I would assume that the statestore is no longer reliable after its files are removed. Special attention should be made to ensure when interfering with storage internals outside of the services that manage their state. In this case, statestore must be reinitialised (and passed again to all already initialized services that use statestore) or if possible initilizad after the data on disk is removed. Handling nuke on node startup is different than in a separate nuke cli command as the node continues to run.
Nuking buy removing data directories is a questionable operation as these paths are constructed locally by assumption that they will not change in other places of the code where services that store data in them are initialized. I know that this is a mistake taken from the nuke cli command, but it would be good to make new code better.
There was a problem hiding this comment.
good point, I reinited statestore after the wipe as it is needed to fetch nonce before wipe.
Regarding the nuking, no service uses these databases before this point, all affected DB initializations happen in this function. It should have a separate clearDb method with options for sure if some of the data was required to be kept (please let me know if there is any).
so basically, this is the only place where databases should managed as files, shouldn't it?
There was a problem hiding this comment.
Thank you for fixing it.
The statestore as a general key/value store, is a tricky to clean. I believe that this is a good time to plan something like a consolidation of all keys (or key prefixes) that are stored. It would be good to remove only the required keys.
I would say that it would even better to have a Nuke function on the databases to cleanup internal storage, in this case leveldb and not to worry about the file management. This would require a few more changes, but I believe that would result in more reliable code, not to worry about the files, just databases as abstraction. This can be an improvement later.
There was a problem hiding this comment.
the statestore actually has a nuke function but it preserves keys related to maintaing the overlay addresses, it may be improved to remove those entries when called upon.
There was a problem hiding this comment.
It is much better to adapt StateStoreAdapter.Nuke for cleaning. I think that it would be the safer way, and maybe to have the list of preserved prefixes a bit more transparent.
pkg/node/node.go
Outdated
| time.Sleep(10 * time.Second) | ||
|
|
||
| const ( | ||
| localstore = "localstore" |
There was a problem hiding this comment.
Have seen this constants in some other places to. Shouldn't we move them in some constants file , or maybe in the util package so we are not hardcoding them every time?
There was a problem hiding this comment.
yes, reasonable to make constants for that. I moved localstorage and kademlia path constants to ioutil within utils package.
nugaon
force-pushed
the
feat/mine-overlay
branch
from
c7db1f8
to
bd0f590
Compare
|
|
||
| // DB folders paths from bee datadir | ||
| const ( | ||
| DataPathLocalstore = "localstore" |
There was a problem hiding this comment.
it does not feel right to put these consts here but i don't know a better place
Allowing the overlay address mining even when nonce exists. That case,
localstoreandkademlia-metricsfolders will be removed andstatestorewill be cleared to start a clean state in the new neighborhood. Before the wipe, 10 seconds wait time allows the user to change their mind.Checklist
Description
Open API Spec Version Changes (if applicable)
Motivation and Context (Optional)
Related Issue (Optional)
Screenshots (if appropriate):