Move relayed HTLC htlc_minimum_msat policy check from incoming channel to outgoing one

[ariard]

A forwarded packet must conform to our HTLC relay policy, this includes
compliance with our outgoing channel policy, as announced previously to
upstream peer.

Previously, we checked HTLC acceptance with regards to htlc_minimum_msat
on our incoming channel in decode_update_add_htlc_onion(). This is a
misinterpretation of BOLT specs as we already proceed to the same check
in update_add_htlc(). This check must be done on our outgoing channel,
as thus it is moved in process_pending_htlc_forwards() before to call
send_htlc(). This latter function still verify upstream peer's
htlc_minimum_msat before to decide on forwarding HTLC.

Modify run_onion_failure_test_with_fail_intercept to test onion packet
failure by intermediate node at HTLC forwarding processing, previously
uncovered by onion failure test framework API.

While reworking our naming nomenclature (#633), I spotted that get_their_htlc_minimum_msat was in fact returning our htlc_minimum_msat. Thinking as first it was a bug, it seems to me this method was used to implement a redundant check and we were actually missing one on the outgoing channel. Spec is quite obscure (The HTLC amount was below the htlc_minimum_msat of the [outgoing] channel from the processing node.) but correct.

[codecov]

Codecov Report

Merging #670 into master will increase coverage by 0.04%.
The diff coverage is 96.66%.

@@ Coverage Diff @@## master #670 +/- ##
==========================================
+ Coverage 91.36% 91.41% +0.04% 
==========================================
Files 35 35 Lines 21703 21724 +21 ==========================================
+ Hits 19828 19858 +30 + Misses 1875 1866 -9 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	87.19% <ø> (-0.02%)	⬇️
lightning/src/ln/functional_tests.rs	97.17% <95.00%> (+0.16%)	⬆️
lightning/src/ln/channelmanager.rs	85.31% <100.00%> (+0.05%)	⬆️

---

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 d0b4f52...0e6d0b0. Read the comment docs.

[ariard]

Thanks @arik-so, I took your beyond-nits suggestion in a new commit.

FYI, constifying onion errors value has been a long standing discussion in this codebase between Matt and I (I'm for it :p )

[TheBlueMatt]

chan here is the forwarding_id channel, not the inbound channel, no? See L1151 above.

[ariard]

Okay you're right, actual code is correct, that just a badly-named method.

But, after looking further, I think that placing our forward policy check should be decided only we effectively process the HTLC forward :

• performance : it's a hit to lock the forward chan, and thus stop its operation, while we have not yet decided to accept this HTLC backward. If it rejected by update_add_htlc, we may have not to lock at all the forward one. And architecturally, that's an unnecessary tightening, you may want to run chans in parallel threads.
• correctness : channel conditions may change, like is_live() and thus we should take forward decision as as near as the real conditions we can. Also you may prevent some features like clients intentionally holding a HTLC before the forward channel is even setup. Also clients implementing this kind of hold-on/delay relay logic may expose themselves to risk, as the height against which is evaluated the cltv_delta at reception might not be the same than the one at which forwarding is accomplished, thus committing an insecure HTLC.

I would lean towards moving all forward chan related relay check in process_pending_htlc_forwards as this PR is doing for htlc_minimum_msat.

[TheBlueMatt]

The point of checking it early is that its somewhat obnoxious to sit on an HTLC until some batch timer fires before we fail it back if we don't even have an upstream channel (open) that we can forward it on to. That said, its arguably better for privacy to do so, but we should just explicitly wait to fail backwards instead of waiting to check if we can forwards.

In any case, its somewhat nicer from a performance lens to just take the lock and fail them than to make the user set a timer and call forward later.

As for correctness around is_live, see #/661, though that's not solved by this type of move. If we're really worried about state changes before we go to forward (though I'm pretty sure I've looked over the forwarding code to make sure its ok if the chain advances before we forward), then we should just refactor the checks and do them twice.

[ariard]

Closing for now, see #680

[ariard]

A forwarded packet must conform to our HTLC relay policy, this includes
compliance with our outgoing channel policy, as announced previously to
upstream peer.

Previously, we checked HTLC acceptance with regards to htlc_minimum_msat
on our incoming channel in decode_update_add_htlc_onion(). This is a
misinterpretation of BOLT specs as we already proceed to the same check
in update_add_htlc(). This check must be done on our outgoing channel,
as thus it is moved in process_pending_htlc_forwards() before to call
send_htlc(). This latter function still verify upstream peer's
htlc_minimum_msat before to decide on forwarding HTLC.

Modify run_onion_failure_test_with_fail_intercept to test onion packet
failure by intermediate node at HTLC forwarding processing, previously
uncovered by onion failure test framework API.

While reworking our naming nomenclature (#633), I spotted that get_their_htlc_minimum_msat was in fact returning our htlc_minimum_msat. Thinking as first it was a bug, it seems to me this method was used to implement a redundant check and we were actually missing one on the outgoing channel. Spec is quite obscure (The HTLC amount was below the htlc_minimum_msat of the [outgoing] channel from the processing node.) but correct.

[codecov]

Codecov Report

Merging #670 into master will increase coverage by 0.04%.
The diff coverage is 96.66%.

@@ Coverage Diff @@## master #670 +/- ##
==========================================
+ Coverage 91.36% 91.41% +0.04% 
==========================================
Files 35 35 Lines 21703 21724 +21 ==========================================
+ Hits 19828 19858 +30 + Misses 1875 1866 -9 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	87.19% <ø> (-0.02%)	⬇️
lightning/src/ln/functional_tests.rs	97.17% <95.00%> (+0.16%)	⬆️
lightning/src/ln/channelmanager.rs	85.31% <100.00%> (+0.05%)	⬆️

---

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 d0b4f52...0e6d0b0. Read the comment docs.

[ariard]

Thanks @arik-so, I took your beyond-nits suggestion in a new commit.

FYI, constifying onion errors value has been a long standing discussion in this codebase between Matt and I (I'm for it :p )

[TheBlueMatt]

chan here is the forwarding_id channel, not the inbound channel, no? See L1151 above.

[ariard]

Okay you're right, actual code is correct, that just a badly-named method.

But, after looking further, I think that placing our forward policy check should be decided only we effectively process the HTLC forward :

• performance : it's a hit to lock the forward chan, and thus stop its operation, while we have not yet decided to accept this HTLC backward. If it rejected by update_add_htlc, we may have not to lock at all the forward one. And architecturally, that's an unnecessary tightening, you may want to run chans in parallel threads.
• correctness : channel conditions may change, like is_live() and thus we should take forward decision as as near as the real conditions we can. Also you may prevent some features like clients intentionally holding a HTLC before the forward channel is even setup. Also clients implementing this kind of hold-on/delay relay logic may expose themselves to risk, as the height against which is evaluated the cltv_delta at reception might not be the same than the one at which forwarding is accomplished, thus committing an insecure HTLC.

I would lean towards moving all forward chan related relay check in process_pending_htlc_forwards as this PR is doing for htlc_minimum_msat.

[TheBlueMatt]

The point of checking it early is that its somewhat obnoxious to sit on an HTLC until some batch timer fires before we fail it back if we don't even have an upstream channel (open) that we can forward it on to. That said, its arguably better for privacy to do so, but we should just explicitly wait to fail backwards instead of waiting to check if we can forwards.

In any case, its somewhat nicer from a performance lens to just take the lock and fail them than to make the user set a timer and call forward later.

As for correctness around is_live, see #/661, though that's not solved by this type of move. If we're really worried about state changes before we go to forward (though I'm pretty sure I've looked over the forwarding code to make sure its ok if the chain advances before we forward), then we should just refactor the checks and do them twice.

[ariard]

Closing for now, see #680

[ariard]

A forwarded packet must conform to our HTLC relay policy, this includes
compliance with our outgoing channel policy, as announced previously to
upstream peer.

Previously, we checked HTLC acceptance with regards to htlc_minimum_msat
on our incoming channel in decode_update_add_htlc_onion(). This is a
misinterpretation of BOLT specs as we already proceed to the same check
in update_add_htlc(). This check must be done on our outgoing channel,
as thus it is moved in process_pending_htlc_forwards() before to call
send_htlc(). This latter function still verify upstream peer's
htlc_minimum_msat before to decide on forwarding HTLC.

Modify run_onion_failure_test_with_fail_intercept to test onion packet
failure by intermediate node at HTLC forwarding processing, previously
uncovered by onion failure test framework API.

While reworking our naming nomenclature (#633), I spotted that get_their_htlc_minimum_msat was in fact returning our htlc_minimum_msat. Thinking as first it was a bug, it seems to me this method was used to implement a redundant check and we were actually missing one on the outgoing channel. Spec is quite obscure (The HTLC amount was below the htlc_minimum_msat of the [outgoing] channel from the processing node.) but correct.

[codecov]

Codecov Report

Merging #670 into master will increase coverage by 0.04%.
The diff coverage is 96.66%.

@@ Coverage Diff @@## master #670 +/- ##
==========================================
+ Coverage 91.36% 91.41% +0.04% 
==========================================
Files 35 35 Lines 21703 21724 +21 ==========================================
+ Hits 19828 19858 +30 + Misses 1875 1866 -9 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	87.19% <ø> (-0.02%)	⬇️
lightning/src/ln/functional_tests.rs	97.17% <95.00%> (+0.16%)	⬆️
lightning/src/ln/channelmanager.rs	85.31% <100.00%> (+0.05%)	⬆️

---

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 d0b4f52...0e6d0b0. Read the comment docs.

[ariard]

Thanks @arik-so, I took your beyond-nits suggestion in a new commit.

FYI, constifying onion errors value has been a long standing discussion in this codebase between Matt and I (I'm for it :p )

[TheBlueMatt]

chan here is the forwarding_id channel, not the inbound channel, no? See L1151 above.

[ariard]

Okay you're right, actual code is correct, that just a badly-named method.

But, after looking further, I think that placing our forward policy check should be decided only we effectively process the HTLC forward :

• performance : it's a hit to lock the forward chan, and thus stop its operation, while we have not yet decided to accept this HTLC backward. If it rejected by update_add_htlc, we may have not to lock at all the forward one. And architecturally, that's an unnecessary tightening, you may want to run chans in parallel threads.
• correctness : channel conditions may change, like is_live() and thus we should take forward decision as as near as the real conditions we can. Also you may prevent some features like clients intentionally holding a HTLC before the forward channel is even setup. Also clients implementing this kind of hold-on/delay relay logic may expose themselves to risk, as the height against which is evaluated the cltv_delta at reception might not be the same than the one at which forwarding is accomplished, thus committing an insecure HTLC.

I would lean towards moving all forward chan related relay check in process_pending_htlc_forwards as this PR is doing for htlc_minimum_msat.

[TheBlueMatt]

The point of checking it early is that its somewhat obnoxious to sit on an HTLC until some batch timer fires before we fail it back if we don't even have an upstream channel (open) that we can forward it on to. That said, its arguably better for privacy to do so, but we should just explicitly wait to fail backwards instead of waiting to check if we can forwards.

In any case, its somewhat nicer from a performance lens to just take the lock and fail them than to make the user set a timer and call forward later.

As for correctness around is_live, see #/661, though that's not solved by this type of move. If we're really worried about state changes before we go to forward (though I'm pretty sure I've looked over the forwarding code to make sure its ok if the chain advances before we forward), then we should just refactor the checks and do them twice.

[ariard]

Closing for now, see #680

[ariard]

A forwarded packet must conform to our HTLC relay policy, this includes
compliance with our outgoing channel policy, as announced previously to
upstream peer.

Previously, we checked HTLC acceptance with regards to htlc_minimum_msat
on our incoming channel in decode_update_add_htlc_onion(). This is a
misinterpretation of BOLT specs as we already proceed to the same check
in update_add_htlc(). This check must be done on our outgoing channel,
as thus it is moved in process_pending_htlc_forwards() before to call
send_htlc(). This latter function still verify upstream peer's
htlc_minimum_msat before to decide on forwarding HTLC.

Modify run_onion_failure_test_with_fail_intercept to test onion packet
failure by intermediate node at HTLC forwarding processing, previously
uncovered by onion failure test framework API.

While reworking our naming nomenclature (#633), I spotted that get_their_htlc_minimum_msat was in fact returning our htlc_minimum_msat. Thinking as first it was a bug, it seems to me this method was used to implement a redundant check and we were actually missing one on the outgoing channel. Spec is quite obscure (The HTLC amount was below the htlc_minimum_msat of the [outgoing] channel from the processing node.) but correct.

[codecov]

Codecov Report

Merging #670 into master will increase coverage by 0.04%.
The diff coverage is 96.66%.

@@ Coverage Diff @@## master #670 +/- ##
==========================================
+ Coverage 91.36% 91.41% +0.04% 
==========================================
Files 35 35 Lines 21703 21724 +21 ==========================================
+ Hits 19828 19858 +30 + Misses 1875 1866 -9 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	87.19% <ø> (-0.02%)	⬇️
lightning/src/ln/functional_tests.rs	97.17% <95.00%> (+0.16%)	⬆️
lightning/src/ln/channelmanager.rs	85.31% <100.00%> (+0.05%)	⬆️

---

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 d0b4f52...0e6d0b0. Read the comment docs.

[ariard]

Thanks @arik-so, I took your beyond-nits suggestion in a new commit.

FYI, constifying onion errors value has been a long standing discussion in this codebase between Matt and I (I'm for it :p )

[TheBlueMatt]

chan here is the forwarding_id channel, not the inbound channel, no? See L1151 above.

[ariard]

Okay you're right, actual code is correct, that just a badly-named method.

But, after looking further, I think that placing our forward policy check should be decided only we effectively process the HTLC forward :

• performance : it's a hit to lock the forward chan, and thus stop its operation, while we have not yet decided to accept this HTLC backward. If it rejected by update_add_htlc, we may have not to lock at all the forward one. And architecturally, that's an unnecessary tightening, you may want to run chans in parallel threads.
• correctness : channel conditions may change, like is_live() and thus we should take forward decision as as near as the real conditions we can. Also you may prevent some features like clients intentionally holding a HTLC before the forward channel is even setup. Also clients implementing this kind of hold-on/delay relay logic may expose themselves to risk, as the height against which is evaluated the cltv_delta at reception might not be the same than the one at which forwarding is accomplished, thus committing an insecure HTLC.

I would lean towards moving all forward chan related relay check in process_pending_htlc_forwards as this PR is doing for htlc_minimum_msat.

[TheBlueMatt]

The point of checking it early is that its somewhat obnoxious to sit on an HTLC until some batch timer fires before we fail it back if we don't even have an upstream channel (open) that we can forward it on to. That said, its arguably better for privacy to do so, but we should just explicitly wait to fail backwards instead of waiting to check if we can forwards.

In any case, its somewhat nicer from a performance lens to just take the lock and fail them than to make the user set a timer and call forward later.

As for correctness around is_live, see #/661, though that's not solved by this type of move. If we're really worried about state changes before we go to forward (though I'm pretty sure I've looked over the forwarding code to make sure its ok if the chain advances before we forward), then we should just refactor the checks and do them twice.

[ariard]

Closing for now, see #680

[ariard]

A forwarded packet must conform to our HTLC relay policy, this includes
compliance with our outgoing channel policy, as announced previously to
upstream peer.

Previously, we checked HTLC acceptance with regards to htlc_minimum_msat
on our incoming channel in decode_update_add_htlc_onion(). This is a
misinterpretation of BOLT specs as we already proceed to the same check
in update_add_htlc(). This check must be done on our outgoing channel,
as thus it is moved in process_pending_htlc_forwards() before to call
send_htlc(). This latter function still verify upstream peer's
htlc_minimum_msat before to decide on forwarding HTLC.

Modify run_onion_failure_test_with_fail_intercept to test onion packet
failure by intermediate node at HTLC forwarding processing, previously
uncovered by onion failure test framework API.

While reworking our naming nomenclature (#633), I spotted that get_their_htlc_minimum_msat was in fact returning our htlc_minimum_msat. Thinking as first it was a bug, it seems to me this method was used to implement a redundant check and we were actually missing one on the outgoing channel. Spec is quite obscure (The HTLC amount was below the htlc_minimum_msat of the [outgoing] channel from the processing node.) but correct.

[codecov]

Codecov Report

Merging #670 into master will increase coverage by 0.04%.
The diff coverage is 96.66%.

@@ Coverage Diff @@## master #670 +/- ##
==========================================
+ Coverage 91.36% 91.41% +0.04% 
==========================================
Files 35 35 Lines 21703 21724 +21 ==========================================
+ Hits 19828 19858 +30 + Misses 1875 1866 -9 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	87.19% <ø> (-0.02%)	⬇️
lightning/src/ln/functional_tests.rs	97.17% <95.00%> (+0.16%)	⬆️
lightning/src/ln/channelmanager.rs	85.31% <100.00%> (+0.05%)	⬆️

---

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 d0b4f52...0e6d0b0. Read the comment docs.

[ariard]

Thanks @arik-so, I took your beyond-nits suggestion in a new commit.

FYI, constifying onion errors value has been a long standing discussion in this codebase between Matt and I (I'm for it :p )

[TheBlueMatt]

chan here is the forwarding_id channel, not the inbound channel, no? See L1151 above.

[ariard]

Okay you're right, actual code is correct, that just a badly-named method.

But, after looking further, I think that placing our forward policy check should be decided only we effectively process the HTLC forward :

• performance : it's a hit to lock the forward chan, and thus stop its operation, while we have not yet decided to accept this HTLC backward. If it rejected by update_add_htlc, we may have not to lock at all the forward one. And architecturally, that's an unnecessary tightening, you may want to run chans in parallel threads.
• correctness : channel conditions may change, like is_live() and thus we should take forward decision as as near as the real conditions we can. Also you may prevent some features like clients intentionally holding a HTLC before the forward channel is even setup. Also clients implementing this kind of hold-on/delay relay logic may expose themselves to risk, as the height against which is evaluated the cltv_delta at reception might not be the same than the one at which forwarding is accomplished, thus committing an insecure HTLC.

I would lean towards moving all forward chan related relay check in process_pending_htlc_forwards as this PR is doing for htlc_minimum_msat.

[TheBlueMatt]

The point of checking it early is that its somewhat obnoxious to sit on an HTLC until some batch timer fires before we fail it back if we don't even have an upstream channel (open) that we can forward it on to. That said, its arguably better for privacy to do so, but we should just explicitly wait to fail backwards instead of waiting to check if we can forwards.

In any case, its somewhat nicer from a performance lens to just take the lock and fail them than to make the user set a timer and call forward later.

As for correctness around is_live, see #/661, though that's not solved by this type of move. If we're really worried about state changes before we go to forward (though I'm pretty sure I've looked over the forwarding code to make sure its ok if the chain advances before we forward), then we should just refactor the checks and do them twice.

[ariard]

Closing for now, see #680

[ariard]

A forwarded packet must conform to our HTLC relay policy, this includes
compliance with our outgoing channel policy, as announced previously to
upstream peer.

Previously, we checked HTLC acceptance with regards to htlc_minimum_msat
on our incoming channel in decode_update_add_htlc_onion(). This is a
misinterpretation of BOLT specs as we already proceed to the same check
in update_add_htlc(). This check must be done on our outgoing channel,
as thus it is moved in process_pending_htlc_forwards() before to call
send_htlc(). This latter function still verify upstream peer's
htlc_minimum_msat before to decide on forwarding HTLC.

Modify run_onion_failure_test_with_fail_intercept to test onion packet
failure by intermediate node at HTLC forwarding processing, previously
uncovered by onion failure test framework API.

While reworking our naming nomenclature (#633), I spotted that get_their_htlc_minimum_msat was in fact returning our htlc_minimum_msat. Thinking as first it was a bug, it seems to me this method was used to implement a redundant check and we were actually missing one on the outgoing channel. Spec is quite obscure (The HTLC amount was below the htlc_minimum_msat of the [outgoing] channel from the processing node.) but correct.

[codecov]

Codecov Report

Merging #670 into master will increase coverage by 0.04%.
The diff coverage is 96.66%.

@@ Coverage Diff @@## master #670 +/- ##
==========================================
+ Coverage 91.36% 91.41% +0.04% 
==========================================
Files 35 35 Lines 21703 21724 +21 ==========================================
+ Hits 19828 19858 +30 + Misses 1875 1866 -9 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	87.19% <ø> (-0.02%)	⬇️
lightning/src/ln/functional_tests.rs	97.17% <95.00%> (+0.16%)	⬆️
lightning/src/ln/channelmanager.rs	85.31% <100.00%> (+0.05%)	⬆️

---

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 d0b4f52...0e6d0b0. Read the comment docs.

[ariard]

Thanks @arik-so, I took your beyond-nits suggestion in a new commit.

FYI, constifying onion errors value has been a long standing discussion in this codebase between Matt and I (I'm for it :p )

[TheBlueMatt]

chan here is the forwarding_id channel, not the inbound channel, no? See L1151 above.

[ariard]

Okay you're right, actual code is correct, that just a badly-named method.

But, after looking further, I think that placing our forward policy check should be decided only we effectively process the HTLC forward :

• performance : it's a hit to lock the forward chan, and thus stop its operation, while we have not yet decided to accept this HTLC backward. If it rejected by update_add_htlc, we may have not to lock at all the forward one. And architecturally, that's an unnecessary tightening, you may want to run chans in parallel threads.
• correctness : channel conditions may change, like is_live() and thus we should take forward decision as as near as the real conditions we can. Also you may prevent some features like clients intentionally holding a HTLC before the forward channel is even setup. Also clients implementing this kind of hold-on/delay relay logic may expose themselves to risk, as the height against which is evaluated the cltv_delta at reception might not be the same than the one at which forwarding is accomplished, thus committing an insecure HTLC.

I would lean towards moving all forward chan related relay check in process_pending_htlc_forwards as this PR is doing for htlc_minimum_msat.

[TheBlueMatt]

The point of checking it early is that its somewhat obnoxious to sit on an HTLC until some batch timer fires before we fail it back if we don't even have an upstream channel (open) that we can forward it on to. That said, its arguably better for privacy to do so, but we should just explicitly wait to fail backwards instead of waiting to check if we can forwards.

In any case, its somewhat nicer from a performance lens to just take the lock and fail them than to make the user set a timer and call forward later.

As for correctness around is_live, see #/661, though that's not solved by this type of move. If we're really worried about state changes before we go to forward (though I'm pretty sure I've looked over the forwarding code to make sure its ok if the chain advances before we forward), then we should just refactor the checks and do them twice.

[ariard]

Closing for now, see #680

[ariard]

A forwarded packet must conform to our HTLC relay policy, this includes
compliance with our outgoing channel policy, as announced previously to
upstream peer.

Previously, we checked HTLC acceptance with regards to htlc_minimum_msat
on our incoming channel in decode_update_add_htlc_onion(). This is a
misinterpretation of BOLT specs as we already proceed to the same check
in update_add_htlc(). This check must be done on our outgoing channel,
as thus it is moved in process_pending_htlc_forwards() before to call
send_htlc(). This latter function still verify upstream peer's
htlc_minimum_msat before to decide on forwarding HTLC.

Modify run_onion_failure_test_with_fail_intercept to test onion packet
failure by intermediate node at HTLC forwarding processing, previously
uncovered by onion failure test framework API.

While reworking our naming nomenclature (#633), I spotted that get_their_htlc_minimum_msat was in fact returning our htlc_minimum_msat. Thinking as first it was a bug, it seems to me this method was used to implement a redundant check and we were actually missing one on the outgoing channel. Spec is quite obscure (The HTLC amount was below the htlc_minimum_msat of the [outgoing] channel from the processing node.) but correct.

[codecov]

Codecov Report

Merging #670 into master will increase coverage by 0.04%.
The diff coverage is 96.66%.

@@ Coverage Diff @@## master #670 +/- ##
==========================================
+ Coverage 91.36% 91.41% +0.04% 
==========================================
Files 35 35 Lines 21703 21724 +21 ==========================================
+ Hits 19828 19858 +30 + Misses 1875 1866 -9 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	87.19% <ø> (-0.02%)	⬇️
lightning/src/ln/functional_tests.rs	97.17% <95.00%> (+0.16%)	⬆️
lightning/src/ln/channelmanager.rs	85.31% <100.00%> (+0.05%)	⬆️

---

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 d0b4f52...0e6d0b0. Read the comment docs.

[ariard]

Thanks @arik-so, I took your beyond-nits suggestion in a new commit.

FYI, constifying onion errors value has been a long standing discussion in this codebase between Matt and I (I'm for it :p )

[TheBlueMatt]

chan here is the forwarding_id channel, not the inbound channel, no? See L1151 above.

[ariard]

Okay you're right, actual code is correct, that just a badly-named method.

But, after looking further, I think that placing our forward policy check should be decided only we effectively process the HTLC forward :

• performance : it's a hit to lock the forward chan, and thus stop its operation, while we have not yet decided to accept this HTLC backward. If it rejected by update_add_htlc, we may have not to lock at all the forward one. And architecturally, that's an unnecessary tightening, you may want to run chans in parallel threads.
• correctness : channel conditions may change, like is_live() and thus we should take forward decision as as near as the real conditions we can. Also you may prevent some features like clients intentionally holding a HTLC before the forward channel is even setup. Also clients implementing this kind of hold-on/delay relay logic may expose themselves to risk, as the height against which is evaluated the cltv_delta at reception might not be the same than the one at which forwarding is accomplished, thus committing an insecure HTLC.

I would lean towards moving all forward chan related relay check in process_pending_htlc_forwards as this PR is doing for htlc_minimum_msat.

[TheBlueMatt]

The point of checking it early is that its somewhat obnoxious to sit on an HTLC until some batch timer fires before we fail it back if we don't even have an upstream channel (open) that we can forward it on to. That said, its arguably better for privacy to do so, but we should just explicitly wait to fail backwards instead of waiting to check if we can forwards.

In any case, its somewhat nicer from a performance lens to just take the lock and fail them than to make the user set a timer and call forward later.

As for correctness around is_live, see #/661, though that's not solved by this type of move. If we're really worried about state changes before we go to forward (though I'm pretty sure I've looked over the forwarding code to make sure its ok if the chain advances before we forward), then we should just refactor the checks and do them twice.

[ariard]

Closing for now, see #680

[ariard]

A forwarded packet must conform to our HTLC relay policy, this includes
compliance with our outgoing channel policy, as announced previously to
upstream peer.

Previously, we checked HTLC acceptance with regards to htlc_minimum_msat
on our incoming channel in decode_update_add_htlc_onion(). This is a
misinterpretation of BOLT specs as we already proceed to the same check
in update_add_htlc(). This check must be done on our outgoing channel,
as thus it is moved in process_pending_htlc_forwards() before to call
send_htlc(). This latter function still verify upstream peer's
htlc_minimum_msat before to decide on forwarding HTLC.

Modify run_onion_failure_test_with_fail_intercept to test onion packet
failure by intermediate node at HTLC forwarding processing, previously
uncovered by onion failure test framework API.

While reworking our naming nomenclature (#633), I spotted that get_their_htlc_minimum_msat was in fact returning our htlc_minimum_msat. Thinking as first it was a bug, it seems to me this method was used to implement a redundant check and we were actually missing one on the outgoing channel. Spec is quite obscure (The HTLC amount was below the htlc_minimum_msat of the [outgoing] channel from the processing node.) but correct.

[codecov]

Codecov Report

Merging #670 into master will increase coverage by 0.04%.
The diff coverage is 96.66%.

@@ Coverage Diff @@## master #670 +/- ##
==========================================
+ Coverage 91.36% 91.41% +0.04% 
==========================================
Files 35 35 Lines 21703 21724 +21 ==========================================
+ Hits 19828 19858 +30 + Misses 1875 1866 -9 

Impacted Files	Coverage Δ
lightning/src/ln/channel.rs	87.19% <ø> (-0.02%)	⬇️
lightning/src/ln/functional_tests.rs	97.17% <95.00%> (+0.16%)	⬆️
lightning/src/ln/channelmanager.rs	85.31% <100.00%> (+0.05%)	⬆️

---

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 d0b4f52...0e6d0b0. Read the comment docs.

[ariard]

Thanks @arik-so, I took your beyond-nits suggestion in a new commit.

FYI, constifying onion errors value has been a long standing discussion in this codebase between Matt and I (I'm for it :p )

[TheBlueMatt]

chan here is the forwarding_id channel, not the inbound channel, no? See L1151 above.

[ariard]

Okay you're right, actual code is correct, that just a badly-named method.

But, after looking further, I think that placing our forward policy check should be decided only we effectively process the HTLC forward :

• performance : it's a hit to lock the forward chan, and thus stop its operation, while we have not yet decided to accept this HTLC backward. If it rejected by update_add_htlc, we may have not to lock at all the forward one. And architecturally, that's an unnecessary tightening, you may want to run chans in parallel threads.
• correctness : channel conditions may change, like is_live() and thus we should take forward decision as as near as the real conditions we can. Also you may prevent some features like clients intentionally holding a HTLC before the forward channel is even setup. Also clients implementing this kind of hold-on/delay relay logic may expose themselves to risk, as the height against which is evaluated the cltv_delta at reception might not be the same than the one at which forwarding is accomplished, thus committing an insecure HTLC.

I would lean towards moving all forward chan related relay check in process_pending_htlc_forwards as this PR is doing for htlc_minimum_msat.

[TheBlueMatt]

The point of checking it early is that its somewhat obnoxious to sit on an HTLC until some batch timer fires before we fail it back if we don't even have an upstream channel (open) that we can forward it on to. That said, its arguably better for privacy to do so, but we should just explicitly wait to fail backwards instead of waiting to check if we can forwards.

In any case, its somewhat nicer from a performance lens to just take the lock and fail them than to make the user set a timer and call forward later.

As for correctness around is_live, see #/661, though that's not solved by this type of move. If we're really worried about state changes before we go to forward (though I'm pretty sure I've looked over the forwarding code to make sure its ok if the chain advances before we forward), then we should just refactor the checks and do them twice.

[ariard]

Closing for now, see #680