Update format response from listpay is the payment was did with keysend

[vincenzopalazzo]

Hi all.

This PR proposes an update to the response of the listpay command, discussed inside this issue. In particular, the listpays could return a bolt11: (null) propriety if the payment was made with keysend or if the payment was didi during the rc3 bug.

In addition, this PR proposes the change inside the parameters accepted by listpay. In fact, the command could lose the meaning if I want to use listpay to filter the payment that I made with keysend. In other words, I can not require with listpays a single payment if it is made with keysend.

Examples

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"
      }

In addition, I see inside the format description of the documentation, that reported an old description for the old payment. I think that this description could be removed or signed.

P.S: Maybe, in this PR the signature Changelog-None is not correct?

Update

After a first review, the response format is changed, now the payment_hash is printed every time and the bolt11 is printed only if the value is not null

[vincenzopalazzo]

Thanks @cdecker to review this PR and to discover my mistakes.

I'm running the local test and after it I will push the new commit. Only one thing that I need to clarify, I should insert a different Changelog? for instance Changelog-listpays? Maybe is possible to add a couple of lines inside the Haking.md to explain these rules at the people that want to start and don't know it?

[cdecker]

Oh right, I meant to comment on that as well: since you add a new optional parameter to the RPC call I'd say it needs to be a Changelog-Added: JSON-RPC: ... :-)

[vincenzopalazzo]

For the moment all done @cdecker.

Changelog-Added: JSON-RPC: listpays now accept a second optional parameter such as payment_hash

[ZmnSCPxj]

destination is probably a lot more relevant when reporting a keysend payment.

[vincenzopalazzo]

@ZmnSCPxj, this is a good point, yep I agree with this update! I will try to improve the format of this feature.

[ZmnSCPxj]

This probably means we need to add yet another argument to sendonion, an optional destination, which we record in the database, and which we modify libplugin-pay.c to also provide.

[vincenzopalazzo]

@ZmnSCPxj I wrote (74d2eee) some code to implement your suggestion and return the destination when the bolt11 is null.

At this point output is like the json below:

{
   "pays": [
      {
         "bolt11": "lntb10n1p0jga20pp53h7k2w8wkvuprjg3ff6l0y4pgdeg6lc9vsln3s74wnfsjl5fzrqqdqdw3jhxazldahx2xqyjw5qcqp2sp5wut5jnhr6n7jd5747ky2g5flmw7hgx9yjnqzu60ps2jf6f7tc0us9qy9qsqu2a0k37nckl62005p69xavlkydkvhnypk4dphffy4x09zltwh9437ad7xkl83tefdarzhu5t30ju5s56wlrg97qkx404pq3srfc425cq3ke9af",
         "payment_hash": "8dfd6538eeb33811c9114a75f792a143728d7f05643f38c3d574d3097e8910c0",
         "status": "complete",
         "preimage": "35bd4e2b481a1a84a22215b5372672cf81460a671816960ddb206464359e1822",
         "amount_msat": "1000msat",
         "amount_sent_msat": "1000msat"
      },
      {
         "destination": "0219f8900ee78a89f050c24d8b69492954f9fdbabed753710845eb75d3a75a5880",
         "payment_hash": "9c87f80bacf06a9ca08a9dcef90cf7f8b68f7b920df6020f3a0a04f35ab6c204",
         "status": "complete",
         "preimage": "3078e0c8574f756a27e7998138fe624c8d806f7075df95caee7e97efb785d108",
         "amount_msat": "1000msat",
         "amount_sent_msat": "1000msat"
      }
   ]
}

Did I understand your suggestion?

In addition, I passed a couple of hours to discover a timeout exception inside the test travis https://travis-ci.com/github/vincenzopalazzo/lightning/jobs/367116591

Any suggestion about how I can discover this bug inside the code that I wrote?

P.S: When @cdecker and @ZmnSCPxj have time, I'm happy to receive a review on the new code 😄 .

[ZmnSCPxj]

Cursory glance looks fine to me, output seems sane.

The timeout bug, no idea, it just sometimes fails that way, grumble grumble....

[vincenzopalazzo]

@ZmnSCPxj Yes is very strange, now all tests are ok, on my branch for 3 consecutive times they were red. any why, all good for the moment.

[cdecker]

Rebased on top of master

[cdecker]

Fixed up and squashed.

Addressed

[ZmnSCPxj]

ACK 8783ae4