Skip to content

Make ChannelMonitor aware of counterparty's node id #1596

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

Merged

Conversation

jurvis
Copy link
Contributor

@jurvis jurvis commented Jul 5, 2022

In #1403, we want to be able to send an HTLCHandlingFailed event, which has an enum field, next_hop_destination: HTLCDestination, that defines the reason for the error.

One of the variants is HTLCDestination::NextHopChannel, which describes the scenario where we tried forwarding to a channel but failed to do so.

When processing a channel monitor's events, we can possibly run into a scenario where we are unable to process an HTLC because the channel has closed. ChannelManager will also not be aware of the channel as a result, and we will not be able to look it up to say who the HTLC was meant for.

Therefore, the only way to define the channel counterparty in HTLCDestination::NextHopChannel is by making ChannelMonitor aware of counterparty_node_id, and piping it through MonitorEvent for it to be used.

Related discussion: #1403 (comment)

@jurvis jurvis marked this pull request as draft July 5, 2022 22:42
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why this needs to be an update - it doesn't change through the lifetime of the channel, just give the channelmonitor the counterparty's node id when its first created.

@jurvis jurvis force-pushed the jurvis/give-chanmon-counterparty-id branch from ad4fba9 to c65f150 Compare July 6, 2022 03:59
@codecov-commenter
Copy link

codecov-commenter commented Jul 6, 2022

Codecov Report

Merging #1596 (ad4fba9) into main (f3d5b94) will increase coverage by 0.04%.
The diff coverage is 88.88%.

❗ Current head ad4fba9 differs from pull request most recent head 2d49336. Consider uploading reports for the commit 2d49336 to get more accurate results

@@            Coverage Diff             @@
##             main    #1596      +/-   ##
==========================================
+ Coverage   91.07%   91.11%   +0.04%     
==========================================
  Files          80       80              
  Lines       44128    44876     +748     
  Branches    44128    44876     +748     
==========================================
+ Hits        40190    40890     +700     
- Misses       3938     3986      +48     
Impacted Files Coverage Δ
lightning/src/chain/channelmonitor.rs 90.88% <80.00%> (-0.04%) ⬇️
lightning/src/ln/channel.rs 88.73% <100.00%> (+0.01%) ⬆️
lightning/src/util/events.rs 41.66% <0.00%> (-0.33%) ⬇️
lightning-net-tokio/src/lib.rs 76.85% <0.00%> (-0.31%) ⬇️
lightning/src/ln/functional_tests.rs 96.93% <0.00%> (-0.18%) ⬇️
lightning/src/util/ser.rs 93.46% <0.00%> (+2.11%) ⬆️
lightning/src/ln/msgs.rs 88.93% <0.00%> (+2.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f3d5b94...2d49336. Read the comment docs.

@jurvis jurvis changed the title Add Counterparty ChannelMonitorUpdateStep variant Make ChannelMonitor aware of counterparty's node id Jul 6, 2022
Copy link
Collaborator

@TheBlueMatt TheBlueMatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@jkczyz jkczyz self-requested a review July 6, 2022 17:23
@jurvis jurvis force-pushed the jurvis/give-chanmon-counterparty-id branch from c65f150 to e5b3b64 Compare July 9, 2022 00:51
@jurvis jurvis marked this pull request as ready for review July 9, 2022 00:52
TheBlueMatt
TheBlueMatt previously approved these changes Jul 9, 2022
@jurvis jurvis force-pushed the jurvis/give-chanmon-counterparty-id branch from e5b3b64 to 2d49336 Compare July 9, 2022 19:47
@jurvis
Copy link
Contributor Author

jurvis commented Jul 9, 2022

@TheBlueMatt saw that I used a type number that is not consistent in read/write_tlv_fields, so I fixed that and repushed

@jkczyz
Copy link
Contributor

jkczyz commented Jul 11, 2022

Could you update the commit message / PR description for the reason for this change?

Copy link
Contributor

@dunxen dunxen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 2d4933

@jurvis
Copy link
Contributor Author

jurvis commented Jul 11, 2022

@jkczyz done :)

@jkczyz jkczyz merged commit 29e34c8 into lightningdevkit:main Jul 11, 2022
@TheBlueMatt
Copy link
Collaborator

FWIW, I'd much rather motivation be in commit descriptions than github. Ideally the git history should stand by itself without cross-referencing github for motivation for things. In this case, though, motivation is pretty trivial :)

@jurvis
Copy link
Contributor Author

jurvis commented Jul 11, 2022

@TheBlueMatt gotcha, my apologies. Will note that for next time :)

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

Successfully merging this pull request may close these issues.

5 participants