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

Wallet_create on the command line doesn't work with a running daemon #458

Closed
clemahieu opened this issue Jan 9, 2018 · 18 comments

Comments

Projects
10 participants
@clemahieu
Copy link
Collaborator

commented Jan 9, 2018

The database entry is created but the items map in memory isn't updated when the file changes.

The items map in memory could be removed and just always check the database for wallet IDs.

@androm3da androm3da added the bug label Jan 9, 2018

@brightmonkey

This comment has been minimized.

Copy link

commented Jan 31, 2018

I was going to take a stab at fixing this, but as far as i can tell wallet_create seems to work. What are the steps I need to do to reproduce the symptoms you see?

@clemahieu

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 31, 2018

I think the issue is if the node is running "rai_node --daemon" and someone runs "rai_node --wallet_create" on the command line, they expect that to be effective in the running node but it's not because the map in rai::wallets isn't updated with the new wallet id.

@brightmonkey

This comment has been minimized.

Copy link

commented Feb 1, 2018

Isn't that the equivalent of running two instances of the node? If so, how would node B influence/change the properties of node A running the daemon? Furthermore, if someone wants to add a wallet to node A, why wouldn't they use the RPC command?

Edit - or is this a case where node B is supposed to update data.ldb and node A is supposed to pick up the change?

@brightmonkey

This comment has been minimized.

Copy link

commented Feb 1, 2018

Never mind, I think I answered my own question. Looks like the expectation is that node B updates the database and node A is unaware of the change. I'll look into removing the items map and checking the database for wallet IDs instead.

@brightmonkey

This comment has been minimized.

Copy link

commented Feb 4, 2018

@clemahieu the majority of command line options don't respect the --data_path flag. Was this a specific design choice? If not I'd like to create and submit a change that allows commands like ./rai_node --wallet_list to use the path defined by --data_path if that's ok with you. The rationale for this is to keep my debug environment entirely separate from the default installation path used by my personal desktop wallet.

@cryptocode

This comment has been minimized.

Copy link
Collaborator

commented Feb 4, 2018

@brightmonkey there's a PR fixing that: #342

@PlasmaPower

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2018

There was a PR to fix that, but it got abandoned.

@brightmonkey

This comment has been minimized.

Copy link

commented Feb 4, 2018

@cryptocode thanks!
@PlasmaPower, are you talking about #342 or a different PR?

@PlasmaPower

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2018

Same PR.

@brightmonkey

This comment has been minimized.

Copy link

commented Feb 4, 2018

Looks like it's still open, although it needs a rebase. @SergiySW, is #342 still active?

@SergiySW

This comment has been minimized.

Copy link
Collaborator

commented Feb 4, 2018

Updated for recent version #586

@brightmonkey

This comment has been minimized.

Copy link

commented Feb 7, 2018

@clemahieu there are ~180 places in the code that reference the wallets.items map. Were you thinking of replacing all of them with direct db reads? I don't know what impact removing the map entirely would have on performance, but I would think accessing a map in memory would always be faster than file IO access to a database on disk.

As an alternative, would it be reasonable to add an RPC command called "wallet_refresh" that updates the wallets.items map from the database? A user could call "rai_node --wallet_create" to create the new wallet, then make an RPC call to "wallet_refresh" to have the running node pick up the new wallet and refresh the map. Thoughts?

@cclafferty

This comment has been minimized.

Copy link

commented Mar 22, 2018

I've found this exact issue when attempting to run a representative node. I must admit it took me a long time to find the problem. Most installation guides instruct to run ./rai_node --wallet_create which is creating a wallet the daemon knows nothing about. I would hazard a guess that most representative nodes are not voting because of this, as it's easier to miss. See nanode.co for quite a large list of offline representatives.

If this bug looks too involved to fix right now perhaps directing people to use the RPC methods for creating wallets would be best? Alternatively inform them they must restart the daemon when using the command line?

@rkeene

This comment has been minimized.

Copy link
Contributor

commented Aug 23, 2018

We can also have methods that change the database in ways the daemon would need to be notified of fail if the daemon is running.

@argakiig

This comment has been minimized.

Copy link
Collaborator

commented Sep 22, 2018

So, to understand are we wanting to block CLI calls that would modify the database if an instance of the daemon or wallet is running?

@rkeene

This comment has been minimized.

Copy link
Contributor

commented Sep 22, 2018

Not block, but fail immediately

@rkeene rkeene assigned SergiySW and unassigned argakiig Dec 20, 2018

@zhyatt zhyatt added this to Unscheduled in V18 Dec 27, 2018

@SergiySW SergiySW moved this from Unscheduled to CP 1 (2018-01-09) in V18 Dec 28, 2018

@SergiySW SergiySW moved this from CP 1 (2018-01-09) to CP 2 (2018-01-16) in V18 Dec 28, 2018

@SergiySW SergiySW moved this from CP 2 (2018-01-16) to CP 3 (2018-01-23) in V18 Dec 28, 2018

@SergiySW

This comment has been minimized.

Copy link
Collaborator

commented Jan 26, 2019

Running daemon will update wallets list each 5 minutes in version 18.0 #1619

@zhyatt

This comment has been minimized.

Copy link
Collaborator

commented Jan 30, 2019

Resolved in #1619

@zhyatt zhyatt closed this Jan 30, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.