monero-ecosystem / monero-python

A comprehensive Python module for handling Monero cryptocurrency
BSD 3-Clause "New" or "Revised" License
247 stars 80 forks source link

Account index out of range #112

Closed maxmqwever closed 2 years ago

maxmqwever commented 2 years ago

Hello I'm not sure if this is a Monero RPC issue, but I noticed that the accounts do not get saved if you force shutdown Monero rpc. This will result in account index out of range if you use account index for users.

Is this a known issue or a better approach would be to create one wallet for each user?

emesik commented 2 years ago

What do you mean by "force shutdown"? Killing the process? In such scenario it cannot be expected from it to have saved all data.

I guess the accounts you had created and not seen after the rpc restart, were clean ones? Or you lost some that already turned funds?

maxmqwever commented 2 years ago

Yes force shutdown, I had this happening hours after the account was first created so I suspect that accounts only get saved once you send the kill signal to monerorpc but I'm not sure.

--

I guess the accounts you had created and not seen after the rpc restart, were clean ones? Or you lost some that already turned funds?

I have only tried on Testnet so far and I have not lost coins

The process looks something like this w=Wallet() User registering a new user and I call w.new_account()

I save the account index number and assign it to the user

I login as the user and get the index out of range since the account index was not saved, I tried to look for rpc options such as refresh but it does not seam to save the account.

Doing this there is a risk that multiple users could get assigned to the same account since the index would get recreated.

I guess a workaround would be assign users with account labels then call 'new_account' again if it does not exist but I have not yet tried this.

emesik commented 2 years ago

I think you should keep the track of users' account IDs in a proper database. The wallet file is definitely not the right place to reliably keep such information. Perhaps even separating funds is not a good idea and you'd better keep them in one account and track balances of users elsewhere?

Be aware that accounts are created deterministically and if you lose account X, then you only need to call .new_account() as many times as needed to reach X again. It will be the same account.

If there's a payment to account you haven't re-created yet, it's possible it would be missed by the wallet. This explanation will tell you more about subaddress-lookahead setting which controls it.

maxmqwever commented 2 years ago

Thanks for the input, I definitely consider rewrite some of the code. Although some feedback to consider is that it might seam a bit counter intuitive to have accounts when they are not quite working as accounts, unless its designed for one person per wallet.

So technically I could save the account index and the block chain height in the database and recreate the accounts and the balance would still be reflected? Another issue would be if a user creates a new account the account index number would get appended to the user and that number might belong to someone else since the new_account() restarted from another number.

maxmqwever commented 2 years ago

So to clarify I could technically call .new_account() until i reach the desired account index number then do a rescan from block count that was logged since last login and the funds should be safe? That would be an easy solution of the problem

emesik commented 2 years ago

You could just set the first value of subaddress-lookahead to accommodate the possible downtime, then there would be no need for a rescan. But keeping the account indexes in another database is, in my opinion, a must. Perhaps just store it within the user record, where name, email, password belong?

If you decide to continue with this solution... Think about an emergency migrating of your service wallet in case the keys have been compromised. Moving hundreds/thousands of accounts to another wallet would be tedious work and significant cost (one transaction per each account).

I know it's appealing to have accounts kept on blockchain and separate the funds, but it has consequences.