Do not lock while looping htlcs_to_fail
#1880
Conversation
Codecov ReportBase: 90.72% // Head: 90.70% // Decreases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #1880 +/- ##
==========================================
- Coverage 90.72% 90.70% -0.03%
==========================================
Files 91 91
Lines 47999 48011 +12
Branches 47999 48011 +12
==========================================
+ Hits 43546 43547 +1
- Misses 4453 4464 +11
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
lightning/src/ln/channelmanager.rs
Outdated
| failure_code, data, | ||
| }, HTLCDestination::NextHopChannel { node_id: Some(channel.get_counterparty_node_id()), channel_id: channel.channel_id() })); | ||
| timed_out_htlcs.push((source, payment_hash, HTLCFailReason::reason(failure_code, data), | ||
| HTLCDestination::NextHopChannel { node_id: Some(channel.get_counterparty_node_id()), channel_id: channel.channel_id() })); |
There was a problem hiding this comment.
Nit: unnecessary indentation here (and below)
There was a problem hiding this comment.
Oh, is one tab in from the start of the function call the place you usually put args like this?
There was a problem hiding this comment.
Generally yea, mostly because otherwise the lines get looongggg especially after we've indented five times cause we have two matches ;)
There was a problem hiding this comment.
And is the choice of line length arbitrary (when to put the next arg on a newline)? I'm not trolling, just trying to fit in with the code.
There was a problem hiding this comment.
The intended limit is 100 chars.
tcharding
force-pushed
the
11-29-move-lock-outside-loop
branch
from
e0cc997 to
aaa2820
Compare
|
Changes in force push:
|
|
Oops, sorry, needs rebase (and there's some new uses of |
tcharding
force-pushed
the
11-29-move-lock-outside-loop
branch
from
aaa2820 to
4f14a9c
Compare
We create `HTLCFailReason` inline in function calls in a bunch of places in the `channelmanager` module, we can make the code more terse with no loss of clarity by implementing a couple of constructor methods.
Currently `fail_htlc_backwards_internal` takes ownership of its source and reason parameters however they are not consumed so we can borrow them. Includes refactoring to use local variables before the function call.
Currently we loop over `htlcs_to_fail` locking `channel_state` for each element only to call `get_htlc_inbound_temp_fail_err_and_data` with the same inputs on each iteration. This is unnecessary, we can refactor and call `get_htlc_inbound_temp_fail_err_and_data` outside of the loop.
tcharding
force-pushed
the
11-29-move-lock-outside-loop
branch
from
4f14a9c to
1dd3184
Compare
|
Tagged 113 as #1879 will probably want to use the changes here. |
Currently we loop over
htlcs_to_faillockingchannel_statefor each element only to callget_htlc_inbound_temp_fail_err_and_datawith the same inputs on each iteration. This is unnecessary, we can refactor and callget_htlc_inbound_temp_fail_err_and_dataoutside of the loop.The first two patches prepare the way for patch 3 which moves the locking.
Fix #1727
Note to reviewers
This is my first PR to
rust-lightning, please prod me if anything is done incorrectly for the project. I was struggling with the indentation (tabs vs spaces) of function arguments when spread over multiple lines so I went around the problem and added local variables.Patch 1 adds constructors to an
enum, this is slightly unusual and not done anywhere else in the codebase that I saw.