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

Store failcode of local forward failure into DB (completed) #2524

Merged
merged 8 commits into from May 3, 2019

Conversation

Projects
None yet
2 participants
@trueptolemy
Copy link
Collaborator

commented Apr 2, 2019

Store failcode of local forward into DB, and thus listforwards will show the failcode for local failed forwarding payment.(issue #2435 )

  • Add a new status for forward payment: FORWARD_LOCAL_FAILED.
    It means we meet failure locally, and we can fetch and store the failcode.

  • Add failcode field in forwarded_payments table;

  • Add failcode arugument in function wallet_forwarded_payment_add();

  • Store local failure in forward process by calling wallet_forwarded_payment_add();

Here are 5 case I think we should handle as local_failed:

Case 1. When Msater resolves the reply about the next peer infor(sent by Gossipd), and need handle unknown next peer failure in channel_resolve_reply().

Test for this case: I ask l1 pay to l3 through l2 but close the channel between l2 and l3 after getroute(), the payment will fail in l2 because of WIRE_UNKNOWN_NEXT_PEER;

Case 2. When Master handle the forward process with the htlc_in and the id of next hop, it tries to drive a new htlc_out but fails in forward_htlc().

Test for this case: I ask l1 pay to 14 through with no fee, so the payment will fail in l2 becase of WIRE_FEE_INSUFFICIENT;

Case 3. When we send htlc_out, Master asks Channeld to add a new htlc into the outgoing channel but Channeld fails. Master need handle and store this failure in rcvd_htlc_reply().

Test for this case: I ask l1 pay to l5 with 10^8 sat through the channel (l2-->l5) with the max capacity of 10^4 msat , the payment will fail in l2 because of CHANNEL_ERR_MAX_HTLC_VALUE_EXCEEDED;

Case 4. When Channeld receives a new revoke message, if the state of corresponding htlc is RCVD_ADD_ACK_REVOCATION, Master will tries to resolve onionpacket and handle the failure before resolving the next hop in peer_got_revoke().

Test for this case: I ask l6 pay to l4 through l1 and l2, but we replace the second node_id in [route] with the wrong one, so the payment will fail in l2 because of WIRE_INVALID_ONION_KEY;

Case 5. When Onchaind finds the htlc time out or missing htlc, Master need handle these failure as FORWARD_LOCAL_FAILED in if it's forward payment case.

Test for this case: I ask l1 pay to l4 through l2 with the amount less than the invoice(the payment must fail in l4), and we also ask l5 disconnected before sending update_fail_htlc, so the htlc will be holding until l2 meets timeout and handle it as local_fail.

EDIT: The last commit in Heisei era.

@trueptolemy trueptolemy requested a review from cdecker as a code owner Apr 2, 2019

@trueptolemy trueptolemy changed the title DB: Store failcode of local forward into DB DB: Store failcode of local forward failure into DB Apr 2, 2019

@trueptolemy trueptolemy force-pushed the trueptolemy:issue-2435 branch 6 times, most recently from f2b4585 to c25c7f2 Apr 2, 2019

@trueptolemy trueptolemy force-pushed the trueptolemy:issue-2435 branch from c25c7f2 to ecf6cd8 Apr 4, 2019

@trueptolemy trueptolemy changed the title DB: Store failcode of local forward failure into DB WIP: Store failcode of local forward failure into DB Apr 4, 2019

@trueptolemy trueptolemy force-pushed the trueptolemy:issue-2435 branch 7 times, most recently from 03f2fb5 to 8f1771b Apr 26, 2019

@trueptolemy trueptolemy changed the title WIP: Store failcode of local forward failure into DB Store failcode of local forward failure into DB (completed) Apr 30, 2019

@trueptolemy

This comment has been minimized.

Copy link
Collaborator Author

commented Apr 30, 2019

Hi! @cdecker I've done. What do you think about? Thank you!

@cdecker

This comment has been minimized.

Copy link
Member

commented Apr 30, 2019

Removed the WIP tag, will review asap

@trueptolemy trueptolemy force-pushed the trueptolemy:issue-2435 branch 2 times, most recently from 3bbaec5 to b070dd5 May 1, 2019

@cdecker cdecker added the needs-rebase label May 2, 2019

@cdecker
Copy link
Member

left a comment

Just two minor cleanups:

  • Reordering the sqlite3_bind_* calls would simplify the if-else-statements
  • Reordering commits would fix bisectability

In addition I'd like to fix the JSON-output to be machine readable.

Show resolved Hide resolved wallet/wallet.c Outdated
Show resolved Hide resolved wallet/db.c
Show resolved Hide resolved lightningd/peer_htlcs.c Outdated
Show resolved Hide resolved lightningd/peer_htlcs.c Outdated

trueptolemy added some commits Apr 15, 2019

wallet: add 'FORWARD_LOCAL_FAILED' as a new forward_status type
In FORWARD_LOCAL_FAILED case, we can get the failcode and can't get the resolve time, that is different with FORWARD_FAILED case.
wallet: add failcode field in forwarding struct
It's a optional field. We only set failcode for FORWARD_LOCAL_FAILED case, and set this field with 0 in other case.

@trueptolemy trueptolemy force-pushed the trueptolemy:issue-2435 branch from b070dd5 to 7831fca May 2, 2019

@trueptolemy trueptolemy force-pushed the trueptolemy:issue-2435 branch from 7831fca to c75570d May 2, 2019

test_pay: Add test_forward_local_failed_stats() in 5 cases
Here I add the test for this 5 local_failed case in this commit.
There 5 cases for FORWARD_LOCAL_FAILED status:
    1. When Msater resolves the reply about the next peer infor(sent by Gossipd), and need handle unknown next peer failure in channel_resolve_reply();
    2. When Master handle the forward process with the htlc_in and the id of next hop, it tries to drive a new htlc_out but fails in forward_htlc();
    3. When we send htlc_out, Master asks Channeld to add a new htlc into the outgoing channel but Channeld fails. Master need handle and store this failure in rcvd_htlc_reply();
    4. When Channeld receives a new revoke message, if the state of corresponding htlc is RCVD_ADD_ACK_REVOCATION, Master will tries to resolve onionpacket and handle the failure before resolving the next hop in peer_got_revoke();
    5. When Onchaind finds the htlc time out or missing htlc, Master need handle these failure as FORWARD_LOCAL_FAILED in if it's forward payment case.

@trueptolemy trueptolemy force-pushed the trueptolemy:issue-2435 branch from c75570d to 88a28d5 May 3, 2019

@cdecker

This comment has been minimized.

Copy link
Member

commented May 3, 2019

ACK 88a28d5

@cdecker

cdecker approved these changes May 3, 2019

@cdecker cdecker merged commit 77f98f8 into ElementsProject:master May 3, 2019

2 checks passed

ackbot PR ack'd by cdecker
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@trueptolemy trueptolemy deleted the trueptolemy:issue-2435 branch May 3, 2019

@trueptolemy trueptolemy restored the trueptolemy:issue-2435 branch May 4, 2019

@trueptolemy trueptolemy deleted the trueptolemy:issue-2435 branch May 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.