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

Plugin: Another new notification type, forward_event #2799

Merged
merged 10 commits into from Aug 1, 2019

Conversation

@trueptolemy
Copy link
Collaborator

commented Jul 7, 2019

Introduction

A notification for topic forward_event is sent every time the status of a forward payment is set. The json format is same as the API listforwards.
Now when the forwad payment status changes, we'll store forward payments with the new status into DB. This notification will notify subscribers after storing operation.

{
  "forward_event": {
  "payment_hash": "f5a6a059a25d1e329d9b094aeeec8c2191ca037d3f5b0662e21ae850debe8ea2",
  "in_channel": "103x2x1",
  "out_channel": "103x1x1",
  "in_msatoshi": 100001001,
  "in_msat": "100001001msat",
  "out_msatoshi": 100000000,
  "out_msat": "100000000msat",
  "fee": 1001,
  "fee_msat": "1001msat",
  "status": "settled",
  "received_time": 1560696342.368,
  "resolved_time": 1560696342.556
  }
}

or

{
  "forward_event": {
  "payment_hash": "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
  "in_channel": "103x2x1",
  "out_channel": "110x1x0",
  "in_msatoshi": 100001001,
  "in_msat": "100001001msat",
  "out_msatoshi": 100000000,
  "out_msat": "100000000msat",
  "fee": 1001,
  "fee_msat": "1001msat",
  "status": "local_failed",
  "failcode": 16392,
  "failreason": "WIRE_PERMANENT_CHANNEL_FAILURE",
  "received_time": 1560696343.052
  }
}

EDIT: change the "forward_event" fieldname in json to "forward_payment".

Related Changes

  • Set the fee to 0 when forward doesn't have out channel with LOCAL_FAILED status;
  • API: listforwards include payment_hash field;
  • JSON: Warp the process of forward payment json object and use it in notification.

A Related Question

I am confused about the difference of struct sha256 and struct sha256_double: these two struct are both used to store payment_hash, maybe we need a cleanup to unify their usage for payment_hash.

@trueptolemy trueptolemy requested a review from cdecker as a code owner Jul 7, 2019

@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

commented Jul 8, 2019

sha256 is intended to specify the result of SHA256(x) while sha256_double is intended to specify the result of SHA256(SHA256(x)). Both are used since onchain we use the SHA256(SHA256(x)) but offchain we use SHA256(x) only (if I remember my BOLT specs correctly).

@ZmnSCPxj
Copy link
Collaborator

left a comment

Have not reviewed completely yet (not much time), only first three commits.

lightningd/peer_htlcs.c Show resolved Hide resolved

@trueptolemy trueptolemy force-pushed the trueptolemy:new-notify branch from 173da6f to c63c16d Jul 8, 2019

@trueptolemy

This comment was marked as outdated.

Copy link
Collaborator Author

commented Jul 8, 2019

Rebased to fix the wrong content in cdceda3 commit.
But now the order of commits present here is disrupted.
The commit 0a5b216 should be the third one(after cdceda3).

