monero-ecosystem / monero-python

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

refresh() does not update the accounts list #47

Closed Lafudoci closed 5 years ago

Lafudoci commented 5 years ago

After creating new account by w.new_account(label=user_id), the accounts seems not updating even with w.refresh() function. The number of len(w.accounts) doesn't change by refresh. So I can't access the balance of new accounts by index number, it gave error: IndexError: list index out of range. I have to restart the script to interact with new account.

Lafudoci commented 5 years ago

I found another issue maybe is related. Sometimes when I call w.new_account(label=user_id), it gives the AssertionError about accounts numbers.

Traceback (most recent call last):
  File "main.py", line 250, in <module>
    main()
  File "main.py", line 246, in main
    parse_n_send(all_push, post)
  File "main.py", line 112, in parse_n_send
    tx_result = send_tip(push[0], command_list[0], command_list[1], currency)
  File "main.py", line 167, in send_tip
    creat_xmr_account(currency_type, tip_sender)
  File "main.py", line 151, in creat_xmr_account
    w.new_account(label=user_id)
  File "C:\Users\walass\AppData\Local\Programs\Python\Python36-32\lib\site-packa
ges\monero\wallet.py", line 97, in new_account
    assert acc.index == len(self.accounts)
AssertionError
emesik commented 5 years ago

Thanks for reporting this. Just to make sure: you are not touching the same wallet with another client (CLI or GUI) when running monero-wallet-rpc, aren't you?

Lafudoci commented 5 years ago

@emesik , no, the wallet is only for rpc wallet. All the operations were performed by rpc call.

emesik commented 5 years ago

Strange, I cannot reproduce this error. In 864829b858dd17457f45bb3a1df86c34a703fc77 I added a test (test_account_creation) which describes the scenario I used. The JSON data comes from a real stagenet wallet. Could you have a look there? (The commit also fixes incorrect label handling.)

A debug output of the failing code would be very helpful. You may enable debug by import logging; logging.basicConfig(level=logging.DEBUG).

Also, are you sure that your code properly assumes account indexing from 0 everywhere?

Lafudoci commented 5 years ago

Strange, I cannot reproduce this error.

Did you mean the part of refresh() or AssertionError? I'll check my code again and test the patch later, thanks for your help.

BTW, is there any way to choose a account/adress by label instead of index?

emesik commented 5 years ago

Did you mean the part of refresh() or AssertionError?

Neither. It was working fine during all tests, and reading the code I found nothing suspicious. That's why I'd like to see debug output.

BTW, is there any way to choose a account/adress by label instead of index?

There's no such option right now, and please be aware the labels are being read from the wallet properly only with the yesterday patch.

In your scenario, where I guess the labels are unique, such mapping boils down to an one-liner:

bylabel = {a.label: a for a in w.accounts}

But the wallet doesn't require account labels to be unique, so the proper way would be to create bucket lists for each key, which breaks that simplicity. As the label seems to be just a descriptive element, not index of any kind, I don't think I should include such code in monero-python.

Lafudoci commented 5 years ago

you are not touching the same wallet with another client (CLI or GUI) when running monero-wallet-rpc

Oh, wait, does two rpc client matter? I did more tests and found the producible condition is that I have two RPC client scripts (backend creating account, sending xmr, and frontend for web checking balance). The issue comes when backend creates an account, then the frontend never gets update unless restarting frontend script. How can I solve this?

As the label seems to be just a descriptive element, not index of any kind, I don't think I should include such code in monero-python

Thanks for your reply, that makes sense to me.

emesik commented 5 years ago

Oh, wait, does two rpc client matter?

Actually, yes, they do. If you run multiple clients to the same monero-wallet-rpc instance, you need to make sure that Wallet.refresh() is called before you try to access accounts created by the other client. In a scenario where you create per-user accounts, that could mean marshaling huge amounts of data via JSON on each call, and that's the reason why I decided not to call .refresh() every time by default.

You may also encounter some other problems, as I never tested the multiple client scenario live. Please feel free to report all bugs if you decide to continue that way.

The issue comes when backend creates an account, then the frontend never gets update unless restarting frontend script. How can I solve this?

If you frontend wallet doesn't need to send XMR, it would be safer to leave it with a view-only wallet. It means two separate wallet files (spend wallet for backend, view wallet for frontend) running within two separate monero-wallet-rpc instances. If you're running the service on separate servers, removing spend key form the frontend alone could be a huge security gain.

However, two instances won't share the account labels and IIRC the view wallet would only show accounts up to the highest index of an account that received any funds.

I can only guess now what's your design, but if you assign a separate account to every user and use labels to store user:account mapping, it's possibly not the best choice for two reasons:

  1. labels aren't indexes and retrieving account for given user wouldn't scale, forcing you to keep the mapping somewhere else (so why not move it completely out of the wallet software),
  2. spending funds from different accounts at the same time is not supported (albeit technically possible) which means reduced flexibility and need to perform moves on-chain, paying fees.

Anyway, that goes well beyond the scope of this thread. It sounds interesting, so feel free to email me or ask questions at Monero Stack Exchange. I'm closing this ticket because apparently the bug isn't there, but please reopen if you think otherwise.