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

listpays rpc method could expose the payment_hash if the bolt11 is null (keysend) #3880

Closed
vincenzopalazzo opened this issue Jul 26, 2020 · 4 comments

Comments

@vincenzopalazzo
Copy link
Collaborator

vincenzopalazzo commented Jul 26, 2020

Hi all.

During my work on introducing a new command delpay (good fist issue), I noted that after the evolution of pay plugin, the method RPC listpays could return payment without id, such as the bolt11 value null if is used keysend plugin.

In cases of bolt11 value null, I can use the command listsendpays to have a result with more details. However, if I don't have a fault on my idea the command listpays could be able to return the bolt11 in cases of payment with invoice and payment_hash in cases of payment without invoice.

An example of output could be the json below

keysend payment

{
      "payment_hash": "7cdf931c7a23efb6815ff239517ef6d373bb8d040caefba96314368afd93252d",
      "status": "complete",
      "preimage": "9a4f8576e0380394ab791b35e08793533b7d1d3303c383d91fda14a13978e7db",
      "amount_sent_msat": "1000msat"
  },

invoice payment

      {
         "bolt11": "lntb10n1p03cskapp5f730rvqpqelvqmtljkuxjku2ek00qnqmf5g3pcaefc06q6rmk8sqdp9ve5hsatsta3x7mr5xyckuatvd30hgam9d3mx2xqyjw5qcqp2sp5nh2swvkuup2qrekvlmstswh8kdjjh7chyuelp7sgy0hvtz9dzhqqrzjqdsdeghn2vmdxqmy8flmzu46vxzmjzr258aavp36z3rs2redmg8cwxe0fyqqq2qqqqqqqqqpqqqqqzsqqc9qy9qsqu7l2057sh8zvpw56ftu87semua2a6a4ftjscvvfewz0gj92dsylx7whv30heqy0zr6jy6udtad57m93q3pmyezfcf4pntjqstks0v5qpedegh0",
         "status": "complete",
         "preimage": "7516ea2e790e5e8edb02709682e445d38aea07c7e772db9441b5091908d19c23",
         "amount_sent_msat": "1000msat"
      }

The content above make sense? or use a different json format inside the same rpc command and it is not correct?

reference implementation vincenzopalazzo@7c13b0b
vincenzopalazzo@b59f72b

vincenzopalazzo added a commit to vincenzopalazzo/lightning that referenced this issue Jul 27, 2020
@vincenzopalazzo
Copy link
Collaborator Author

vincenzopalazzo commented Jul 27, 2020

Also, I discovered my fault, in the past, there was a case that the bolt11 was null because these values did not store inside DB. In this case, the json response of the listpays returned a different format that it included also payment_hash.

The code that built a different format if the bol11 was null (in 0.7.3) is reported below

lightning/plugins/pay.c

Lines 1248 to 1257 in 5bc2de8

if (!b11) {
if (b11str) {
/* If it's a single query, we can fake it */
json_out_addstr(ret, "bolt11", b11str);
} else {
copy_member(ret, buf, t, "payment_hash");
copy_member(ret, buf, t, "destination");
copy_member(ret, buf, t, "amount_msat");
}
}

With the actual code, I'm not able to see these code or a code that do this behavior, (maybe is my fault or maybe this retro compatibility was a break and for these reason, we see in my original post the bolt11 value null).

In conclusion, the command listpay is the command which is possible to maintain the compatibility with new and old pay. For instance, with keysend and pay, but in this case, if I have the bolt11 null and I call the command listpays bolt11 I don't receive the correct behavior (maybe I can receive also a crash?) the only possibility that I have is call listsendpays payment_hash or listpays.

For this reason, we could to have the possibility to pass a new value inside the listpay command such as payment_hash.

I'm losing somethings?

@cdecker
Copy link
Member

cdecker commented Jul 28, 2020

I think you're right, we should add the payment_hash to the results you mentioned, potentially always, even if we have a bolt11 field, since it's about the only field that is guaranteed to be there.

@vincenzopalazzo
Copy link
Collaborator Author

I implemented somethings, I can open a PR, but before I want make enough tests and make all changes inside the docs file. To make a good work

@ZmnSCPxj
Copy link
Collaborator

fixed by #3888

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

No branches or pull requests

3 participants