Add `tx_hashes` to exported payment requests

[MrNaif2018]

This pull request adds a new field, tx_hashes to payment requests output

This is useful to know which transactions were used to pay an invoice.

It should work as it relies on existing logic, the only question is: are payments via lightning network to be included here too somehow?

P.S. Failing LN tests in CI are obviously flaky and not caused by this PR

[ecdsa]

It would be misleading to return tx_hashes if the request was paid using lightning. (because the address will be recycled in another request) Currently this PR does not handle that.

[MrNaif2018]

Sorry, I am not sure what do you mean by the "because the address will be recycled in another request". Could you re-explain, please? Also I think it doesn't include lightning tx hashes for now as far as I understand

(asked in IRC too but you probably disconnected)

[ecdsa]

if a request R1 is paid with lightning, its address will be reused in another payment request R2. if R2 is paid onchain, you will see the tx hash paying R2 in the exported R1

[MrNaif2018]

Ah, I get it now. So onchain payment requests don't re-use addresses but lightning ones do? Is it because wallet.get_unused_address doesn't account for lightning?

[ecdsa]

Lightning requests have an on-chain fallback address. we need to recycle these addresses because otherwise we would soon hit the gap limit.

[MrNaif2018]

Ah, I see #7938, it might fix those issues (and others I outlined before about expired addresses not re-used being bad for gap limit), right?

[SomberNight]

if a request R1 is paid with lightning, its address will be reused in another payment request R2. if R2 is paid onchain, you will see the tx hash paying R2 in the exported R1

don't we already have the same issue with the confirmations field? https://github.com/spesmilo/electrum/blob/acde8cd0b797dd27492099956b9c7f54b6a4ccb7/electrum/wallet.py#L2415

How about something like this?

diff --git a/electrum/wallet.py b/electrum/wallet.py
index 3c00968edb..aa9bc85087 100644
--- a/electrum/wallet.py
+++ b/electrum/wallet.py
@@ -2407,12 +2407,16 @@ class Abstract_Wallet(ABC, Logger, EventListener):
             if self.lnworker and status == PR_UNPAID:
                 d['can_receive'] = self.lnworker.can_receive_invoice(x)
         if address:
-            paid, conf = self.is_onchain_invoice_paid(x)
             d['amount_sat'] = int(x.get_amount_sat())
             d['address'] = address
             d['URI'] = self.get_request_URI(x)
-            if conf is not None:
-                d['confirmations'] = conf
+            # if request was paid onchain, add relevant fields
+            # note: addr is reused when getting paid on LN! so we check for that.
+            is_paid, conf, tx_hashes = self._is_onchain_invoice_paid(x)
+            if is_paid and (not self.lnworker or self.lnworker.get_invoice_status(x) != PR_PAID):
+                if conf is not None:
+                    d['confirmations'] = conf
+                d['tx_hashes'] = tx_hashes
         run_hook('wallet_export_request', d, key)
         return d

We gravely need unit tests for the wallet invoice logic...

[SomberNight]

There is another related bug/weirdness pre-existing on master: if we get paid onchain, for an expired request, we still progress that to paid. And as we reuse addresses for requests after the prev request expires, this is really confusing imo.