Chore: Node manager refactoring

[moneymanolis]

• This PR changes the keys in the node_manager.nodes dictionary to the node aliases instead of the node names to avoid bugs like we had in Bug: Node details are lost #1968. This PR fixes Bug: Node details are lost #1968
• Bugfix where the default node was created all the time not only when it was not available anymore
• Node manager test coverage improved

[netlify]

✅ Deploy Preview for specter-desktop-docs canceled.

Name	Link
🔨 Latest commit	5119165
🔍 Latest deploy log	https://app.netlify.com/sites/specter-desktop-docs/deploys/637d2eb75603450009e3281f

[moneymanolis]

@relativisticelectron could you review?

[k9ert]

One question for the internal dictionary representation but LGTM as far as i can see.

[relativisticelectron]

@relativisticelectron could you review?

When switching from my working configuration (on PR #1901 ) to this PR, the node details (default.json) are not found and new node json are created .

[moneymanolis]

@relativisticelectron could please check against master, #1901 is a whole different beast.

[relativisticelectron]

@relativisticelectron could please check against master, #1901 is a whole different beast.

• 9a3f92d is the last working commit on master
• de8f5af breaks the node savings on master
• Switching from 9a3f92d (working) to this PR fdf248e loses the node details

[moneymanolis]

@relativisticelectron could you elaborate a bit more on "the node details (default.json) are not found and new node json are created"

What is your setup exactly? How many node jsons do you have etc. Which node jsons are created?
There is also a migration happening across those commit I think. See here: https://github.com/cryptoadvance/specter-desktop/blob/de8f5af619d06fdb8b508dae1c5e54d36f5b3bd3/src/cryptoadvance/specter/util/migrations/migration_0002.py
where this key-value pair is added: "python_class": "cryptoadvance.specter.node.Node"

I'd need to replicate your issue basically :-)

[moneymanolis]

@relativisticelectron could please check against master, #1901 is a whole different beast.

* [9a3f92d](https://github.com/cryptoadvance/specter-desktop/commit/9a3f92d6187e933d2ff47ea5986356fa326cfd9a) is the last working commit on master

* [de8f5af](https://github.com/cryptoadvance/specter-desktop/commit/de8f5af619d06fdb8b508dae1c5e54d36f5b3bd3) breaks the node savings on master

* Switching from [9a3f92d](https://github.com/cryptoadvance/specter-desktop/commit/9a3f92d6187e933d2ff47ea5986356fa326cfd9a) (working) to  this PR  [fdf248e](https://github.com/cryptoadvance/specter-desktop/commit/fdf248ef315e54db0d44ec280ef3461caaadf62f)  loses the node details

I think the reason is by doing this you skip the migration and that it is why you don't have "python_class": "cryptoadvance.specter.node.Node" in your node jsons.

@k9ert just double checking can I assume in this PR that the node-migration has happened? Or do we have to handle the case that it hasn't?

[k9ert]

I can't imagine how the migration gets prevented?! Persistence changing releases are some kind of special beast. At least it makes it easier, to put it into code. migration_0002.py is already in master but we haven't had a release until then. So assume:

• that if the release happens, it'll affect all old nodes and the migration will run on all of them with no obvious way of rolling back.
• Yes, we'll have a more or less comprehensive description to roll back but we need to prepare to roll forward.
• The current state of the migration was buggy (my fault, didn't tested it good enough) and so this is a bugfix. As we haven't released yet, you need to assume that the migration will run on top of this PR if you want to test things. This is especially annoying situation for developers, especially if you test with a lot of different nodes.

@k9ert just double checking can I assume in this PR that the node-migration has happened?
Or do we have to handle the case that it hasn't?

So short: answer: You need to assume that the migration happens after this PR and we shouldn't handle any other case. But i'm not yet sure which issue arises here as you haven't touched the migration-code and it shouldn't matter as much:

• Either the migration ran and the changes are there or
• a developer rolled back to work on an other PR which needs the old structure but in this case he also is suppose to clean up the migrations_data_structure @relativisticelectron

Last line:
https://github.com/cryptoadvance/specter-desktop/blob/master/src/cryptoadvance/specter/util/migrations/migration_0002.py#L32-L47

Node-class migration:
            We introduced the Spectrum Node on an extension. With that, we made the choice of 
            the Node to be instantiated much more flexible. The node.json get a member called
            python_class which is the fully qualified package name of the class the NodeManager
            should instantiate.
            This migrates all the node.json files to the new format.
            Effectively it will:
            * Iterate over all nodes in ~/.specter/nodes/*.json
            * load each node.json and adds the correct python_class 
            * stores it again
            In order to reverse this migration, you do need to reverese the addition of the 
            python_class like this:
            
            cd ~/.specter/nodes
            for file in `ls *.json`; do jq 'del(.python_class)' $file | sponge $file ; done
            cd ..
            jq 'del(.migration_executions[] | select(.migration_id == 2))' migration_data.json | sponge migration_data.json

[k9ert]

@moneymanolis please have a look at my last three commits, here. Replacing the name with alias in the migration-script solved the ever growing node-list for me.

[relativisticelectron]

a developer rolled back to work on an other PR which needs the old structure but in this case he also is suppose to clean up the migrations_data_structure @relativisticelectron

Ok, thanks. Cleaning up the migration_data.json solved it. It seems I had migrations ( 2 and 3) from previous experiments (not master though) in migration_data.json and that prevented the essential migration from happening here.