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

Pass counterparty_node_id to fail_holding_cell_htlcs #1524

Conversation

ViktorTigerstrom
Copy link
Contributor

Both #1507 and #1403 needs the counterparty_node_id to be passed to ChannelManager::fail_holding_cell_htlcs. This PR simply passes the parameter to the function, so that both PRs can be based on this.

@codecov-commenter
Copy link

Codecov Report

Merging #1524 (cb9ae00) into main (8e5cf75) will decrease coverage by 0.04%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##             main    #1524      +/-   ##
==========================================
- Coverage   90.93%   90.88%   -0.05%     
==========================================
  Files          80       80              
  Lines       43386    43375      -11     
  Branches    43386    43375      -11     
==========================================
- Hits        39451    39422      -29     
- Misses       3935     3953      +18     
Impacted Files Coverage Δ
lightning/src/ln/channelmanager.rs 84.40% <66.66%> (ø)
lightning/src/chain/onchaintx.rs 93.98% <0.00%> (-0.93%) ⬇️
lightning/src/ln/payment_tests.rs 98.87% <0.00%> (-0.38%) ⬇️
lightning/src/util/events.rs 41.66% <0.00%> (-0.33%) ⬇️
lightning/src/ln/functional_tests.rs 96.91% <0.00%> (-0.32%) ⬇️
lightning-block-sync/src/http.rs 91.40% <0.00%> (+0.20%) ⬆️
lightning-net-tokio/src/lib.rs 76.85% <0.00%> (+0.63%) ⬆️
lightning-block-sync/src/lib.rs 93.23% <0.00%> (+1.61%) ⬆️

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 8e5cf75...cb9ae00. Read the comment docs.

TheBlueMatt
TheBlueMatt previously approved these changes Jun 6, 2022
tnull
tnull previously approved these changes Jun 7, 2022
Copy link
Contributor

@tnull tnull left a comment

Choose a reason for hiding this comment

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

LGTM, only two tiny formatting nits.

@@ -3620,7 +3620,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
// Fail a list of HTLCs that were just freed from the holding cell. The HTLCs need to be
// failed backwards or, if they were one of our outgoing HTLCs, then their failure needs to
// be surfaced to the user.
fn fail_holding_cell_htlcs(&self, mut htlcs_to_fail: Vec<(HTLCSource, PaymentHash)>, channel_id: [u8; 32]) {
fn fail_holding_cell_htlcs(&self, mut htlcs_to_fail: Vec<(HTLCSource, PaymentHash)>, channel_id: [u8; 32], _counterparty_node_id: &PublicKey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: you probably should wrap this line at 100 characters width.

@@ -5066,7 +5066,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
match chan.maybe_free_holding_cell_htlcs(&self.logger) {
Ok((commitment_opt, holding_cell_failed_htlcs)) => {
if !holding_cell_failed_htlcs.is_empty() {
failed_htlcs.push((holding_cell_failed_htlcs, *channel_id));
failed_htlcs.push((holding_cell_failed_htlcs, *channel_id, chan.get_counterparty_node_id()));
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: same here: wrap at 100 chars.

@ViktorTigerstrom ViktorTigerstrom dismissed stale reviews from tnull and TheBlueMatt via cd59bc8 June 7, 2022 20:00
@ViktorTigerstrom
Copy link
Contributor Author

Thanks for the review @tnull!
Addressed the feedback with cd59bc8! Not completely sure if it really made things better though as the code now became a bit less consistent with how we wrap the code in rest of the ChannelManager codebase :).

@tnull
Copy link
Contributor

tnull commented Jun 8, 2022

Addressed the feedback with cd59bc8! Not completely sure if it really made things better though as the code now became a bit less consistent with how we wrap the code in rest of the ChannelManager codebase :).

Yeah, I think we currently follow the policy to slowly improve the quality and formatting in parts we touch. Hopefully, though, we may be able to adopt rustfmt soon, which then hopefully would make formatting nits go away..

tnull
tnull previously approved these changes Jun 8, 2022
@ViktorTigerstrom ViktorTigerstrom force-pushed the 2022-06-update-fail-holding-cell-htlcs-args branch from cd59bc8 to 6032a56 Compare June 8, 2022 09:00
@ViktorTigerstrom
Copy link
Contributor Author

Yeah, I think we currently follow the policy to slowly improve the quality and formatting in parts we touch. Hopefully, though, we may be able to adopt rustfmt soon, which then hopefully would make formatting nits go away..

Ok great thanks :)! Squashed the line wrapping fixup without changes.

@TheBlueMatt TheBlueMatt merged commit 7adf2c7 into lightningdevkit:main Jun 9, 2022
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.

4 participants