Add random 'shadow route' CLTV delta offsets to improve privacy.

[tnull]

This PR is an attempt to address #158.

In order to generate a plausible CLTV delta offset, a three-hop random walk starting from the payee is conducted and cltv_expiry_delta values are aggregated. This offset is then added to all but the final hop of the selected paths (since the last hop knows anyways that it is the recipient).

Tests are currently failing since they of course do not expect the randomly added offset. I'll fix them when someone had a look at the implementation itself.

[TheBlueMatt]

Sorry for the delay on this one! A few comments, don't worry about the failing tests for now, I can help out with those if needed.

[tnull]

Addressed comments and rebased off a1fedea.

[ariard]

One improvement, I think if the randomly selected next hop is actually part of the extended payment path you could pick up another hop to pursue the random walk. What we would like to avoid is the random walk rolling back on the actually drawn out path. Let's say you have A -> B -> C -> D and you sort the random walk D -> C -> B -> A thus a shadow offset partially overlapping with the real cltv deltas. I would say it's a low-probability phenomena but the last publicly known node might not have numerous public channels opened for routing.

That said, it might not be worthy the effort at that point.

[tnull]

Thank you very much for having a look at this! Could you elaborate why we wouldn't want the shadow route to overlap the actual path? I see this only as a problem if we assume an on-path adversary that can say for sure that it wasn't part of the actual payment path and hence might be able to reduce the number of candidate routes. However, if we exclude overlapping paths from the beginning, we might reduce the randomness from the getgo, making any adversary's life easier?

[TheBlueMatt]

I'm not sure if its worth the effort to implement this, but I see a few concerns with overlapping paths. eg if you're an attacker and the CLTV deltas only line up with one path, and it overlaps/backtraces, you know for sure that that is shadow route, because the normal routing algorithm won't exhibit that behavior.

[tnull]

Hum, I think it may not be worth to rule out any overlap, since this could indeed reduce the variety of potential random walks quite a bit. But maybe it would be enough to ensure that we do not immediately loop back on the actual path. And in the case this excludes the only public edge we could fall back to a 'synthetic' random offset, just as in the case for fully private paths.

[TheBlueMatt]

Yea, I'll leave it up to you. I don't care too much because so many nodes use the same CLTV delta that its probably all the same in practice, but if its easy to avoid the same-node-backtrack then feel free to add it.

[tnull]

I think I'm leaning on the side of leaving this as-is for now and potentially having another look at it in a potential follow-up PR. We might want to have a follow-up anyways, for example in order to introduce some kind of value obfuscation, as noted in #158.

[ariard]

Yeah really up to you, fine to leave it as a follow-up.

[TheBlueMatt]

Looks like this needs rebase, sorry about that.

[tnull]

Looks like this needs rebase, sorry about that.

Rebased.

[TheBlueMatt]

I'm basically happy with this. There's some compilation errors due to new tests that weren't updated on rebase and the benchmark-only code needs updating, I believe. I'm happy to help resolve any test failures left after that.

[TheBlueMatt]

To make the router tests pass, maybe it'd be best to split get_route into two methods, one that gets the route and one that applies the CLTV deltas so that you don't have to update all the tests.

[tnull]

To make the router tests pass, maybe it'd be best to split get_route into two methods, one that gets the route and one that applies the CLTV deltas so that you don't have to update all the tests.

Good idea, especially given that get_route is ~1000 lines already. I now split adding the offset to its dedicated add_random_cltv_offset method and added a test.

For the time being I left random_seed_bytes as a parameter to get_route instead of removing it and all the changes to the test etc. I think it could be used to implement the 'real' random shuffle in Step (6). If that is wanted I can add this to this PR or open a follow-up.

[codecov-commenter]

Codecov Report

Merging #1286 (e92b5a7) into main (af99a94) will decrease coverage by 0.14%.
The diff coverage is 95.26%.

@@ Coverage Diff @@## main #1286 +/- ##
==========================================
- Coverage 90.75% 90.60% -0.15% 
==========================================
Files 72 72 Lines 42254 40324 -1930 ==========================================
- Hits 38347 36536 -1811 + Misses 3907 3788 -119 

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	86.92% <58.33%> (-1.22%)	⬇️
lightning/src/ln/channelmanager.rs	84.61% <87.50%> (-1.52%)	⬇️
lightning/src/routing/network_graph.rs	89.52% <88.88%> (-0.01%)	⬇️
lightning/src/routing/router.rs	92.39% <96.15%> (+0.29%)	⬆️
lightning/src/routing/scoring.rs	94.29% <96.96%> (-1.13%)	⬇️
lightning-background-processor/src/lib.rs	93.10% <100.00%> (+0.05%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.32% <100.00%> (+0.03%)	⬆️
lightning/src/ln/functional_tests.rs	97.13% <100.00%> (+0.36%)	⬆️
lightning/src/ln/onion_route_tests.rs	97.62% <100.00%> (ø)
lightning/src/ln/payment_tests.rs	99.15% <100.00%> (+<0.01%)	⬆️
... and 8 more

---

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 1a73449...e92b5a7. Read the comment docs.

[TheBlueMatt]

Feel free to squash the commits down to one or two, note that we'd ideally like to ensure all code compiles and all tests pass after each commit.

[tnull]

Feel free to squash the commits down to one or two, note that we'd ideally like to ensure all code compiles and all tests pass after each commit.

Sure sure, just wanted to avoid squashing too early. Rebased and squashed with 67137c6.

[TheBlueMatt]

Some style nits and minor test comments.

[TheBlueMatt]

As is the lock is held through find_route which we maybe don't want. Instead, we'll need an explicit {} scope to tell the compiler where to drop the locked_random_seed_bytes

[jkczyz]

Should we consider parameterizing DefaultRouter by K: Deref where K::Target: KeysInterface and call get_secure_random_bytes each time? I suppose the interface is too broad, but I wonder if a narrower Randomness interface is warranted. No strong opinion here, but it would avoid the mutex.

[tnull]

I now went the scoping route with 386a0b5. However, I agree that introducing a Randomness interface (not only here) would be a nicer abstraction than pushing around arrays with random_seed_bytes. Possibly a good follow-up PR?

[TheBlueMatt]

I suppose we could, yea, agree we'd want to do it in a followup, though. I'm a little hesitant to replace a trivial parameter with a full interface, though, as it just seems like overkill, but we can discuss that later.

[jkczyz]

Should we consider parameterizing DefaultRouter by K: Deref where K::Target: KeysInterface and call get_secure_random_bytes each time? I suppose the interface is too broad, but I wonder if a narrower Randomness interface is warranted. No strong opinion here, but it would avoid the mutex.

[ariard]

Good with the new approach of only offsetting the final hop.
(Thought the old one had the benefit to also mask the sender topology as the intermediate node couldn't deduce from their CLTV deltas the distance from sender, I believe)

[tnull]

Thanks for all the great feedback, I'll take it into account going forward!

[TheBlueMatt]

Shouldn't this be unwrap_or(0)? If our path expiry is more than the max already we probably shouldn't add an offset at all? Also, were we planning on limiting the total CLTV during the get_route run to give us a little headroom here?

[tnull]

Well my thinking here was that since we limit the route overall with max_total_cltv_expiry_delta, this subtraction should never fail. If however something unexpected were to happen and it would fail, we might be better off to default to provide some privacy (capped at 3*144), instead of none. But it can of course also default to 0, I have no strong preference here.

As for the considering the offset in get_route: after looking at the data I found that there were basically no cases in which there was not ample room to add the offset and hence thought it would be cleaner not to interfere with pathfinding and keep the two methods entirely separate. However, if we still want to take some of the offset into account, I can of course go back to, for example, subtracting 2*40 or 2*144 from the limit during path finding.

[TheBlueMatt]

Well my thinking here was that since we limit the route overall with max_total_cltv_expiry_delta, this subtraction should never fail.

Right, sorry, indeed.

If however something unexpected were to happen and it would fail, we might be better off to default to provide some privacy (capped at 3*144), instead of none.

🤷‍♂️ I guess we shouldn't ever violate the user-provided value ever?

As for the considering the offset in get_route: #1286 (comment) I found that there were basically no cases in which there was not ample room to add the offset and hence thought it would be cleaner not to interfere with pathfinding and keep the two methods entirely separate.

Hmm, I feel like that's a better reason to interfere than to not interfere.

I'm still a bit confused by this hunk here - why do we do the subtraction at max_path_offset.checked_sub(max_path_offset.wrapping_rem(40))?

[tnull]

🤷‍♂️ I guess we shouldn't ever violate the user-provided value ever?
...
Hmm, I feel like that's a better reason to interfere than to not interfere.

Alright, both addressed by f7be9da.

I'm still a bit confused by this hunk here - why do we do the subtraction at max_path_offset.checked_sub(max_path_offset.wrapping_rem(40))?

The difference of the path total to max_total_cltv_expiry_delta may be a pretty uncommon value, which could result in an implausible shadow route offset. The thinking here therefore was to choose the path offset to be the closest multiple of 40 to increase plausibility. I now had another look at this part and added a comment to make this more transparent in f7be9da.

[tnull]

Rebased and squashed once more.

[jkczyz]

random_seed_bytes is not needed here since it is only used in add_random_cltv_offset.

[tnull]

This is correct. If however possible, I’d like to leave it in get_route for now and use it to implement the 'real random shuffle' TODO in a follow-up PR. I now prefixed it with an underscore to disable the warning, but let me know if you'd rather keep it out of get_route altogether in this PR.

[jurvis]

hey @tnull is it okay if I take a stab at implementing the real random shuffle? I believe it may be related to this issue #869

[tnull]

hey @tnull is it okay if I take a stab at implementing the real random shuffle? I believe it may be related to this issue #869

Ah, thank you for the interest! This issue indeed seems related. However, I’m already mostly done with a first draft and plan to open a PR in the next few days.

[jkczyz]

If the subtraction wraps, shadow_ctlv_expiry_delta_offset would not likely change, IIUC. Could it then possibly exceed payment_params.max_total_cltv_expiry_delta for small values?

[tnull]

Hum, I'm not entirely sure if I'm understanding you correctly. The wrapping_sub should actually never wrap due to x >= x mod y. I made this a bit more explicit by using saturating_sub in 0b777cb.
Moreover, the line prior ensures that max_path_offset is never larger than max_total_cltv_expiry_delta and all changes from there only ever would reduce it further.

[tnull]

Rebased.

[tnull]

This PR is an attempt to address #158.

In order to generate a plausible CLTV delta offset, a three-hop random walk starting from the payee is conducted and cltv_expiry_delta values are aggregated. This offset is then added to all but the final hop of the selected paths (since the last hop knows anyways that it is the recipient).

Tests are currently failing since they of course do not expect the randomly added offset. I'll fix them when someone had a look at the implementation itself.

[TheBlueMatt]

Sorry for the delay on this one! A few comments, don't worry about the failing tests for now, I can help out with those if needed.

[tnull]

Addressed comments and rebased off a1fedea.

[ariard]

One improvement, I think if the randomly selected next hop is actually part of the extended payment path you could pick up another hop to pursue the random walk. What we would like to avoid is the random walk rolling back on the actually drawn out path. Let's say you have A -> B -> C -> D and you sort the random walk D -> C -> B -> A thus a shadow offset partially overlapping with the real cltv deltas. I would say it's a low-probability phenomena but the last publicly known node might not have numerous public channels opened for routing.

That said, it might not be worthy the effort at that point.

[tnull]

Thank you very much for having a look at this! Could you elaborate why we wouldn't want the shadow route to overlap the actual path? I see this only as a problem if we assume an on-path adversary that can say for sure that it wasn't part of the actual payment path and hence might be able to reduce the number of candidate routes. However, if we exclude overlapping paths from the beginning, we might reduce the randomness from the getgo, making any adversary's life easier?

[TheBlueMatt]

I'm not sure if its worth the effort to implement this, but I see a few concerns with overlapping paths. eg if you're an attacker and the CLTV deltas only line up with one path, and it overlaps/backtraces, you know for sure that that is shadow route, because the normal routing algorithm won't exhibit that behavior.

[tnull]

Hum, I think it may not be worth to rule out any overlap, since this could indeed reduce the variety of potential random walks quite a bit. But maybe it would be enough to ensure that we do not immediately loop back on the actual path. And in the case this excludes the only public edge we could fall back to a 'synthetic' random offset, just as in the case for fully private paths.

[TheBlueMatt]

Yea, I'll leave it up to you. I don't care too much because so many nodes use the same CLTV delta that its probably all the same in practice, but if its easy to avoid the same-node-backtrack then feel free to add it.

[tnull]

I think I'm leaning on the side of leaving this as-is for now and potentially having another look at it in a potential follow-up PR. We might want to have a follow-up anyways, for example in order to introduce some kind of value obfuscation, as noted in #158.

[ariard]

Yeah really up to you, fine to leave it as a follow-up.

[TheBlueMatt]

Looks like this needs rebase, sorry about that.

[tnull]

Looks like this needs rebase, sorry about that.

Rebased.

[TheBlueMatt]

I'm basically happy with this. There's some compilation errors due to new tests that weren't updated on rebase and the benchmark-only code needs updating, I believe. I'm happy to help resolve any test failures left after that.

[TheBlueMatt]

To make the router tests pass, maybe it'd be best to split get_route into two methods, one that gets the route and one that applies the CLTV deltas so that you don't have to update all the tests.

[tnull]

To make the router tests pass, maybe it'd be best to split get_route into two methods, one that gets the route and one that applies the CLTV deltas so that you don't have to update all the tests.

Good idea, especially given that get_route is ~1000 lines already. I now split adding the offset to its dedicated add_random_cltv_offset method and added a test.

For the time being I left random_seed_bytes as a parameter to get_route instead of removing it and all the changes to the test etc. I think it could be used to implement the 'real' random shuffle in Step (6). If that is wanted I can add this to this PR or open a follow-up.

[codecov-commenter]

Codecov Report

Merging #1286 (e92b5a7) into main (af99a94) will decrease coverage by 0.14%.
The diff coverage is 95.26%.

@@ Coverage Diff @@## main #1286 +/- ##
==========================================
- Coverage 90.75% 90.60% -0.15% 
==========================================
Files 72 72 Lines 42254 40324 -1930 ==========================================
- Hits 38347 36536 -1811 + Misses 3907 3788 -119 

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	86.92% <58.33%> (-1.22%)	⬇️
lightning/src/ln/channelmanager.rs	84.61% <87.50%> (-1.52%)	⬇️
lightning/src/routing/network_graph.rs	89.52% <88.88%> (-0.01%)	⬇️
lightning/src/routing/router.rs	92.39% <96.15%> (+0.29%)	⬆️
lightning/src/routing/scoring.rs	94.29% <96.96%> (-1.13%)	⬇️
lightning-background-processor/src/lib.rs	93.10% <100.00%> (+0.05%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.32% <100.00%> (+0.03%)	⬆️
lightning/src/ln/functional_tests.rs	97.13% <100.00%> (+0.36%)	⬆️
lightning/src/ln/onion_route_tests.rs	97.62% <100.00%> (ø)
lightning/src/ln/payment_tests.rs	99.15% <100.00%> (+<0.01%)	⬆️
... and 8 more

---

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 1a73449...e92b5a7. Read the comment docs.

[TheBlueMatt]

Feel free to squash the commits down to one or two, note that we'd ideally like to ensure all code compiles and all tests pass after each commit.

[tnull]

Feel free to squash the commits down to one or two, note that we'd ideally like to ensure all code compiles and all tests pass after each commit.

Sure sure, just wanted to avoid squashing too early. Rebased and squashed with 67137c6.

[TheBlueMatt]

Some style nits and minor test comments.

[TheBlueMatt]

As is the lock is held through find_route which we maybe don't want. Instead, we'll need an explicit {} scope to tell the compiler where to drop the locked_random_seed_bytes

[jkczyz]

Should we consider parameterizing DefaultRouter by K: Deref where K::Target: KeysInterface and call get_secure_random_bytes each time? I suppose the interface is too broad, but I wonder if a narrower Randomness interface is warranted. No strong opinion here, but it would avoid the mutex.

[tnull]

I now went the scoping route with 386a0b5. However, I agree that introducing a Randomness interface (not only here) would be a nicer abstraction than pushing around arrays with random_seed_bytes. Possibly a good follow-up PR?

[TheBlueMatt]

I suppose we could, yea, agree we'd want to do it in a followup, though. I'm a little hesitant to replace a trivial parameter with a full interface, though, as it just seems like overkill, but we can discuss that later.

[jkczyz]

Should we consider parameterizing DefaultRouter by K: Deref where K::Target: KeysInterface and call get_secure_random_bytes each time? I suppose the interface is too broad, but I wonder if a narrower Randomness interface is warranted. No strong opinion here, but it would avoid the mutex.

[ariard]

Good with the new approach of only offsetting the final hop.
(Thought the old one had the benefit to also mask the sender topology as the intermediate node couldn't deduce from their CLTV deltas the distance from sender, I believe)

[tnull]

Thanks for all the great feedback, I'll take it into account going forward!

[TheBlueMatt]

Shouldn't this be unwrap_or(0)? If our path expiry is more than the max already we probably shouldn't add an offset at all? Also, were we planning on limiting the total CLTV during the get_route run to give us a little headroom here?

[tnull]

Well my thinking here was that since we limit the route overall with max_total_cltv_expiry_delta, this subtraction should never fail. If however something unexpected were to happen and it would fail, we might be better off to default to provide some privacy (capped at 3*144), instead of none. But it can of course also default to 0, I have no strong preference here.

As for the considering the offset in get_route: after looking at the data I found that there were basically no cases in which there was not ample room to add the offset and hence thought it would be cleaner not to interfere with pathfinding and keep the two methods entirely separate. However, if we still want to take some of the offset into account, I can of course go back to, for example, subtracting 2*40 or 2*144 from the limit during path finding.

[TheBlueMatt]

Well my thinking here was that since we limit the route overall with max_total_cltv_expiry_delta, this subtraction should never fail.

Right, sorry, indeed.

If however something unexpected were to happen and it would fail, we might be better off to default to provide some privacy (capped at 3*144), instead of none.

🤷‍♂️ I guess we shouldn't ever violate the user-provided value ever?

As for the considering the offset in get_route: #1286 (comment) I found that there were basically no cases in which there was not ample room to add the offset and hence thought it would be cleaner not to interfere with pathfinding and keep the two methods entirely separate.

Hmm, I feel like that's a better reason to interfere than to not interfere.

I'm still a bit confused by this hunk here - why do we do the subtraction at max_path_offset.checked_sub(max_path_offset.wrapping_rem(40))?

[tnull]

🤷‍♂️ I guess we shouldn't ever violate the user-provided value ever?
...
Hmm, I feel like that's a better reason to interfere than to not interfere.

Alright, both addressed by f7be9da.

I'm still a bit confused by this hunk here - why do we do the subtraction at max_path_offset.checked_sub(max_path_offset.wrapping_rem(40))?

The difference of the path total to max_total_cltv_expiry_delta may be a pretty uncommon value, which could result in an implausible shadow route offset. The thinking here therefore was to choose the path offset to be the closest multiple of 40 to increase plausibility. I now had another look at this part and added a comment to make this more transparent in f7be9da.

[tnull]

Rebased and squashed once more.

[jkczyz]

random_seed_bytes is not needed here since it is only used in add_random_cltv_offset.

[tnull]

This is correct. If however possible, I’d like to leave it in get_route for now and use it to implement the 'real random shuffle' TODO in a follow-up PR. I now prefixed it with an underscore to disable the warning, but let me know if you'd rather keep it out of get_route altogether in this PR.

[jurvis]

hey @tnull is it okay if I take a stab at implementing the real random shuffle? I believe it may be related to this issue #869

[tnull]

hey @tnull is it okay if I take a stab at implementing the real random shuffle? I believe it may be related to this issue #869

Ah, thank you for the interest! This issue indeed seems related. However, I’m already mostly done with a first draft and plan to open a PR in the next few days.

[jkczyz]

If the subtraction wraps, shadow_ctlv_expiry_delta_offset would not likely change, IIUC. Could it then possibly exceed payment_params.max_total_cltv_expiry_delta for small values?

[tnull]

Hum, I'm not entirely sure if I'm understanding you correctly. The wrapping_sub should actually never wrap due to x >= x mod y. I made this a bit more explicit by using saturating_sub in 0b777cb.
Moreover, the line prior ensures that max_path_offset is never larger than max_total_cltv_expiry_delta and all changes from there only ever would reduce it further.

[tnull]

Rebased.

[tnull]

This PR is an attempt to address #158.

In order to generate a plausible CLTV delta offset, a three-hop random walk starting from the payee is conducted and cltv_expiry_delta values are aggregated. This offset is then added to all but the final hop of the selected paths (since the last hop knows anyways that it is the recipient).

Tests are currently failing since they of course do not expect the randomly added offset. I'll fix them when someone had a look at the implementation itself.

[TheBlueMatt]

Sorry for the delay on this one! A few comments, don't worry about the failing tests for now, I can help out with those if needed.

[tnull]

Addressed comments and rebased off a1fedea.

[ariard]

One improvement, I think if the randomly selected next hop is actually part of the extended payment path you could pick up another hop to pursue the random walk. What we would like to avoid is the random walk rolling back on the actually drawn out path. Let's say you have A -> B -> C -> D and you sort the random walk D -> C -> B -> A thus a shadow offset partially overlapping with the real cltv deltas. I would say it's a low-probability phenomena but the last publicly known node might not have numerous public channels opened for routing.

That said, it might not be worthy the effort at that point.

[tnull]

Thank you very much for having a look at this! Could you elaborate why we wouldn't want the shadow route to overlap the actual path? I see this only as a problem if we assume an on-path adversary that can say for sure that it wasn't part of the actual payment path and hence might be able to reduce the number of candidate routes. However, if we exclude overlapping paths from the beginning, we might reduce the randomness from the getgo, making any adversary's life easier?

[TheBlueMatt]

I'm not sure if its worth the effort to implement this, but I see a few concerns with overlapping paths. eg if you're an attacker and the CLTV deltas only line up with one path, and it overlaps/backtraces, you know for sure that that is shadow route, because the normal routing algorithm won't exhibit that behavior.

[tnull]

Hum, I think it may not be worth to rule out any overlap, since this could indeed reduce the variety of potential random walks quite a bit. But maybe it would be enough to ensure that we do not immediately loop back on the actual path. And in the case this excludes the only public edge we could fall back to a 'synthetic' random offset, just as in the case for fully private paths.

[TheBlueMatt]

Yea, I'll leave it up to you. I don't care too much because so many nodes use the same CLTV delta that its probably all the same in practice, but if its easy to avoid the same-node-backtrack then feel free to add it.

[tnull]

I think I'm leaning on the side of leaving this as-is for now and potentially having another look at it in a potential follow-up PR. We might want to have a follow-up anyways, for example in order to introduce some kind of value obfuscation, as noted in #158.

[ariard]

Yeah really up to you, fine to leave it as a follow-up.

[TheBlueMatt]

Looks like this needs rebase, sorry about that.

[tnull]

Looks like this needs rebase, sorry about that.

Rebased.

[TheBlueMatt]

I'm basically happy with this. There's some compilation errors due to new tests that weren't updated on rebase and the benchmark-only code needs updating, I believe. I'm happy to help resolve any test failures left after that.

[TheBlueMatt]

To make the router tests pass, maybe it'd be best to split get_route into two methods, one that gets the route and one that applies the CLTV deltas so that you don't have to update all the tests.

[tnull]

To make the router tests pass, maybe it'd be best to split get_route into two methods, one that gets the route and one that applies the CLTV deltas so that you don't have to update all the tests.

Good idea, especially given that get_route is ~1000 lines already. I now split adding the offset to its dedicated add_random_cltv_offset method and added a test.

For the time being I left random_seed_bytes as a parameter to get_route instead of removing it and all the changes to the test etc. I think it could be used to implement the 'real' random shuffle in Step (6). If that is wanted I can add this to this PR or open a follow-up.

[codecov-commenter]

Codecov Report

Merging #1286 (e92b5a7) into main (af99a94) will decrease coverage by 0.14%.
The diff coverage is 95.26%.

@@ Coverage Diff @@## main #1286 +/- ##
==========================================
- Coverage 90.75% 90.60% -0.15% 
==========================================
Files 72 72 Lines 42254 40324 -1930 ==========================================
- Hits 38347 36536 -1811 + Misses 3907 3788 -119 

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	86.92% <58.33%> (-1.22%)	⬇️
lightning/src/ln/channelmanager.rs	84.61% <87.50%> (-1.52%)	⬇️
lightning/src/routing/network_graph.rs	89.52% <88.88%> (-0.01%)	⬇️
lightning/src/routing/router.rs	92.39% <96.15%> (+0.29%)	⬆️
lightning/src/routing/scoring.rs	94.29% <96.96%> (-1.13%)	⬇️
lightning-background-processor/src/lib.rs	93.10% <100.00%> (+0.05%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.32% <100.00%> (+0.03%)	⬆️
lightning/src/ln/functional_tests.rs	97.13% <100.00%> (+0.36%)	⬆️
lightning/src/ln/onion_route_tests.rs	97.62% <100.00%> (ø)
lightning/src/ln/payment_tests.rs	99.15% <100.00%> (+<0.01%)	⬆️
... and 8 more

---

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 1a73449...e92b5a7. Read the comment docs.

[TheBlueMatt]

Feel free to squash the commits down to one or two, note that we'd ideally like to ensure all code compiles and all tests pass after each commit.

[tnull]

Feel free to squash the commits down to one or two, note that we'd ideally like to ensure all code compiles and all tests pass after each commit.

Sure sure, just wanted to avoid squashing too early. Rebased and squashed with 67137c6.

[TheBlueMatt]

Some style nits and minor test comments.

[TheBlueMatt]

As is the lock is held through find_route which we maybe don't want. Instead, we'll need an explicit {} scope to tell the compiler where to drop the locked_random_seed_bytes

[jkczyz]

Should we consider parameterizing DefaultRouter by K: Deref where K::Target: KeysInterface and call get_secure_random_bytes each time? I suppose the interface is too broad, but I wonder if a narrower Randomness interface is warranted. No strong opinion here, but it would avoid the mutex.

[tnull]

I now went the scoping route with 386a0b5. However, I agree that introducing a Randomness interface (not only here) would be a nicer abstraction than pushing around arrays with random_seed_bytes. Possibly a good follow-up PR?

[TheBlueMatt]

I suppose we could, yea, agree we'd want to do it in a followup, though. I'm a little hesitant to replace a trivial parameter with a full interface, though, as it just seems like overkill, but we can discuss that later.

[jkczyz]

Should we consider parameterizing DefaultRouter by K: Deref where K::Target: KeysInterface and call get_secure_random_bytes each time? I suppose the interface is too broad, but I wonder if a narrower Randomness interface is warranted. No strong opinion here, but it would avoid the mutex.

[ariard]

Good with the new approach of only offsetting the final hop.
(Thought the old one had the benefit to also mask the sender topology as the intermediate node couldn't deduce from their CLTV deltas the distance from sender, I believe)

[tnull]

Thanks for all the great feedback, I'll take it into account going forward!

[TheBlueMatt]

Shouldn't this be unwrap_or(0)? If our path expiry is more than the max already we probably shouldn't add an offset at all? Also, were we planning on limiting the total CLTV during the get_route run to give us a little headroom here?

[tnull]

Well my thinking here was that since we limit the route overall with max_total_cltv_expiry_delta, this subtraction should never fail. If however something unexpected were to happen and it would fail, we might be better off to default to provide some privacy (capped at 3*144), instead of none. But it can of course also default to 0, I have no strong preference here.

As for the considering the offset in get_route: after looking at the data I found that there were basically no cases in which there was not ample room to add the offset and hence thought it would be cleaner not to interfere with pathfinding and keep the two methods entirely separate. However, if we still want to take some of the offset into account, I can of course go back to, for example, subtracting 2*40 or 2*144 from the limit during path finding.

[TheBlueMatt]

Well my thinking here was that since we limit the route overall with max_total_cltv_expiry_delta, this subtraction should never fail.

Right, sorry, indeed.

If however something unexpected were to happen and it would fail, we might be better off to default to provide some privacy (capped at 3*144), instead of none.

🤷‍♂️ I guess we shouldn't ever violate the user-provided value ever?

As for the considering the offset in get_route: #1286 (comment) I found that there were basically no cases in which there was not ample room to add the offset and hence thought it would be cleaner not to interfere with pathfinding and keep the two methods entirely separate.

Hmm, I feel like that's a better reason to interfere than to not interfere.

I'm still a bit confused by this hunk here - why do we do the subtraction at max_path_offset.checked_sub(max_path_offset.wrapping_rem(40))?

[tnull]

🤷‍♂️ I guess we shouldn't ever violate the user-provided value ever?
...
Hmm, I feel like that's a better reason to interfere than to not interfere.

Alright, both addressed by f7be9da.

I'm still a bit confused by this hunk here - why do we do the subtraction at max_path_offset.checked_sub(max_path_offset.wrapping_rem(40))?

The difference of the path total to max_total_cltv_expiry_delta may be a pretty uncommon value, which could result in an implausible shadow route offset. The thinking here therefore was to choose the path offset to be the closest multiple of 40 to increase plausibility. I now had another look at this part and added a comment to make this more transparent in f7be9da.

[tnull]

Rebased and squashed once more.

[jkczyz]

random_seed_bytes is not needed here since it is only used in add_random_cltv_offset.

[tnull]

This is correct. If however possible, I’d like to leave it in get_route for now and use it to implement the 'real random shuffle' TODO in a follow-up PR. I now prefixed it with an underscore to disable the warning, but let me know if you'd rather keep it out of get_route altogether in this PR.

[jurvis]

hey @tnull is it okay if I take a stab at implementing the real random shuffle? I believe it may be related to this issue #869

[tnull]

hey @tnull is it okay if I take a stab at implementing the real random shuffle? I believe it may be related to this issue #869

Ah, thank you for the interest! This issue indeed seems related. However, I’m already mostly done with a first draft and plan to open a PR in the next few days.

[jkczyz]

If the subtraction wraps, shadow_ctlv_expiry_delta_offset would not likely change, IIUC. Could it then possibly exceed payment_params.max_total_cltv_expiry_delta for small values?

[tnull]

Hum, I'm not entirely sure if I'm understanding you correctly. The wrapping_sub should actually never wrap due to x >= x mod y. I made this a bit more explicit by using saturating_sub in 0b777cb.
Moreover, the line prior ensures that max_path_offset is never larger than max_total_cltv_expiry_delta and all changes from there only ever would reduce it further.

[tnull]

Rebased.

[tnull]

This PR is an attempt to address #158.

In order to generate a plausible CLTV delta offset, a three-hop random walk starting from the payee is conducted and cltv_expiry_delta values are aggregated. This offset is then added to all but the final hop of the selected paths (since the last hop knows anyways that it is the recipient).

Tests are currently failing since they of course do not expect the randomly added offset. I'll fix them when someone had a look at the implementation itself.

[TheBlueMatt]

Sorry for the delay on this one! A few comments, don't worry about the failing tests for now, I can help out with those if needed.

[tnull]

Addressed comments and rebased off a1fedea.

[ariard]

One improvement, I think if the randomly selected next hop is actually part of the extended payment path you could pick up another hop to pursue the random walk. What we would like to avoid is the random walk rolling back on the actually drawn out path. Let's say you have A -> B -> C -> D and you sort the random walk D -> C -> B -> A thus a shadow offset partially overlapping with the real cltv deltas. I would say it's a low-probability phenomena but the last publicly known node might not have numerous public channels opened for routing.

That said, it might not be worthy the effort at that point.

[tnull]

Thank you very much for having a look at this! Could you elaborate why we wouldn't want the shadow route to overlap the actual path? I see this only as a problem if we assume an on-path adversary that can say for sure that it wasn't part of the actual payment path and hence might be able to reduce the number of candidate routes. However, if we exclude overlapping paths from the beginning, we might reduce the randomness from the getgo, making any adversary's life easier?

[TheBlueMatt]

I'm not sure if its worth the effort to implement this, but I see a few concerns with overlapping paths. eg if you're an attacker and the CLTV deltas only line up with one path, and it overlaps/backtraces, you know for sure that that is shadow route, because the normal routing algorithm won't exhibit that behavior.

[tnull]

Hum, I think it may not be worth to rule out any overlap, since this could indeed reduce the variety of potential random walks quite a bit. But maybe it would be enough to ensure that we do not immediately loop back on the actual path. And in the case this excludes the only public edge we could fall back to a 'synthetic' random offset, just as in the case for fully private paths.

[TheBlueMatt]

Yea, I'll leave it up to you. I don't care too much because so many nodes use the same CLTV delta that its probably all the same in practice, but if its easy to avoid the same-node-backtrack then feel free to add it.

[tnull]

I think I'm leaning on the side of leaving this as-is for now and potentially having another look at it in a potential follow-up PR. We might want to have a follow-up anyways, for example in order to introduce some kind of value obfuscation, as noted in #158.

[ariard]

Yeah really up to you, fine to leave it as a follow-up.

[TheBlueMatt]

Looks like this needs rebase, sorry about that.

[tnull]

Looks like this needs rebase, sorry about that.

Rebased.

[TheBlueMatt]

I'm basically happy with this. There's some compilation errors due to new tests that weren't updated on rebase and the benchmark-only code needs updating, I believe. I'm happy to help resolve any test failures left after that.

[TheBlueMatt]

To make the router tests pass, maybe it'd be best to split get_route into two methods, one that gets the route and one that applies the CLTV deltas so that you don't have to update all the tests.

[tnull]

To make the router tests pass, maybe it'd be best to split get_route into two methods, one that gets the route and one that applies the CLTV deltas so that you don't have to update all the tests.

Good idea, especially given that get_route is ~1000 lines already. I now split adding the offset to its dedicated add_random_cltv_offset method and added a test.

For the time being I left random_seed_bytes as a parameter to get_route instead of removing it and all the changes to the test etc. I think it could be used to implement the 'real' random shuffle in Step (6). If that is wanted I can add this to this PR or open a follow-up.

[codecov-commenter]

Codecov Report

Merging #1286 (e92b5a7) into main (af99a94) will decrease coverage by 0.14%.
The diff coverage is 95.26%.

@@ Coverage Diff @@## main #1286 +/- ##
==========================================
- Coverage 90.75% 90.60% -0.15% 
==========================================
Files 72 72 Lines 42254 40324 -1930 ==========================================
- Hits 38347 36536 -1811 + Misses 3907 3788 -119 

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	86.92% <58.33%> (-1.22%)	⬇️
lightning/src/ln/channelmanager.rs	84.61% <87.50%> (-1.52%)	⬇️
lightning/src/routing/network_graph.rs	89.52% <88.88%> (-0.01%)	⬇️
lightning/src/routing/router.rs	92.39% <96.15%> (+0.29%)	⬆️
lightning/src/routing/scoring.rs	94.29% <96.96%> (-1.13%)	⬇️
lightning-background-processor/src/lib.rs	93.10% <100.00%> (+0.05%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.32% <100.00%> (+0.03%)	⬆️
lightning/src/ln/functional_tests.rs	97.13% <100.00%> (+0.36%)	⬆️
lightning/src/ln/onion_route_tests.rs	97.62% <100.00%> (ø)
lightning/src/ln/payment_tests.rs	99.15% <100.00%> (+<0.01%)	⬆️
... and 8 more

---

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 1a73449...e92b5a7. Read the comment docs.

[TheBlueMatt]

Feel free to squash the commits down to one or two, note that we'd ideally like to ensure all code compiles and all tests pass after each commit.

[tnull]

Feel free to squash the commits down to one or two, note that we'd ideally like to ensure all code compiles and all tests pass after each commit.

Sure sure, just wanted to avoid squashing too early. Rebased and squashed with 67137c6.

[TheBlueMatt]

Some style nits and minor test comments.

[TheBlueMatt]

As is the lock is held through find_route which we maybe don't want. Instead, we'll need an explicit {} scope to tell the compiler where to drop the locked_random_seed_bytes

[jkczyz]

Should we consider parameterizing DefaultRouter by K: Deref where K::Target: KeysInterface and call get_secure_random_bytes each time? I suppose the interface is too broad, but I wonder if a narrower Randomness interface is warranted. No strong opinion here, but it would avoid the mutex.

[tnull]

I now went the scoping route with 386a0b5. However, I agree that introducing a Randomness interface (not only here) would be a nicer abstraction than pushing around arrays with random_seed_bytes. Possibly a good follow-up PR?

[TheBlueMatt]

I suppose we could, yea, agree we'd want to do it in a followup, though. I'm a little hesitant to replace a trivial parameter with a full interface, though, as it just seems like overkill, but we can discuss that later.

[jkczyz]

Should we consider parameterizing DefaultRouter by K: Deref where K::Target: KeysInterface and call get_secure_random_bytes each time? I suppose the interface is too broad, but I wonder if a narrower Randomness interface is warranted. No strong opinion here, but it would avoid the mutex.

[ariard]

Good with the new approach of only offsetting the final hop.
(Thought the old one had the benefit to also mask the sender topology as the intermediate node couldn't deduce from their CLTV deltas the distance from sender, I believe)

[tnull]

Thanks for all the great feedback, I'll take it into account going forward!

[TheBlueMatt]

Shouldn't this be unwrap_or(0)? If our path expiry is more than the max already we probably shouldn't add an offset at all? Also, were we planning on limiting the total CLTV during the get_route run to give us a little headroom here?

[tnull]

Well my thinking here was that since we limit the route overall with max_total_cltv_expiry_delta, this subtraction should never fail. If however something unexpected were to happen and it would fail, we might be better off to default to provide some privacy (capped at 3*144), instead of none. But it can of course also default to 0, I have no strong preference here.

As for the considering the offset in get_route: after looking at the data I found that there were basically no cases in which there was not ample room to add the offset and hence thought it would be cleaner not to interfere with pathfinding and keep the two methods entirely separate. However, if we still want to take some of the offset into account, I can of course go back to, for example, subtracting 2*40 or 2*144 from the limit during path finding.

[TheBlueMatt]

Well my thinking here was that since we limit the route overall with max_total_cltv_expiry_delta, this subtraction should never fail.

Right, sorry, indeed.

If however something unexpected were to happen and it would fail, we might be better off to default to provide some privacy (capped at 3*144), instead of none.

🤷‍♂️ I guess we shouldn't ever violate the user-provided value ever?

As for the considering the offset in get_route: #1286 (comment) I found that there were basically no cases in which there was not ample room to add the offset and hence thought it would be cleaner not to interfere with pathfinding and keep the two methods entirely separate.

Hmm, I feel like that's a better reason to interfere than to not interfere.

I'm still a bit confused by this hunk here - why do we do the subtraction at max_path_offset.checked_sub(max_path_offset.wrapping_rem(40))?

[tnull]

🤷‍♂️ I guess we shouldn't ever violate the user-provided value ever?
...
Hmm, I feel like that's a better reason to interfere than to not interfere.

Alright, both addressed by f7be9da.

I'm still a bit confused by this hunk here - why do we do the subtraction at max_path_offset.checked_sub(max_path_offset.wrapping_rem(40))?

The difference of the path total to max_total_cltv_expiry_delta may be a pretty uncommon value, which could result in an implausible shadow route offset. The thinking here therefore was to choose the path offset to be the closest multiple of 40 to increase plausibility. I now had another look at this part and added a comment to make this more transparent in f7be9da.

[tnull]

Rebased and squashed once more.

[jkczyz]

random_seed_bytes is not needed here since it is only used in add_random_cltv_offset.

[tnull]

This is correct. If however possible, I’d like to leave it in get_route for now and use it to implement the 'real random shuffle' TODO in a follow-up PR. I now prefixed it with an underscore to disable the warning, but let me know if you'd rather keep it out of get_route altogether in this PR.

[jurvis]

hey @tnull is it okay if I take a stab at implementing the real random shuffle? I believe it may be related to this issue #869

[tnull]

hey @tnull is it okay if I take a stab at implementing the real random shuffle? I believe it may be related to this issue #869

Ah, thank you for the interest! This issue indeed seems related. However, I’m already mostly done with a first draft and plan to open a PR in the next few days.

[jkczyz]

If the subtraction wraps, shadow_ctlv_expiry_delta_offset would not likely change, IIUC. Could it then possibly exceed payment_params.max_total_cltv_expiry_delta for small values?

[tnull]

Hum, I'm not entirely sure if I'm understanding you correctly. The wrapping_sub should actually never wrap due to x >= x mod y. I made this a bit more explicit by using saturating_sub in 0b777cb.
Moreover, the line prior ensures that max_path_offset is never larger than max_total_cltv_expiry_delta and all changes from there only ever would reduce it further.

[tnull]

Rebased.

[tnull]

This PR is an attempt to address #158.

In order to generate a plausible CLTV delta offset, a three-hop random walk starting from the payee is conducted and cltv_expiry_delta values are aggregated. This offset is then added to all but the final hop of the selected paths (since the last hop knows anyways that it is the recipient).

Tests are currently failing since they of course do not expect the randomly added offset. I'll fix them when someone had a look at the implementation itself.

[TheBlueMatt]

Sorry for the delay on this one! A few comments, don't worry about the failing tests for now, I can help out with those if needed.

[tnull]

Addressed comments and rebased off a1fedea.

[ariard]

One improvement, I think if the randomly selected next hop is actually part of the extended payment path you could pick up another hop to pursue the random walk. What we would like to avoid is the random walk rolling back on the actually drawn out path. Let's say you have A -> B -> C -> D and you sort the random walk D -> C -> B -> A thus a shadow offset partially overlapping with the real cltv deltas. I would say it's a low-probability phenomena but the last publicly known node might not have numerous public channels opened for routing.

That said, it might not be worthy the effort at that point.

[tnull]

Thank you very much for having a look at this! Could you elaborate why we wouldn't want the shadow route to overlap the actual path? I see this only as a problem if we assume an on-path adversary that can say for sure that it wasn't part of the actual payment path and hence might be able to reduce the number of candidate routes. However, if we exclude overlapping paths from the beginning, we might reduce the randomness from the getgo, making any adversary's life easier?

[TheBlueMatt]

I'm not sure if its worth the effort to implement this, but I see a few concerns with overlapping paths. eg if you're an attacker and the CLTV deltas only line up with one path, and it overlaps/backtraces, you know for sure that that is shadow route, because the normal routing algorithm won't exhibit that behavior.

[tnull]

Hum, I think it may not be worth to rule out any overlap, since this could indeed reduce the variety of potential random walks quite a bit. But maybe it would be enough to ensure that we do not immediately loop back on the actual path. And in the case this excludes the only public edge we could fall back to a 'synthetic' random offset, just as in the case for fully private paths.

[TheBlueMatt]

Yea, I'll leave it up to you. I don't care too much because so many nodes use the same CLTV delta that its probably all the same in practice, but if its easy to avoid the same-node-backtrack then feel free to add it.

[tnull]

I think I'm leaning on the side of leaving this as-is for now and potentially having another look at it in a potential follow-up PR. We might want to have a follow-up anyways, for example in order to introduce some kind of value obfuscation, as noted in #158.

[ariard]

Yeah really up to you, fine to leave it as a follow-up.

[TheBlueMatt]

Looks like this needs rebase, sorry about that.

[tnull]

Looks like this needs rebase, sorry about that.

Rebased.

[TheBlueMatt]

I'm basically happy with this. There's some compilation errors due to new tests that weren't updated on rebase and the benchmark-only code needs updating, I believe. I'm happy to help resolve any test failures left after that.

[TheBlueMatt]

To make the router tests pass, maybe it'd be best to split get_route into two methods, one that gets the route and one that applies the CLTV deltas so that you don't have to update all the tests.

[tnull]

To make the router tests pass, maybe it'd be best to split get_route into two methods, one that gets the route and one that applies the CLTV deltas so that you don't have to update all the tests.

Good idea, especially given that get_route is ~1000 lines already. I now split adding the offset to its dedicated add_random_cltv_offset method and added a test.

For the time being I left random_seed_bytes as a parameter to get_route instead of removing it and all the changes to the test etc. I think it could be used to implement the 'real' random shuffle in Step (6). If that is wanted I can add this to this PR or open a follow-up.

[codecov-commenter]

Codecov Report

Merging #1286 (e92b5a7) into main (af99a94) will decrease coverage by 0.14%.
The diff coverage is 95.26%.

@@ Coverage Diff @@## main #1286 +/- ##
==========================================
- Coverage 90.75% 90.60% -0.15% 
==========================================
Files 72 72 Lines 42254 40324 -1930 ==========================================
- Hits 38347 36536 -1811 + Misses 3907 3788 -119 

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	86.92% <58.33%> (-1.22%)	⬇️
lightning/src/ln/channelmanager.rs	84.61% <87.50%> (-1.52%)	⬇️
lightning/src/routing/network_graph.rs	89.52% <88.88%> (-0.01%)	⬇️
lightning/src/routing/router.rs	92.39% <96.15%> (+0.29%)	⬆️
lightning/src/routing/scoring.rs	94.29% <96.96%> (-1.13%)	⬇️
lightning-background-processor/src/lib.rs	93.10% <100.00%> (+0.05%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.32% <100.00%> (+0.03%)	⬆️
lightning/src/ln/functional_tests.rs	97.13% <100.00%> (+0.36%)	⬆️
lightning/src/ln/onion_route_tests.rs	97.62% <100.00%> (ø)
lightning/src/ln/payment_tests.rs	99.15% <100.00%> (+<0.01%)	⬆️
... and 8 more

---

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 1a73449...e92b5a7. Read the comment docs.

[TheBlueMatt]

Feel free to squash the commits down to one or two, note that we'd ideally like to ensure all code compiles and all tests pass after each commit.

[tnull]

Feel free to squash the commits down to one or two, note that we'd ideally like to ensure all code compiles and all tests pass after each commit.

Sure sure, just wanted to avoid squashing too early. Rebased and squashed with 67137c6.

[TheBlueMatt]

Some style nits and minor test comments.

[TheBlueMatt]

As is the lock is held through find_route which we maybe don't want. Instead, we'll need an explicit {} scope to tell the compiler where to drop the locked_random_seed_bytes

[jkczyz]

Should we consider parameterizing DefaultRouter by K: Deref where K::Target: KeysInterface and call get_secure_random_bytes each time? I suppose the interface is too broad, but I wonder if a narrower Randomness interface is warranted. No strong opinion here, but it would avoid the mutex.

[tnull]

I now went the scoping route with 386a0b5. However, I agree that introducing a Randomness interface (not only here) would be a nicer abstraction than pushing around arrays with random_seed_bytes. Possibly a good follow-up PR?

[TheBlueMatt]

I suppose we could, yea, agree we'd want to do it in a followup, though. I'm a little hesitant to replace a trivial parameter with a full interface, though, as it just seems like overkill, but we can discuss that later.

[jkczyz]

Should we consider parameterizing DefaultRouter by K: Deref where K::Target: KeysInterface and call get_secure_random_bytes each time? I suppose the interface is too broad, but I wonder if a narrower Randomness interface is warranted. No strong opinion here, but it would avoid the mutex.

[ariard]

Good with the new approach of only offsetting the final hop.
(Thought the old one had the benefit to also mask the sender topology as the intermediate node couldn't deduce from their CLTV deltas the distance from sender, I believe)

[tnull]

Thanks for all the great feedback, I'll take it into account going forward!

[TheBlueMatt]

Shouldn't this be unwrap_or(0)? If our path expiry is more than the max already we probably shouldn't add an offset at all? Also, were we planning on limiting the total CLTV during the get_route run to give us a little headroom here?

[tnull]

Well my thinking here was that since we limit the route overall with max_total_cltv_expiry_delta, this subtraction should never fail. If however something unexpected were to happen and it would fail, we might be better off to default to provide some privacy (capped at 3*144), instead of none. But it can of course also default to 0, I have no strong preference here.

As for the considering the offset in get_route: after looking at the data I found that there were basically no cases in which there was not ample room to add the offset and hence thought it would be cleaner not to interfere with pathfinding and keep the two methods entirely separate. However, if we still want to take some of the offset into account, I can of course go back to, for example, subtracting 2*40 or 2*144 from the limit during path finding.

[TheBlueMatt]

Well my thinking here was that since we limit the route overall with max_total_cltv_expiry_delta, this subtraction should never fail.

Right, sorry, indeed.

If however something unexpected were to happen and it would fail, we might be better off to default to provide some privacy (capped at 3*144), instead of none.

🤷‍♂️ I guess we shouldn't ever violate the user-provided value ever?

As for the considering the offset in get_route: #1286 (comment) I found that there were basically no cases in which there was not ample room to add the offset and hence thought it would be cleaner not to interfere with pathfinding and keep the two methods entirely separate.

Hmm, I feel like that's a better reason to interfere than to not interfere.

I'm still a bit confused by this hunk here - why do we do the subtraction at max_path_offset.checked_sub(max_path_offset.wrapping_rem(40))?

[tnull]

🤷‍♂️ I guess we shouldn't ever violate the user-provided value ever?
...
Hmm, I feel like that's a better reason to interfere than to not interfere.

Alright, both addressed by f7be9da.

I'm still a bit confused by this hunk here - why do we do the subtraction at max_path_offset.checked_sub(max_path_offset.wrapping_rem(40))?

The difference of the path total to max_total_cltv_expiry_delta may be a pretty uncommon value, which could result in an implausible shadow route offset. The thinking here therefore was to choose the path offset to be the closest multiple of 40 to increase plausibility. I now had another look at this part and added a comment to make this more transparent in f7be9da.

[tnull]

Rebased and squashed once more.

[jkczyz]

random_seed_bytes is not needed here since it is only used in add_random_cltv_offset.

[tnull]

This is correct. If however possible, I’d like to leave it in get_route for now and use it to implement the 'real random shuffle' TODO in a follow-up PR. I now prefixed it with an underscore to disable the warning, but let me know if you'd rather keep it out of get_route altogether in this PR.

[jurvis]

hey @tnull is it okay if I take a stab at implementing the real random shuffle? I believe it may be related to this issue #869

[tnull]

hey @tnull is it okay if I take a stab at implementing the real random shuffle? I believe it may be related to this issue #869

Ah, thank you for the interest! This issue indeed seems related. However, I’m already mostly done with a first draft and plan to open a PR in the next few days.

[jkczyz]

If the subtraction wraps, shadow_ctlv_expiry_delta_offset would not likely change, IIUC. Could it then possibly exceed payment_params.max_total_cltv_expiry_delta for small values?

[tnull]

Hum, I'm not entirely sure if I'm understanding you correctly. The wrapping_sub should actually never wrap due to x >= x mod y. I made this a bit more explicit by using saturating_sub in 0b777cb.
Moreover, the line prior ensures that max_path_offset is never larger than max_total_cltv_expiry_delta and all changes from there only ever would reduce it further.

[tnull]

Rebased.

[tnull]

This PR is an attempt to address #158.

In order to generate a plausible CLTV delta offset, a three-hop random walk starting from the payee is conducted and cltv_expiry_delta values are aggregated. This offset is then added to all but the final hop of the selected paths (since the last hop knows anyways that it is the recipient).

Tests are currently failing since they of course do not expect the randomly added offset. I'll fix them when someone had a look at the implementation itself.

[TheBlueMatt]

Sorry for the delay on this one! A few comments, don't worry about the failing tests for now, I can help out with those if needed.

[tnull]

Addressed comments and rebased off a1fedea.

[ariard]

One improvement, I think if the randomly selected next hop is actually part of the extended payment path you could pick up another hop to pursue the random walk. What we would like to avoid is the random walk rolling back on the actually drawn out path. Let's say you have A -> B -> C -> D and you sort the random walk D -> C -> B -> A thus a shadow offset partially overlapping with the real cltv deltas. I would say it's a low-probability phenomena but the last publicly known node might not have numerous public channels opened for routing.

That said, it might not be worthy the effort at that point.

[tnull]

Thank you very much for having a look at this! Could you elaborate why we wouldn't want the shadow route to overlap the actual path? I see this only as a problem if we assume an on-path adversary that can say for sure that it wasn't part of the actual payment path and hence might be able to reduce the number of candidate routes. However, if we exclude overlapping paths from the beginning, we might reduce the randomness from the getgo, making any adversary's life easier?

[TheBlueMatt]

I'm not sure if its worth the effort to implement this, but I see a few concerns with overlapping paths. eg if you're an attacker and the CLTV deltas only line up with one path, and it overlaps/backtraces, you know for sure that that is shadow route, because the normal routing algorithm won't exhibit that behavior.

[tnull]

Hum, I think it may not be worth to rule out any overlap, since this could indeed reduce the variety of potential random walks quite a bit. But maybe it would be enough to ensure that we do not immediately loop back on the actual path. And in the case this excludes the only public edge we could fall back to a 'synthetic' random offset, just as in the case for fully private paths.

[TheBlueMatt]

Yea, I'll leave it up to you. I don't care too much because so many nodes use the same CLTV delta that its probably all the same in practice, but if its easy to avoid the same-node-backtrack then feel free to add it.

[tnull]

I think I'm leaning on the side of leaving this as-is for now and potentially having another look at it in a potential follow-up PR. We might want to have a follow-up anyways, for example in order to introduce some kind of value obfuscation, as noted in #158.

[ariard]

Yeah really up to you, fine to leave it as a follow-up.

[TheBlueMatt]

Looks like this needs rebase, sorry about that.

[tnull]

Looks like this needs rebase, sorry about that.

Rebased.

[TheBlueMatt]

I'm basically happy with this. There's some compilation errors due to new tests that weren't updated on rebase and the benchmark-only code needs updating, I believe. I'm happy to help resolve any test failures left after that.

[TheBlueMatt]

To make the router tests pass, maybe it'd be best to split get_route into two methods, one that gets the route and one that applies the CLTV deltas so that you don't have to update all the tests.

[tnull]

To make the router tests pass, maybe it'd be best to split get_route into two methods, one that gets the route and one that applies the CLTV deltas so that you don't have to update all the tests.

Good idea, especially given that get_route is ~1000 lines already. I now split adding the offset to its dedicated add_random_cltv_offset method and added a test.

For the time being I left random_seed_bytes as a parameter to get_route instead of removing it and all the changes to the test etc. I think it could be used to implement the 'real' random shuffle in Step (6). If that is wanted I can add this to this PR or open a follow-up.

[codecov-commenter]

Codecov Report

Merging #1286 (e92b5a7) into main (af99a94) will decrease coverage by 0.14%.
The diff coverage is 95.26%.

@@ Coverage Diff @@## main #1286 +/- ##
==========================================
- Coverage 90.75% 90.60% -0.15% 
==========================================
Files 72 72 Lines 42254 40324 -1930 ==========================================
- Hits 38347 36536 -1811 + Misses 3907 3788 -119 

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	86.92% <58.33%> (-1.22%)	⬇️
lightning/src/ln/channelmanager.rs	84.61% <87.50%> (-1.52%)	⬇️
lightning/src/routing/network_graph.rs	89.52% <88.88%> (-0.01%)	⬇️
lightning/src/routing/router.rs	92.39% <96.15%> (+0.29%)	⬆️
lightning/src/routing/scoring.rs	94.29% <96.96%> (-1.13%)	⬇️
lightning-background-processor/src/lib.rs	93.10% <100.00%> (+0.05%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.32% <100.00%> (+0.03%)	⬆️
lightning/src/ln/functional_tests.rs	97.13% <100.00%> (+0.36%)	⬆️
lightning/src/ln/onion_route_tests.rs	97.62% <100.00%> (ø)
lightning/src/ln/payment_tests.rs	99.15% <100.00%> (+<0.01%)	⬆️
... and 8 more

---

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 1a73449...e92b5a7. Read the comment docs.

[TheBlueMatt]

Feel free to squash the commits down to one or two, note that we'd ideally like to ensure all code compiles and all tests pass after each commit.

[tnull]

Feel free to squash the commits down to one or two, note that we'd ideally like to ensure all code compiles and all tests pass after each commit.

Sure sure, just wanted to avoid squashing too early. Rebased and squashed with 67137c6.

[TheBlueMatt]

Some style nits and minor test comments.

[TheBlueMatt]

As is the lock is held through find_route which we maybe don't want. Instead, we'll need an explicit {} scope to tell the compiler where to drop the locked_random_seed_bytes

[jkczyz]

Should we consider parameterizing DefaultRouter by K: Deref where K::Target: KeysInterface and call get_secure_random_bytes each time? I suppose the interface is too broad, but I wonder if a narrower Randomness interface is warranted. No strong opinion here, but it would avoid the mutex.

[tnull]

I now went the scoping route with 386a0b5. However, I agree that introducing a Randomness interface (not only here) would be a nicer abstraction than pushing around arrays with random_seed_bytes. Possibly a good follow-up PR?

[TheBlueMatt]

I suppose we could, yea, agree we'd want to do it in a followup, though. I'm a little hesitant to replace a trivial parameter with a full interface, though, as it just seems like overkill, but we can discuss that later.

[jkczyz]

Should we consider parameterizing DefaultRouter by K: Deref where K::Target: KeysInterface and call get_secure_random_bytes each time? I suppose the interface is too broad, but I wonder if a narrower Randomness interface is warranted. No strong opinion here, but it would avoid the mutex.

[ariard]

Good with the new approach of only offsetting the final hop.
(Thought the old one had the benefit to also mask the sender topology as the intermediate node couldn't deduce from their CLTV deltas the distance from sender, I believe)

[tnull]

Thanks for all the great feedback, I'll take it into account going forward!

[TheBlueMatt]

Shouldn't this be unwrap_or(0)? If our path expiry is more than the max already we probably shouldn't add an offset at all? Also, were we planning on limiting the total CLTV during the get_route run to give us a little headroom here?

[tnull]

Well my thinking here was that since we limit the route overall with max_total_cltv_expiry_delta, this subtraction should never fail. If however something unexpected were to happen and it would fail, we might be better off to default to provide some privacy (capped at 3*144), instead of none. But it can of course also default to 0, I have no strong preference here.

As for the considering the offset in get_route: after looking at the data I found that there were basically no cases in which there was not ample room to add the offset and hence thought it would be cleaner not to interfere with pathfinding and keep the two methods entirely separate. However, if we still want to take some of the offset into account, I can of course go back to, for example, subtracting 2*40 or 2*144 from the limit during path finding.

[TheBlueMatt]

Well my thinking here was that since we limit the route overall with max_total_cltv_expiry_delta, this subtraction should never fail.

Right, sorry, indeed.

If however something unexpected were to happen and it would fail, we might be better off to default to provide some privacy (capped at 3*144), instead of none.

🤷‍♂️ I guess we shouldn't ever violate the user-provided value ever?

As for the considering the offset in get_route: #1286 (comment) I found that there were basically no cases in which there was not ample room to add the offset and hence thought it would be cleaner not to interfere with pathfinding and keep the two methods entirely separate.

Hmm, I feel like that's a better reason to interfere than to not interfere.

I'm still a bit confused by this hunk here - why do we do the subtraction at max_path_offset.checked_sub(max_path_offset.wrapping_rem(40))?

[tnull]

🤷‍♂️ I guess we shouldn't ever violate the user-provided value ever?
...
Hmm, I feel like that's a better reason to interfere than to not interfere.

Alright, both addressed by f7be9da.

I'm still a bit confused by this hunk here - why do we do the subtraction at max_path_offset.checked_sub(max_path_offset.wrapping_rem(40))?

The difference of the path total to max_total_cltv_expiry_delta may be a pretty uncommon value, which could result in an implausible shadow route offset. The thinking here therefore was to choose the path offset to be the closest multiple of 40 to increase plausibility. I now had another look at this part and added a comment to make this more transparent in f7be9da.

[tnull]

Rebased and squashed once more.

[jkczyz]

random_seed_bytes is not needed here since it is only used in add_random_cltv_offset.

[tnull]

This is correct. If however possible, I’d like to leave it in get_route for now and use it to implement the 'real random shuffle' TODO in a follow-up PR. I now prefixed it with an underscore to disable the warning, but let me know if you'd rather keep it out of get_route altogether in this PR.

[jurvis]

hey @tnull is it okay if I take a stab at implementing the real random shuffle? I believe it may be related to this issue #869

[tnull]

hey @tnull is it okay if I take a stab at implementing the real random shuffle? I believe it may be related to this issue #869

Ah, thank you for the interest! This issue indeed seems related. However, I’m already mostly done with a first draft and plan to open a PR in the next few days.

[jkczyz]

If the subtraction wraps, shadow_ctlv_expiry_delta_offset would not likely change, IIUC. Could it then possibly exceed payment_params.max_total_cltv_expiry_delta for small values?

[tnull]

Hum, I'm not entirely sure if I'm understanding you correctly. The wrapping_sub should actually never wrap due to x >= x mod y. I made this a bit more explicit by using saturating_sub in 0b777cb.
Moreover, the line prior ensures that max_path_offset is never larger than max_total_cltv_expiry_delta and all changes from there only ever would reduce it further.

[tnull]

Rebased.

[tnull]

This PR is an attempt to address #158.

In order to generate a plausible CLTV delta offset, a three-hop random walk starting from the payee is conducted and cltv_expiry_delta values are aggregated. This offset is then added to all but the final hop of the selected paths (since the last hop knows anyways that it is the recipient).

Tests are currently failing since they of course do not expect the randomly added offset. I'll fix them when someone had a look at the implementation itself.

[TheBlueMatt]

Sorry for the delay on this one! A few comments, don't worry about the failing tests for now, I can help out with those if needed.

[tnull]

Addressed comments and rebased off a1fedea.

[ariard]

One improvement, I think if the randomly selected next hop is actually part of the extended payment path you could pick up another hop to pursue the random walk. What we would like to avoid is the random walk rolling back on the actually drawn out path. Let's say you have A -> B -> C -> D and you sort the random walk D -> C -> B -> A thus a shadow offset partially overlapping with the real cltv deltas. I would say it's a low-probability phenomena but the last publicly known node might not have numerous public channels opened for routing.

That said, it might not be worthy the effort at that point.

[tnull]

Thank you very much for having a look at this! Could you elaborate why we wouldn't want the shadow route to overlap the actual path? I see this only as a problem if we assume an on-path adversary that can say for sure that it wasn't part of the actual payment path and hence might be able to reduce the number of candidate routes. However, if we exclude overlapping paths from the beginning, we might reduce the randomness from the getgo, making any adversary's life easier?

[TheBlueMatt]

I'm not sure if its worth the effort to implement this, but I see a few concerns with overlapping paths. eg if you're an attacker and the CLTV deltas only line up with one path, and it overlaps/backtraces, you know for sure that that is shadow route, because the normal routing algorithm won't exhibit that behavior.

[tnull]

Hum, I think it may not be worth to rule out any overlap, since this could indeed reduce the variety of potential random walks quite a bit. But maybe it would be enough to ensure that we do not immediately loop back on the actual path. And in the case this excludes the only public edge we could fall back to a 'synthetic' random offset, just as in the case for fully private paths.

[TheBlueMatt]

Yea, I'll leave it up to you. I don't care too much because so many nodes use the same CLTV delta that its probably all the same in practice, but if its easy to avoid the same-node-backtrack then feel free to add it.

[tnull]

I think I'm leaning on the side of leaving this as-is for now and potentially having another look at it in a potential follow-up PR. We might want to have a follow-up anyways, for example in order to introduce some kind of value obfuscation, as noted in #158.

[ariard]

Yeah really up to you, fine to leave it as a follow-up.

[TheBlueMatt]

Looks like this needs rebase, sorry about that.

[tnull]

Looks like this needs rebase, sorry about that.

Rebased.

[TheBlueMatt]

I'm basically happy with this. There's some compilation errors due to new tests that weren't updated on rebase and the benchmark-only code needs updating, I believe. I'm happy to help resolve any test failures left after that.

[TheBlueMatt]

To make the router tests pass, maybe it'd be best to split get_route into two methods, one that gets the route and one that applies the CLTV deltas so that you don't have to update all the tests.

[tnull]

To make the router tests pass, maybe it'd be best to split get_route into two methods, one that gets the route and one that applies the CLTV deltas so that you don't have to update all the tests.

Good idea, especially given that get_route is ~1000 lines already. I now split adding the offset to its dedicated add_random_cltv_offset method and added a test.

For the time being I left random_seed_bytes as a parameter to get_route instead of removing it and all the changes to the test etc. I think it could be used to implement the 'real' random shuffle in Step (6). If that is wanted I can add this to this PR or open a follow-up.

[codecov-commenter]

Codecov Report

Merging #1286 (e92b5a7) into main (af99a94) will decrease coverage by 0.14%.
The diff coverage is 95.26%.

@@ Coverage Diff @@## main #1286 +/- ##
==========================================
- Coverage 90.75% 90.60% -0.15% 
==========================================
Files 72 72 Lines 42254 40324 -1930 ==========================================
- Hits 38347 36536 -1811 + Misses 3907 3788 -119 

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	86.92% <58.33%> (-1.22%)	⬇️
lightning/src/ln/channelmanager.rs	84.61% <87.50%> (-1.52%)	⬇️
lightning/src/routing/network_graph.rs	89.52% <88.88%> (-0.01%)	⬇️
lightning/src/routing/router.rs	92.39% <96.15%> (+0.29%)	⬆️
lightning/src/routing/scoring.rs	94.29% <96.96%> (-1.13%)	⬇️
lightning-background-processor/src/lib.rs	93.10% <100.00%> (+0.05%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.32% <100.00%> (+0.03%)	⬆️
lightning/src/ln/functional_tests.rs	97.13% <100.00%> (+0.36%)	⬆️
lightning/src/ln/onion_route_tests.rs	97.62% <100.00%> (ø)
lightning/src/ln/payment_tests.rs	99.15% <100.00%> (+<0.01%)	⬆️
... and 8 more

---

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 1a73449...e92b5a7. Read the comment docs.

[TheBlueMatt]

Feel free to squash the commits down to one or two, note that we'd ideally like to ensure all code compiles and all tests pass after each commit.

[tnull]

Feel free to squash the commits down to one or two, note that we'd ideally like to ensure all code compiles and all tests pass after each commit.

Sure sure, just wanted to avoid squashing too early. Rebased and squashed with 67137c6.

[TheBlueMatt]

Some style nits and minor test comments.

[TheBlueMatt]

As is the lock is held through find_route which we maybe don't want. Instead, we'll need an explicit {} scope to tell the compiler where to drop the locked_random_seed_bytes

[jkczyz]

Should we consider parameterizing DefaultRouter by K: Deref where K::Target: KeysInterface and call get_secure_random_bytes each time? I suppose the interface is too broad, but I wonder if a narrower Randomness interface is warranted. No strong opinion here, but it would avoid the mutex.

[tnull]

I now went the scoping route with 386a0b5. However, I agree that introducing a Randomness interface (not only here) would be a nicer abstraction than pushing around arrays with random_seed_bytes. Possibly a good follow-up PR?

[TheBlueMatt]

I suppose we could, yea, agree we'd want to do it in a followup, though. I'm a little hesitant to replace a trivial parameter with a full interface, though, as it just seems like overkill, but we can discuss that later.

[jkczyz]

Should we consider parameterizing DefaultRouter by K: Deref where K::Target: KeysInterface and call get_secure_random_bytes each time? I suppose the interface is too broad, but I wonder if a narrower Randomness interface is warranted. No strong opinion here, but it would avoid the mutex.

[ariard]

Good with the new approach of only offsetting the final hop.
(Thought the old one had the benefit to also mask the sender topology as the intermediate node couldn't deduce from their CLTV deltas the distance from sender, I believe)

[tnull]

Thanks for all the great feedback, I'll take it into account going forward!

[TheBlueMatt]

Shouldn't this be unwrap_or(0)? If our path expiry is more than the max already we probably shouldn't add an offset at all? Also, were we planning on limiting the total CLTV during the get_route run to give us a little headroom here?

[tnull]

Well my thinking here was that since we limit the route overall with max_total_cltv_expiry_delta, this subtraction should never fail. If however something unexpected were to happen and it would fail, we might be better off to default to provide some privacy (capped at 3*144), instead of none. But it can of course also default to 0, I have no strong preference here.

As for the considering the offset in get_route: after looking at the data I found that there were basically no cases in which there was not ample room to add the offset and hence thought it would be cleaner not to interfere with pathfinding and keep the two methods entirely separate. However, if we still want to take some of the offset into account, I can of course go back to, for example, subtracting 2*40 or 2*144 from the limit during path finding.

[TheBlueMatt]

Well my thinking here was that since we limit the route overall with max_total_cltv_expiry_delta, this subtraction should never fail.

Right, sorry, indeed.

If however something unexpected were to happen and it would fail, we might be better off to default to provide some privacy (capped at 3*144), instead of none.

🤷‍♂️ I guess we shouldn't ever violate the user-provided value ever?

As for the considering the offset in get_route: #1286 (comment) I found that there were basically no cases in which there was not ample room to add the offset and hence thought it would be cleaner not to interfere with pathfinding and keep the two methods entirely separate.

Hmm, I feel like that's a better reason to interfere than to not interfere.

I'm still a bit confused by this hunk here - why do we do the subtraction at max_path_offset.checked_sub(max_path_offset.wrapping_rem(40))?

[tnull]

🤷‍♂️ I guess we shouldn't ever violate the user-provided value ever?
...
Hmm, I feel like that's a better reason to interfere than to not interfere.

Alright, both addressed by f7be9da.

I'm still a bit confused by this hunk here - why do we do the subtraction at max_path_offset.checked_sub(max_path_offset.wrapping_rem(40))?

The difference of the path total to max_total_cltv_expiry_delta may be a pretty uncommon value, which could result in an implausible shadow route offset. The thinking here therefore was to choose the path offset to be the closest multiple of 40 to increase plausibility. I now had another look at this part and added a comment to make this more transparent in f7be9da.

[tnull]

Rebased and squashed once more.

[jkczyz]

random_seed_bytes is not needed here since it is only used in add_random_cltv_offset.

[tnull]

This is correct. If however possible, I’d like to leave it in get_route for now and use it to implement the 'real random shuffle' TODO in a follow-up PR. I now prefixed it with an underscore to disable the warning, but let me know if you'd rather keep it out of get_route altogether in this PR.

[jurvis]

hey @tnull is it okay if I take a stab at implementing the real random shuffle? I believe it may be related to this issue #869

[tnull]

hey @tnull is it okay if I take a stab at implementing the real random shuffle? I believe it may be related to this issue #869

Ah, thank you for the interest! This issue indeed seems related. However, I’m already mostly done with a first draft and plan to open a PR in the next few days.

[jkczyz]

If the subtraction wraps, shadow_ctlv_expiry_delta_offset would not likely change, IIUC. Could it then possibly exceed payment_params.max_total_cltv_expiry_delta for small values?

[tnull]

Hum, I'm not entirely sure if I'm understanding you correctly. The wrapping_sub should actually never wrap due to x >= x mod y. I made this a bit more explicit by using saturating_sub in 0b777cb.
Moreover, the line prior ensures that max_path_offset is never larger than max_total_cltv_expiry_delta and all changes from there only ever would reduce it further.

[tnull]

Rebased.

[tnull]

This PR is an attempt to address #158.

In order to generate a plausible CLTV delta offset, a three-hop random walk starting from the payee is conducted and cltv_expiry_delta values are aggregated. This offset is then added to all but the final hop of the selected paths (since the last hop knows anyways that it is the recipient).

Tests are currently failing since they of course do not expect the randomly added offset. I'll fix them when someone had a look at the implementation itself.

[TheBlueMatt]

Sorry for the delay on this one! A few comments, don't worry about the failing tests for now, I can help out with those if needed.

[tnull]

Addressed comments and rebased off a1fedea.

[ariard]

One improvement, I think if the randomly selected next hop is actually part of the extended payment path you could pick up another hop to pursue the random walk. What we would like to avoid is the random walk rolling back on the actually drawn out path. Let's say you have A -> B -> C -> D and you sort the random walk D -> C -> B -> A thus a shadow offset partially overlapping with the real cltv deltas. I would say it's a low-probability phenomena but the last publicly known node might not have numerous public channels opened for routing.

That said, it might not be worthy the effort at that point.

[tnull]

Thank you very much for having a look at this! Could you elaborate why we wouldn't want the shadow route to overlap the actual path? I see this only as a problem if we assume an on-path adversary that can say for sure that it wasn't part of the actual payment path and hence might be able to reduce the number of candidate routes. However, if we exclude overlapping paths from the beginning, we might reduce the randomness from the getgo, making any adversary's life easier?

[TheBlueMatt]

I'm not sure if its worth the effort to implement this, but I see a few concerns with overlapping paths. eg if you're an attacker and the CLTV deltas only line up with one path, and it overlaps/backtraces, you know for sure that that is shadow route, because the normal routing algorithm won't exhibit that behavior.

[tnull]

Hum, I think it may not be worth to rule out any overlap, since this could indeed reduce the variety of potential random walks quite a bit. But maybe it would be enough to ensure that we do not immediately loop back on the actual path. And in the case this excludes the only public edge we could fall back to a 'synthetic' random offset, just as in the case for fully private paths.

[TheBlueMatt]

Yea, I'll leave it up to you. I don't care too much because so many nodes use the same CLTV delta that its probably all the same in practice, but if its easy to avoid the same-node-backtrack then feel free to add it.

[tnull]

I think I'm leaning on the side of leaving this as-is for now and potentially having another look at it in a potential follow-up PR. We might want to have a follow-up anyways, for example in order to introduce some kind of value obfuscation, as noted in #158.

[ariard]

Yeah really up to you, fine to leave it as a follow-up.

[TheBlueMatt]

Looks like this needs rebase, sorry about that.

[tnull]

Looks like this needs rebase, sorry about that.

Rebased.

[TheBlueMatt]

I'm basically happy with this. There's some compilation errors due to new tests that weren't updated on rebase and the benchmark-only code needs updating, I believe. I'm happy to help resolve any test failures left after that.

[TheBlueMatt]

To make the router tests pass, maybe it'd be best to split get_route into two methods, one that gets the route and one that applies the CLTV deltas so that you don't have to update all the tests.

[tnull]

To make the router tests pass, maybe it'd be best to split get_route into two methods, one that gets the route and one that applies the CLTV deltas so that you don't have to update all the tests.

Good idea, especially given that get_route is ~1000 lines already. I now split adding the offset to its dedicated add_random_cltv_offset method and added a test.

For the time being I left random_seed_bytes as a parameter to get_route instead of removing it and all the changes to the test etc. I think it could be used to implement the 'real' random shuffle in Step (6). If that is wanted I can add this to this PR or open a follow-up.

[codecov-commenter]

Codecov Report

Merging #1286 (e92b5a7) into main (af99a94) will decrease coverage by 0.14%.
The diff coverage is 95.26%.

@@ Coverage Diff @@## main #1286 +/- ##
==========================================
- Coverage 90.75% 90.60% -0.15% 
==========================================
Files 72 72 Lines 42254 40324 -1930 ==========================================
- Hits 38347 36536 -1811 + Misses 3907 3788 -119 

Impacted Files	Coverage Δ
lightning-invoice/src/utils.rs	86.92% <58.33%> (-1.22%)	⬇️
lightning/src/ln/channelmanager.rs	84.61% <87.50%> (-1.52%)	⬇️
lightning/src/routing/network_graph.rs	89.52% <88.88%> (-0.01%)	⬇️
lightning/src/routing/router.rs	92.39% <96.15%> (+0.29%)	⬆️
lightning/src/routing/scoring.rs	94.29% <96.96%> (-1.13%)	⬇️
lightning-background-processor/src/lib.rs	93.10% <100.00%> (+0.05%)	⬆️
lightning/src/ln/functional_test_utils.rs	95.32% <100.00%> (+0.03%)	⬆️
lightning/src/ln/functional_tests.rs	97.13% <100.00%> (+0.36%)	⬆️
lightning/src/ln/onion_route_tests.rs	97.62% <100.00%> (ø)
lightning/src/ln/payment_tests.rs	99.15% <100.00%> (+<0.01%)	⬆️
... and 8 more

---

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 1a73449...e92b5a7. Read the comment docs.

[TheBlueMatt]

Feel free to squash the commits down to one or two, note that we'd ideally like to ensure all code compiles and all tests pass after each commit.

[tnull]

Feel free to squash the commits down to one or two, note that we'd ideally like to ensure all code compiles and all tests pass after each commit.

Sure sure, just wanted to avoid squashing too early. Rebased and squashed with 67137c6.

[TheBlueMatt]

Some style nits and minor test comments.

[TheBlueMatt]

As is the lock is held through find_route which we maybe don't want. Instead, we'll need an explicit {} scope to tell the compiler where to drop the locked_random_seed_bytes

[jkczyz]

Should we consider parameterizing DefaultRouter by K: Deref where K::Target: KeysInterface and call get_secure_random_bytes each time? I suppose the interface is too broad, but I wonder if a narrower Randomness interface is warranted. No strong opinion here, but it would avoid the mutex.

[tnull]

I now went the scoping route with 386a0b5. However, I agree that introducing a Randomness interface (not only here) would be a nicer abstraction than pushing around arrays with random_seed_bytes. Possibly a good follow-up PR?

[TheBlueMatt]

I suppose we could, yea, agree we'd want to do it in a followup, though. I'm a little hesitant to replace a trivial parameter with a full interface, though, as it just seems like overkill, but we can discuss that later.

[jkczyz]

Should we consider parameterizing DefaultRouter by K: Deref where K::Target: KeysInterface and call get_secure_random_bytes each time? I suppose the interface is too broad, but I wonder if a narrower Randomness interface is warranted. No strong opinion here, but it would avoid the mutex.

[ariard]

Good with the new approach of only offsetting the final hop.
(Thought the old one had the benefit to also mask the sender topology as the intermediate node couldn't deduce from their CLTV deltas the distance from sender, I believe)

[tnull]

Thanks for all the great feedback, I'll take it into account going forward!

[TheBlueMatt]

Shouldn't this be unwrap_or(0)? If our path expiry is more than the max already we probably shouldn't add an offset at all? Also, were we planning on limiting the total CLTV during the get_route run to give us a little headroom here?

[tnull]

Well my thinking here was that since we limit the route overall with max_total_cltv_expiry_delta, this subtraction should never fail. If however something unexpected were to happen and it would fail, we might be better off to default to provide some privacy (capped at 3*144), instead of none. But it can of course also default to 0, I have no strong preference here.

As for the considering the offset in get_route: after looking at the data I found that there were basically no cases in which there was not ample room to add the offset and hence thought it would be cleaner not to interfere with pathfinding and keep the two methods entirely separate. However, if we still want to take some of the offset into account, I can of course go back to, for example, subtracting 2*40 or 2*144 from the limit during path finding.

[TheBlueMatt]

Well my thinking here was that since we limit the route overall with max_total_cltv_expiry_delta, this subtraction should never fail.

Right, sorry, indeed.

If however something unexpected were to happen and it would fail, we might be better off to default to provide some privacy (capped at 3*144), instead of none.

🤷‍♂️ I guess we shouldn't ever violate the user-provided value ever?

As for the considering the offset in get_route: #1286 (comment) I found that there were basically no cases in which there was not ample room to add the offset and hence thought it would be cleaner not to interfere with pathfinding and keep the two methods entirely separate.

Hmm, I feel like that's a better reason to interfere than to not interfere.

I'm still a bit confused by this hunk here - why do we do the subtraction at max_path_offset.checked_sub(max_path_offset.wrapping_rem(40))?

[tnull]

🤷‍♂️ I guess we shouldn't ever violate the user-provided value ever?
...
Hmm, I feel like that's a better reason to interfere than to not interfere.

Alright, both addressed by f7be9da.

I'm still a bit confused by this hunk here - why do we do the subtraction at max_path_offset.checked_sub(max_path_offset.wrapping_rem(40))?

The difference of the path total to max_total_cltv_expiry_delta may be a pretty uncommon value, which could result in an implausible shadow route offset. The thinking here therefore was to choose the path offset to be the closest multiple of 40 to increase plausibility. I now had another look at this part and added a comment to make this more transparent in f7be9da.

[tnull]

Rebased and squashed once more.

[jkczyz]

random_seed_bytes is not needed here since it is only used in add_random_cltv_offset.

[tnull]

This is correct. If however possible, I’d like to leave it in get_route for now and use it to implement the 'real random shuffle' TODO in a follow-up PR. I now prefixed it with an underscore to disable the warning, but let me know if you'd rather keep it out of get_route altogether in this PR.

[jurvis]

hey @tnull is it okay if I take a stab at implementing the real random shuffle? I believe it may be related to this issue #869

[tnull]

hey @tnull is it okay if I take a stab at implementing the real random shuffle? I believe it may be related to this issue #869

Ah, thank you for the interest! This issue indeed seems related. However, I’m already mostly done with a first draft and plan to open a PR in the next few days.

[jkczyz]

If the subtraction wraps, shadow_ctlv_expiry_delta_offset would not likely change, IIUC. Could it then possibly exceed payment_params.max_total_cltv_expiry_delta for small values?

[tnull]

Hum, I'm not entirely sure if I'm understanding you correctly. The wrapping_sub should actually never wrap due to x >= x mod y. I made this a bit more explicit by using saturating_sub in 0b777cb.
Moreover, the line prior ensures that max_path_offset is never larger than max_total_cltv_expiry_delta and all changes from there only ever would reduce it further.

[tnull]

Rebased.