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

Command line 'listrequests' json output is always one confirmation behind GUI #5597

Closed
gits7r opened this issue Aug 29, 2019 · 7 comments
Closed

Comments

@gits7r
Copy link

gits7r commented Aug 29, 2019

I am coding a script for merchant applications, after the network thread has been rewritten electrum is more reliable to be used as a a merchant wallet. It relies on command line / rpc of course in order to be used in this scenario.

Here is some fast we can fix fast, maybe before lightning: (tests were made on 3.3.8):

  1. There is no PR_INFLIGHT / Paid (unconfirmed) status. In my tests in only saw statuses like Pending, Expired, and Paid.

Even if it's unconfirmed, we still see Paid as status. The only difference is that an additional parameter "confirmations": 0 is added to the json blob.

I think we should have more statuses here (merchants usually keep full log trace of user deposit attempts, etc):

  • Pending == nothing incoming yet
  • Unpaid == something is incoming but the value is less (<) than the amount of btc in the request
  • Paid (unconfirmed) == at least the sum of btc in the request (>=) has been detected as incoming, but it's not confirmed yet.
  • Paid == at least the sum of btc in the request (>=) has been confirmed at least once. We need to go to "confirmations": parameter to see exactly how many times.

We don't ditch the "confirmations": param, as some merchants have different thresholds of min number of required confirmations before marking the request as paid. Like 1000-5000$ 1 confirmation, 5001 - 50000$ 2 confirmations, etc.

Or, if we think it's simpler, we can pass the number of required confirms as a parameter, like we pass expiration, and add a new field like "minreqconfirmations": and until "confirmations": >= "minreqconfirmations" keep the status as "Paid (unconfirmed)" if the amount of BTC is also correct of course. I don't see a huge advantage for this over each merchant coding its own logic for confirmations.

  1. The "confirmations": field in the json blob is always -1 (one confirmation behind) the GUI of the same wallet connected to the same Electrum sever. My guess it that there is a typo somewhere that it also counts 0 instead of 0 unconfirmed, it counts 0, 1 and when GUI has 2 confirmations command line listrequests will show as 1 confirmation for the same txid / payment request.

  2. [maybe] - I couldn't check this as I can't create arbitrary conditions in the bitcoin network (not even on testnet) but I want to confirm and make sure that these statuses:

  • Paid (unconfirmed)
  • Paid

override the expiration of the payment request. So if the expiration time for a request is 300s, the user might send the tx and it will go into status Paid (unconfirmed) within 300s, but even under normal network conditions the time between Paid (unconfirmed) and Paid cat be even greater than the expiration time of the payment request.

If it's sent with ultra low fee and discarded from mempools, the status of the payment request will be taken directly to Expired because current time is greater than "time": + "exp": in the request. If exp is null, then it goes back to Pending instead of Expired. Not sure how to handle unpaid in this case but I think it's easiest and creates the least of confusions to just mark as Expired a request if not fully paid within its validity time window. Should anyone encounter such a case will have to contact merchant administrator and give the txid.

@gits7r
Copy link
Author

gits7r commented Aug 29, 2019

[maybe] - I couldn't check this as I can't create arbitrary conditions in the bitcoin network (not even on testnet) but I want to confirm and make sure that these statuses:

  • Paid (unconfirmed)

  • Paid

override the expiration of the payment request. So if the expiration time for a request is 300s, the user might send the tx and it will go into status Paid (unconfirmed) within 300s, but even under normal network conditions the time between Paid (unconfirmed) and Paid cat be even greater than the expiration time of the payment request.

If it's sent with ultra low fee and discarded from mempools, the status of the payment request will be taken directly to Expired because current time is greater than "time": + "exp": in the request. If exp is null, then it goes back to Pending instead of Expired. Not sure how to handle unpaid in this case but I think it's easiest and creates the least of confusions to just mark as Expired a request if not fully paid within its validity time window. Should anyone encounter such a case will have to contact merchant administrator and give the txid.

I found a way to simulate this. The bug does not exist, when it gets Paid (no matter if confirmed or not) it never goes into Expired status any longer, as expected and as it should.

But while the behavior is logic and doesn't need any change for when the payment tx is sent before expiration, the other way around might no be too OK for merchants that need strict FX rate control: if we have an Expired payment request and send the BTC to it long after it has been marked as Expired, it will be switched to status: Paid.

It would make sense to add:

  • Paid (afterexpiration, unconfirmed) status if the amount of BTC is >= of the requested amount and it was detected after the request was already in status: Expired and current number of confirmations is 0.

  • Paid (afterexpiration) status if the amount of BTC is >= of the requested amount and it was detected after the request was already in status: Expired and current number of confirmations is >= 1.

  • Unpaid (afterexpiration) status if the amount of BTC is < of the requested amount and it was detected after the request was already in status: Expired.

(there's no need for Unpaid unconfirmed)

This way such cases can be treated separately / manually / by refetching a FX rate when detected and make it easier to follow up billing related issue tickets.

@ecdsa
Copy link
Member

ecdsa commented Sep 6, 2019

should we rename this issue? it seems to be about the need for an extra state.
AFAICT the JSON output shows the same number of confirmations as as the GUI

@gits7r
Copy link
Author

gits7r commented Sep 6, 2019

I agree we could rename the issue since it only mentions one small part of the ticket and not the need for extra sates for payment requests.

Negative on the f act that JSON output shows the same number of confirmations of the GUI. It is always 1 block behind. I don't know if we treat the same in the JSON as we have in the GUI (-1 at least 1 unconfirmed parent, 0 unconfirmed, 1 confirmed once, and +1 since then).

image 1: tx confirmed at block 593553 in the GUI (as well as everywhere in the bitcoin mainnet)

1

image 2: run electrum daemon status to check we are actually fully synced to height 593553

2

image 3: listrequest returns 0 confirmations for the same payment request where the GUI has 1.

3

As soon as the GUI grows to 2 confirmations, JSON will return 1.
As soon as the GUI grows to 3, JSON will return 2.
and so on.

@ecdsa
Copy link
Member

ecdsa commented Sep 9, 2019

related #2573

@ecdsa
Copy link
Member

ecdsa commented Sep 9, 2019

I cannot reproduce the "off by one" issue. did you have it on git master, or on an older version?
edit: it might have been fixed in 8010123

@gits7r
Copy link
Author

gits7r commented Sep 9, 2019

Right. On this particular server I was not on latest git master, so I wasn't up to date with that commit. Indeed, the bug is fixed in the latest git master as the RPC JSON report is the same as the GUI, it's not off by one any more as it used to be.

Another thing that I noticed is that, when a request is Paid, it is automatically removed from the GUI -> Receive dialog. However, it is not removed from listrequests command, where it is shown as paid. This is very very good! Think of scripts or cronjobs that from time to time check all requests using listrequests and make modifications to database or whatever. This should stay like this, I was afraid that listrequests command will not show Paid requests as soon as they are deleted from the GUI, but fortunately it does not.

Also, electrum listrequests --expired or listrequests --paid or listrequests --pending work in filtering the requests in the output. We should have these enabled for all the extra states that are needed (like Paid(unconfirmed)) as well as maybe a feature to make listrequests by memo field (label). I am thinking of use cases where users will use one single wallet and payserver for several services they host on the same shop, and might want to differentiate payments using memo field instead of having separate wallets / instances.

@ecdsa
Copy link
Member

ecdsa commented Sep 10, 2019

ok, I am closing this because the issue in the title is fixed.
the request for a new state is a duplicate of #2573 and will be addressed later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants