Questioning the additional filtering done on the already-filtered RPC response

[lalvarezguillen]

Hi! I noticed this project is doing additional filtering on the output of the already-filtered get_transfers RPC call. See https://github.com/emesik/monero-python/blob/master/monero/backends/jsonrpc.py#L221

Some of the filtering seems redundant, and some messes with the result you'd expect to get from the Wallet RPC.

Example: We have 1 unconfirmed incoming transaction, and lets say we're currently on the block 500.

If we ask the Wallet RPC for all the in and pool transactions where min_height is 500, we would see the unconfirmed transaction.

$ curl -X POST http://127.0.0.1:18082/json_rpc -d '{"jsonrpc":"2.0","id":"0","method":"get_transfers","params":{"in":true, "pool": true, "filter_by_height": true, "min_height": 500}}' -H 'Content-Type: application/json'

But if we ask the same to monero-python, we wouldn't see the unconfirmed transaction

from monero.wallet import Wallet
from monero.backends.jsonrpc import JSONRPCWallet

wallet = Wallet(JSONRPCWallet(host='localhost', port=18082))
inc = wallet.incoming(unconfirmed=True, min_height=500)

And the problem stems from the post-filtering done here. The unconfirmed Tx would fail to meet this check, and be filtered out from the results

I'd suggest reviewing the need for checking every result against PaymentFilter.check

[emesik]

Could you include the result you get from curl request? It seems easy to craft something similar but perhaps you still have the original?

[lalvarezguillen]

Sorry, I don't have the original.

I'll invite you to reconsider the bigger picture: monero-python trying to do the filtering that the RPC server should be doing.

[emesik]

The filtering is there because in the future we may (and should) have other backends. The RPC is slow, inefficient and the existing methods are suboptimal, to say the least. I fix issues in monero code itself but this part actually asks for a major refactor, for which I don't have enough time (and the 0MQ replacement is also a dead project). Simply changing methods' behavior is just impossible due to compatibility with other existing clients that rely upon quite peculiar way of filtering, the RPC does right now.

What I should stress here, is that results of RPC calls aren't the proper way of verification if the filtering in monero-python works well. The idea is exactly the opposite: filters exist to return the user what they were asked for, without the need to know what exactly the backend does, and often resulting in a very limited subset of what the RPC had produced. And I tried to design them to behave consistently, in which — of course — I might have made mistakes.

That brings us back to the issue you've reported. I checked and found that actually it is desired behavior, not a bug. It falls along the logic that filtering by height implies asking only for txns from blockchain, as mempool txns have no height at all. There are even tests that verify it behaves that way.

Of course, both your experience and my need to check things instead of answering you right away, are very strong indicators that something is not done optimally. I guess that particular feature asks for clear documentation in the first place, and perhaps a warning/exception being raised when user asks for both unfonfirmed and height range.

Therefore, I'm leaving this issue open but not considering it a bug anymore.

Thanks a lot for your input!

[lalvarezguillen]

Thanks for the very detailed answer. That really clears thing for me.

I guess that the TLDR is, monero-python does not intend to just be a wrapper around Monero's RPC