lnd icon indicating copy to clipboard operation
lnd copied to clipboard

Add Payment Hash to a subset of htlc events

Open ziggie1984 opened this issue 2 years ago • 4 comments

Change Description

This PR adds the payment hash to the htlc events Forward, ForwardFail, Settle and Linkfail and fixes #7165.

I intentionally decided to not include it in the FinalHtlcEvent because it would have broken a lot of compatibility with existing code and from my perspective I see no benefit introducing them there.

Based on Jesse's Code who introduced the router-rpc tests in #6335, I created additional Testcases for the SubscribeHtlcEvents server feature.

I did not find it reasonable to add itests for this functionality because there are already tests on the unit-test level in the htlcswitch package, but open for other opinions.

Steps to Test

For the unit tests you can test it by doing:

make unit pkg=htlcswitch

For the router grpc-server you can do:

make unit pkg=lnrpc/routerrpc log="info" case=TestSubscribeHtlcEvents

Pull Request Checklist

Testing

  • [ ] Your PR passes all CI checks.
  • [x] Tests covering the positive and negative (error paths) are included.
  • [x] Bug fixes contain tests triggering the bug to prevent regressions.

Code Style and Documentation

📝 Please see our Contribution Guidelines for further guidance.

ziggie1984 avatar Jan 11 '23 11:01 ziggie1984

I cannot really find a suitable unit/integration test where we test this htlc streaming method thats why for testing purposes I included an lncli command to test the new message on regtest.

Yeah the unit test for HtlcNotifier is missing, something we def want to patch. For itest, if search SubscribeHtlcEvents, there are multiple usages, for instance testHtlcErrorPropagation.

I found it reasonable to only include the the payment_hash in events where it actually makes sense to use it. Events like SubscribedEvent and FinalHtlcEvent where intentionally not included.

Make sense.

Does it make sense to include the lncli subscribehtlcs for general usage, for now consider it experimental for testing purposes only

I'd imagine the use case for this endpoint would not be inside a terminal, so I guess not(not sure tho).

yyforyongyu avatar Jan 12 '23 17:01 yyforyongyu

Thank you :) Now I can continue my work

ziggie1984 avatar Jan 13 '23 07:01 ziggie1984

I found the unit-tests, so I changed my initial commit.

ziggie1984 avatar Jan 16 '23 15:01 ziggie1984

Testing the gprc stream of htlcs I found an error when not populating the Linkerror in the LinkFail event, though I did not change the behaviour, so asking for your opinion how to handle this.

Also in the htlcswitch package, I had to add the function newPayHash, because the unit tests reveiled that sometimes the payment circuit during a failevent is nil, maybe just returning the payment hash is not the cleanest solution here. I marked the relavant code parts in further comments

ziggie1984 avatar Jan 18 '23 13:01 ziggie1984