@@ -510,7 +510,7 @@ static void gossipd_incoming_channels_reply(struct subd *gossipd,
details = wallet_invoice_details(info, wallet, invoice);

response = json_stream_success(info->cmd);
json_add_hex(response, "payment_hash", details->rhash.u.u8,
json_add_hex(response, "payment_hash", &details->rhash,

This comment has been minimized.

Copy link
@ZmnSCPxj

ZmnSCPxj Jul 8, 2019

Collaborator

If this is common enough, maybe a botique json_add_sha256 could be added?

This comment has been minimized.

Copy link
@trueptolemy

trueptolemy Jul 8, 2019

Author Collaborator

Sounds good! What about adding it in anothor PR?
It seems need change many places.

"out_msat": "100000000msat",
"fee": 1001,
"fee_msat": "1001msat",
"status": "settled",

This comment has been minimized.

Copy link
@ZmnSCPxj

ZmnSCPxj Jul 8, 2019

Collaborator

The rest of your document refers to FORWARD_OFFERED, FORWARD_SETTLED, etc. Which one is actually used on the actual plugin interface? Please use what is actually visible to plugins, as this is PLUGINS.md, and avoid reference to internal defines that plugins need not know about.

This comment has been minimized.

Copy link
@trueptolemy

trueptolemy Jul 8, 2019

Author Collaborator

Thank you, I'll change FORWARD_SETTLED to "settled", etc.

}
cur->payment_hash = tal(cur, struct sha256_double);
cur->payment_hash->sha = in->payment_hash;

This comment has been minimized.

Copy link
@trueptolemy

trueptolemy Jul 8, 2019

Author Collaborator

@ZmnSCPxj My thoughts about struct sha256 and struct sha256_double are mainly reflected here.
The same payment hash data, the different structures we used.
Before storing forward payment information into DB, we used struct sha256. And after storing, we used struct sha256_double, which is involved in struct forwarding:

lightning/wallet/wallet.h

Lines 189 to 198 in 2945b25

struct forwarding {
struct short_channel_id channel_in, channel_out;
struct amount_msat msat_in, msat_out, fee;
struct sha256_double *payment_hash;
enum forward_status status;
enum onion_type failcode;
struct timeabs received_time;
/* May not be present if the HTLC was not resolved yet. */
struct timeabs *resolved_time;
};

For another example, we used struct sha256 for payment_hash in struct wallet_payment:

lightning/wallet/wallet.h

Lines 241 to 262 in 2945b25

struct wallet_payment {
/* If it's in unstored_payments */
struct list_node list;
u64 id;
u32 timestamp;
struct sha256 payment_hash;
enum wallet_payment_status status;
struct node_id destination;
struct amount_msat msatoshi;
struct amount_msat msatoshi_sent;
/* If and only if PAYMENT_COMPLETE */
struct preimage *payment_preimage;
/* Needed for recovering from routing failures. */
struct secret *path_secrets;
struct node_id *route_nodes;
struct short_channel_id *route_channels;
/* bolt11 string; NULL for old payments. */
const char *bolt11;
/* The label of the payment. Must support `tal_len` */
const char *label;
};

Like BOLT#11 said,

set payment_hash to the SHA2 256-bit hash of the payment_preimage that will be given in return for payment.

Should we change the structure of payment_hash in struct forwarding to struct sha256?

This comment has been minimized.

Copy link
@ZmnSCPxj

ZmnSCPxj Jul 8, 2019

Collaborator

It does look quite wrong... yes, we should probably fix this in a new PR.

@trueptolemy trueptolemy force-pushed the trueptolemy:new-notify branch 2 times, most recently from 57c250b to 2910427 Jul 8, 2019

@trueptolemy trueptolemy force-pushed the trueptolemy:new-notify branch from 2910427 to 254f66c Jul 19, 2019

doc/PLUGINS.md Show resolved Hide resolved

@trueptolemy trueptolemy force-pushed the trueptolemy:new-notify branch 5 times, most recently from 8bad91d to ac830d2 Jul 21, 2019

@trueptolemy

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 27, 2019

Rebased and handled conflicts.

@rustyrussell
Copy link
Contributor

left a comment

Ack ac830d2


const char *notification_topics[] = {
"connect",
"disconnect",
"warning",
"invoice_payment",
"channel_opened"
"channel_opened",
"forward_event"

This comment has been minimized.

Copy link
@rustyrussell

rustyrussell Jul 30, 2019

Contributor

Nit: For future reference, I prefer , on the trailing element, to avoid the extra line change like this. It's legal since C99 and it's been allowed by compilers for even longer, too.

This comment has been minimized.

Copy link
@trueptolemy

trueptolemy Jul 30, 2019

Author Collaborator

I guess you prefer autodata to register notification. :)

CHANGELOG.md Outdated Show resolved Hide resolved

@trueptolemy trueptolemy force-pushed the trueptolemy:new-notify branch 2 times, most recently from 91a3e1a to b52eb3c Jul 30, 2019

@trueptolemy

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 30, 2019

Rebased and corrected the CHANGELOG.
Need another rebase to correct conflict when #2859 merged.

@trueptolemy trueptolemy force-pushed the trueptolemy:new-notify branch from b52eb3c to 067a042 Jul 30, 2019

@trueptolemy

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 30, 2019

Rebased and Resolved conflict.

resolved

@ZmnSCPxj
Copy link
Collaborator

left a comment

Minor nit, sorry....

lightningd/notification.c Outdated Show resolved Hide resolved

@trueptolemy trueptolemy force-pushed the trueptolemy:new-notify branch from 067a042 to bff3f9f Jul 31, 2019

@ZmnSCPxj
Copy link
Collaborator

left a comment

ACK bff3f9f

@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2019

ACK 52f552f

Fixed merge conflict.

@ZmnSCPxj ZmnSCPxj force-pushed the trueptolemy:new-notify branch from 52f552f to d060330 Aug 1, 2019

trueptolemy added some commits Jun 15, 2019

Plugin: New notification type, forward_event
`forward_event`

A notification for topic `forward_event` is sent every time the status
of a forward payment is set. The json format is same as the API
`listforwards`.

```json
{
  "forward_event": {
  "payment_hash": "f5a6a059a25d1e329d9b094aeeec8c2191ca037d3f5b0662e21ae850debe8ea2",
  "in_channel": "103x2x1",
  "out_channel": "103x1x1",
  "in_msatoshi": 100001001,
  "in_msat": "100001001msat",
  "out_msatoshi": 100000000,
  "out_msat": "100000000msat",
  "fee": 1001,
  "fee_msat": "1001msat",
  "status": "settled",
  "received_time": 1560696342.368,
  "resolved_time": 1560696342.556
  }
}
```
or

```json
{
  "forward_event": {
  "payment_hash": "ffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff",
  "in_channel": "103x2x1",
  "out_channel": "110x1x0",
  "in_msatoshi": 100001001,
  "in_msat": "100001001msat",
  "out_msatoshi": 100000000,
  "out_msat": "100000000msat",
  "fee": 1001,
  "fee_msat": "1001msat",
  "status": "local_failed",
  "failcode": 16392,
  "failreason": "WIRE_PERMANENT_CHANNEL_FAILURE",
  "received_time": 1560696343.052
  }
}

```
 - The status includes `offered`, `settled`, `failed` and `local_failed`,
   and they are all string type in json.
   - When the forward payment is valid for us, we'll set `offered`
     and send the forward payment to next hop to resolve;
   - When the payment forwarded by us gets paid eventually, the forward
     payment will change the status from `offered` to `settled`;
   - If payment fails locally(like failing to resolve locally) or the
     corresponding htlc with next hop fails(like htlc timeout), we will
     set the status as `local_failed`. `local_failed` may be set before
     setting `offered` or after setting `offered`. In fact, from the
     time we receive the htlc of the previous hop, all we can know the
     cause of the failure is treated as `local_failed`. `local_failed`
     only occuors locally or happens in the htlc between us and next hop;
     - If `local_failed` is set before `offered`, this
       means we just received htlc from the previous hop and haven't
       generate htlc for next hop. In this case, the json of `forward_event`
       sets the fields of `out_msatoshi`, `out_msat`,`fee` and `out_channel`
       as 0;
       - Note: In fact, for this case we may be not sure if this incoming
         htlc represents a pay to us or a payment we need to forward.
         We just simply treat all incoming failed to resolve as
         `local_failed`.
     - Only in `local_failed` case, json includes `failcode` and
       `failreason` fields;
   - `failed` means the payment forwarded by us fails in the
     latter hops, and the failure isn't related to us, so we aren't
     accessed to the fail reason. `failed` must be set after
     `offered`.
     - `failed` case doesn't include `failcode` and `failreason`
       fields;
 - `received_time` means when we received the htlc of this payment from
   the previous peer. It will be contained into all status case;
 - `resolved_time` means when the htlc of this payment between us and the
   next peer was resolved. The resolved result may success or fail, so
   only `settled` and `failed` case contain `resolved_time`;
 - The `failcode` and `failreason` are defined in [BOLT 4][bolt4-failure-codes].
API: 'listforwards' now include 'payment_hash' field
'payment_hash' can help users learn more about the forward payment.
invoice: a cleanup for the json of struct sha256
Here should't be accessed directly to the underlying of struct sha256.
JSON: Warp the process of forward payment json object
Warp this process as a new function: 'void json_format_forwarding_object()'. This function will be used in 'forward_event' next, and can ensure the consistent json object structure for forward_payment between 'listforwards' API and 'forward_event' notification.

@ZmnSCPxj ZmnSCPxj force-pushed the trueptolemy:new-notify branch from d060330 to d5edafa Aug 1, 2019

@ZmnSCPxj

This comment has been minimized.

Copy link
Collaborator

commented Aug 1, 2019

ACK d5edafa

Sorry, temporarily messed up the branch, but should be ok state now.

@trueptolemy

This comment has been minimized.

Copy link
Collaborator Author

commented Aug 1, 2019

Thank you very much!

@ZmnSCPxj ZmnSCPxj merged commit 5c153de into ElementsProject:master Aug 1, 2019

2 checks passed

ackbot PR ack'd by ZmnSCPxj
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.