Avoid looping CLTV shadow routes.

[tnull]

This PR is a follow-up to #1286: it avoids 'shadow route' random walks that loop back on the actual payment path, since such paths are implausible from an adversary's perspective.

With this I also simplified the code a bit and opted to always generate a random shadow route of a certain length. When no suitable offset can be retrieved from the (unvisited) public network graph, the hop offset is filled with the default of MEDIAN_HOP_CLTV_EXPIRY_DELTA == 40 to simulate a partially private path.

As however already noted in #1286, such loopless random walks tend to be less diverse. For example, in the small test scenario generated by build_graph(), this implementation now basically always uses the fallback value since it's running out of options. It should also be noted that in order not to reduce the set of candidate paths even further, I now opted to account for visited nodes on per-path basis, i.e., crossing the streams is still possible for MPPs.

[codecov-commenter]

Codecov Report

Merging #1360 (11c3120) into main (cb1d795) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1360 +/- ##
==========================================
- Coverage 90.77% 90.75% -0.02% 
==========================================
Files 73 73 Lines 41062 41069 +7 Branches 41062 41069 +7 ==========================================
+ Hits 37272 37273 +1 - Misses 3790 3796 +6 

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	92.39% <100.00%> (+0.02%)	⬆️
lightning/src/ln/functional_tests.rs	97.06% <0.00%> (-0.10%)	⬇️

---

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 cb1d795...11c3120. Read the comment docs.

[tnull]

Since the fuzzer was failing at a related point, I now rebased this off #1358.

[TheBlueMatt]

Sorry about the fuzz failure, #1358 landed now so feel free to rebase.

[tnull]

Sorry about the fuzz failure, #1358 landed now so feel free to rebase.

No apology necessary, especially in this case... Rebased on main now.

[jkczyz]

Overall looks good. Fine with squashing and merging as is. Just want to note my observation that Node::from_pubkey calls PublicKey::serialize, which allocates a Vec. So while we avoid one allocation by using an array, there is still three allocations there.

[tnull]

Rebased and squashed.

[jkczyz]

Looks like github actions are acting up. Could you run the following?

git commit --amend && git push -f

[TheBlueMatt]

Rebased and squashed.

Hmm, did you have to rebase? Please don't rebase unless there's an actual conflict as it means reviewers can't as easily compare to the previous state.

[tnull]

Looks like github actions are acting up. Could you run the following?

Done!

Hmm, did you have to rebase? Please don't rebase unless there's an actual conflict as it means reviewers can't as easily compare to the previous state.

Ah, sorry, didn't realize that. Will keep in mind.

[tnull]

This PR is a follow-up to #1286: it avoids 'shadow route' random walks that loop back on the actual payment path, since such paths are implausible from an adversary's perspective.

With this I also simplified the code a bit and opted to always generate a random shadow route of a certain length. When no suitable offset can be retrieved from the (unvisited) public network graph, the hop offset is filled with the default of MEDIAN_HOP_CLTV_EXPIRY_DELTA == 40 to simulate a partially private path.

As however already noted in #1286, such loopless random walks tend to be less diverse. For example, in the small test scenario generated by build_graph(), this implementation now basically always uses the fallback value since it's running out of options. It should also be noted that in order not to reduce the set of candidate paths even further, I now opted to account for visited nodes on per-path basis, i.e., crossing the streams is still possible for MPPs.

[codecov-commenter]

Codecov Report

Merging #1360 (11c3120) into main (cb1d795) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1360 +/- ##
==========================================
- Coverage 90.77% 90.75% -0.02% 
==========================================
Files 73 73 Lines 41062 41069 +7 Branches 41062 41069 +7 ==========================================
+ Hits 37272 37273 +1 - Misses 3790 3796 +6 

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	92.39% <100.00%> (+0.02%)	⬆️
lightning/src/ln/functional_tests.rs	97.06% <0.00%> (-0.10%)	⬇️

---

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 cb1d795...11c3120. Read the comment docs.

[tnull]

Since the fuzzer was failing at a related point, I now rebased this off #1358.

[TheBlueMatt]

Sorry about the fuzz failure, #1358 landed now so feel free to rebase.

[tnull]

Sorry about the fuzz failure, #1358 landed now so feel free to rebase.

No apology necessary, especially in this case... Rebased on main now.

[jkczyz]

Overall looks good. Fine with squashing and merging as is. Just want to note my observation that Node::from_pubkey calls PublicKey::serialize, which allocates a Vec. So while we avoid one allocation by using an array, there is still three allocations there.

[tnull]

Rebased and squashed.

[jkczyz]

Looks like github actions are acting up. Could you run the following?

git commit --amend && git push -f

[TheBlueMatt]

Rebased and squashed.

Hmm, did you have to rebase? Please don't rebase unless there's an actual conflict as it means reviewers can't as easily compare to the previous state.

[tnull]

Looks like github actions are acting up. Could you run the following?

Done!

Hmm, did you have to rebase? Please don't rebase unless there's an actual conflict as it means reviewers can't as easily compare to the previous state.

Ah, sorry, didn't realize that. Will keep in mind.

[tnull]

This PR is a follow-up to #1286: it avoids 'shadow route' random walks that loop back on the actual payment path, since such paths are implausible from an adversary's perspective.

With this I also simplified the code a bit and opted to always generate a random shadow route of a certain length. When no suitable offset can be retrieved from the (unvisited) public network graph, the hop offset is filled with the default of MEDIAN_HOP_CLTV_EXPIRY_DELTA == 40 to simulate a partially private path.

As however already noted in #1286, such loopless random walks tend to be less diverse. For example, in the small test scenario generated by build_graph(), this implementation now basically always uses the fallback value since it's running out of options. It should also be noted that in order not to reduce the set of candidate paths even further, I now opted to account for visited nodes on per-path basis, i.e., crossing the streams is still possible for MPPs.

[codecov-commenter]

Codecov Report

Merging #1360 (11c3120) into main (cb1d795) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1360 +/- ##
==========================================
- Coverage 90.77% 90.75% -0.02% 
==========================================
Files 73 73 Lines 41062 41069 +7 Branches 41062 41069 +7 ==========================================
+ Hits 37272 37273 +1 - Misses 3790 3796 +6 

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	92.39% <100.00%> (+0.02%)	⬆️
lightning/src/ln/functional_tests.rs	97.06% <0.00%> (-0.10%)	⬇️

---

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 cb1d795...11c3120. Read the comment docs.

[tnull]

Since the fuzzer was failing at a related point, I now rebased this off #1358.

[TheBlueMatt]

Sorry about the fuzz failure, #1358 landed now so feel free to rebase.

[tnull]

Sorry about the fuzz failure, #1358 landed now so feel free to rebase.

No apology necessary, especially in this case... Rebased on main now.

[jkczyz]

Overall looks good. Fine with squashing and merging as is. Just want to note my observation that Node::from_pubkey calls PublicKey::serialize, which allocates a Vec. So while we avoid one allocation by using an array, there is still three allocations there.

[tnull]

Rebased and squashed.

[jkczyz]

Looks like github actions are acting up. Could you run the following?

git commit --amend && git push -f

[TheBlueMatt]

Rebased and squashed.

Hmm, did you have to rebase? Please don't rebase unless there's an actual conflict as it means reviewers can't as easily compare to the previous state.

[tnull]

Looks like github actions are acting up. Could you run the following?

Done!

Hmm, did you have to rebase? Please don't rebase unless there's an actual conflict as it means reviewers can't as easily compare to the previous state.

Ah, sorry, didn't realize that. Will keep in mind.

[tnull]

This PR is a follow-up to #1286: it avoids 'shadow route' random walks that loop back on the actual payment path, since such paths are implausible from an adversary's perspective.

With this I also simplified the code a bit and opted to always generate a random shadow route of a certain length. When no suitable offset can be retrieved from the (unvisited) public network graph, the hop offset is filled with the default of MEDIAN_HOP_CLTV_EXPIRY_DELTA == 40 to simulate a partially private path.

As however already noted in #1286, such loopless random walks tend to be less diverse. For example, in the small test scenario generated by build_graph(), this implementation now basically always uses the fallback value since it's running out of options. It should also be noted that in order not to reduce the set of candidate paths even further, I now opted to account for visited nodes on per-path basis, i.e., crossing the streams is still possible for MPPs.

[codecov-commenter]

Codecov Report

Merging #1360 (11c3120) into main (cb1d795) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1360 +/- ##
==========================================
- Coverage 90.77% 90.75% -0.02% 
==========================================
Files 73 73 Lines 41062 41069 +7 Branches 41062 41069 +7 ==========================================
+ Hits 37272 37273 +1 - Misses 3790 3796 +6 

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	92.39% <100.00%> (+0.02%)	⬆️
lightning/src/ln/functional_tests.rs	97.06% <0.00%> (-0.10%)	⬇️

---

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 cb1d795...11c3120. Read the comment docs.

[tnull]

Since the fuzzer was failing at a related point, I now rebased this off #1358.

[TheBlueMatt]

Sorry about the fuzz failure, #1358 landed now so feel free to rebase.

[tnull]

Sorry about the fuzz failure, #1358 landed now so feel free to rebase.

No apology necessary, especially in this case... Rebased on main now.

[jkczyz]

Overall looks good. Fine with squashing and merging as is. Just want to note my observation that Node::from_pubkey calls PublicKey::serialize, which allocates a Vec. So while we avoid one allocation by using an array, there is still three allocations there.

[tnull]

Rebased and squashed.

[jkczyz]

Looks like github actions are acting up. Could you run the following?

git commit --amend && git push -f

[TheBlueMatt]

Rebased and squashed.

Hmm, did you have to rebase? Please don't rebase unless there's an actual conflict as it means reviewers can't as easily compare to the previous state.

[tnull]

Looks like github actions are acting up. Could you run the following?

Done!

Hmm, did you have to rebase? Please don't rebase unless there's an actual conflict as it means reviewers can't as easily compare to the previous state.

Ah, sorry, didn't realize that. Will keep in mind.

[tnull]

This PR is a follow-up to #1286: it avoids 'shadow route' random walks that loop back on the actual payment path, since such paths are implausible from an adversary's perspective.

With this I also simplified the code a bit and opted to always generate a random shadow route of a certain length. When no suitable offset can be retrieved from the (unvisited) public network graph, the hop offset is filled with the default of MEDIAN_HOP_CLTV_EXPIRY_DELTA == 40 to simulate a partially private path.

As however already noted in #1286, such loopless random walks tend to be less diverse. For example, in the small test scenario generated by build_graph(), this implementation now basically always uses the fallback value since it's running out of options. It should also be noted that in order not to reduce the set of candidate paths even further, I now opted to account for visited nodes on per-path basis, i.e., crossing the streams is still possible for MPPs.

[codecov-commenter]

Codecov Report

Merging #1360 (11c3120) into main (cb1d795) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1360 +/- ##
==========================================
- Coverage 90.77% 90.75% -0.02% 
==========================================
Files 73 73 Lines 41062 41069 +7 Branches 41062 41069 +7 ==========================================
+ Hits 37272 37273 +1 - Misses 3790 3796 +6 

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	92.39% <100.00%> (+0.02%)	⬆️
lightning/src/ln/functional_tests.rs	97.06% <0.00%> (-0.10%)	⬇️

---

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 cb1d795...11c3120. Read the comment docs.

[tnull]

Since the fuzzer was failing at a related point, I now rebased this off #1358.

[TheBlueMatt]

Sorry about the fuzz failure, #1358 landed now so feel free to rebase.

[tnull]

Sorry about the fuzz failure, #1358 landed now so feel free to rebase.

No apology necessary, especially in this case... Rebased on main now.

[jkczyz]

Overall looks good. Fine with squashing and merging as is. Just want to note my observation that Node::from_pubkey calls PublicKey::serialize, which allocates a Vec. So while we avoid one allocation by using an array, there is still three allocations there.

[tnull]

Rebased and squashed.

[jkczyz]

Looks like github actions are acting up. Could you run the following?

git commit --amend && git push -f

[TheBlueMatt]

Rebased and squashed.

Hmm, did you have to rebase? Please don't rebase unless there's an actual conflict as it means reviewers can't as easily compare to the previous state.

[tnull]

Looks like github actions are acting up. Could you run the following?

Done!

Hmm, did you have to rebase? Please don't rebase unless there's an actual conflict as it means reviewers can't as easily compare to the previous state.

Ah, sorry, didn't realize that. Will keep in mind.

[tnull]

This PR is a follow-up to #1286: it avoids 'shadow route' random walks that loop back on the actual payment path, since such paths are implausible from an adversary's perspective.

With this I also simplified the code a bit and opted to always generate a random shadow route of a certain length. When no suitable offset can be retrieved from the (unvisited) public network graph, the hop offset is filled with the default of MEDIAN_HOP_CLTV_EXPIRY_DELTA == 40 to simulate a partially private path.

As however already noted in #1286, such loopless random walks tend to be less diverse. For example, in the small test scenario generated by build_graph(), this implementation now basically always uses the fallback value since it's running out of options. It should also be noted that in order not to reduce the set of candidate paths even further, I now opted to account for visited nodes on per-path basis, i.e., crossing the streams is still possible for MPPs.

[codecov-commenter]

Codecov Report

Merging #1360 (11c3120) into main (cb1d795) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1360 +/- ##
==========================================
- Coverage 90.77% 90.75% -0.02% 
==========================================
Files 73 73 Lines 41062 41069 +7 Branches 41062 41069 +7 ==========================================
+ Hits 37272 37273 +1 - Misses 3790 3796 +6 

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	92.39% <100.00%> (+0.02%)	⬆️
lightning/src/ln/functional_tests.rs	97.06% <0.00%> (-0.10%)	⬇️

---

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 cb1d795...11c3120. Read the comment docs.

[tnull]

Since the fuzzer was failing at a related point, I now rebased this off #1358.

[TheBlueMatt]

Sorry about the fuzz failure, #1358 landed now so feel free to rebase.

[tnull]

Sorry about the fuzz failure, #1358 landed now so feel free to rebase.

No apology necessary, especially in this case... Rebased on main now.

[jkczyz]

Overall looks good. Fine with squashing and merging as is. Just want to note my observation that Node::from_pubkey calls PublicKey::serialize, which allocates a Vec. So while we avoid one allocation by using an array, there is still three allocations there.

[tnull]

Rebased and squashed.

[jkczyz]

Looks like github actions are acting up. Could you run the following?

git commit --amend && git push -f

[TheBlueMatt]

Rebased and squashed.

Hmm, did you have to rebase? Please don't rebase unless there's an actual conflict as it means reviewers can't as easily compare to the previous state.

[tnull]

Looks like github actions are acting up. Could you run the following?

Done!

Hmm, did you have to rebase? Please don't rebase unless there's an actual conflict as it means reviewers can't as easily compare to the previous state.

Ah, sorry, didn't realize that. Will keep in mind.

[tnull]

This PR is a follow-up to #1286: it avoids 'shadow route' random walks that loop back on the actual payment path, since such paths are implausible from an adversary's perspective.

With this I also simplified the code a bit and opted to always generate a random shadow route of a certain length. When no suitable offset can be retrieved from the (unvisited) public network graph, the hop offset is filled with the default of MEDIAN_HOP_CLTV_EXPIRY_DELTA == 40 to simulate a partially private path.

As however already noted in #1286, such loopless random walks tend to be less diverse. For example, in the small test scenario generated by build_graph(), this implementation now basically always uses the fallback value since it's running out of options. It should also be noted that in order not to reduce the set of candidate paths even further, I now opted to account for visited nodes on per-path basis, i.e., crossing the streams is still possible for MPPs.

[codecov-commenter]

Codecov Report

Merging #1360 (11c3120) into main (cb1d795) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1360 +/- ##
==========================================
- Coverage 90.77% 90.75% -0.02% 
==========================================
Files 73 73 Lines 41062 41069 +7 Branches 41062 41069 +7 ==========================================
+ Hits 37272 37273 +1 - Misses 3790 3796 +6 

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	92.39% <100.00%> (+0.02%)	⬆️
lightning/src/ln/functional_tests.rs	97.06% <0.00%> (-0.10%)	⬇️

---

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 cb1d795...11c3120. Read the comment docs.

[tnull]

Since the fuzzer was failing at a related point, I now rebased this off #1358.

[TheBlueMatt]

Sorry about the fuzz failure, #1358 landed now so feel free to rebase.

[tnull]

Sorry about the fuzz failure, #1358 landed now so feel free to rebase.

No apology necessary, especially in this case... Rebased on main now.

[jkczyz]

Overall looks good. Fine with squashing and merging as is. Just want to note my observation that Node::from_pubkey calls PublicKey::serialize, which allocates a Vec. So while we avoid one allocation by using an array, there is still three allocations there.

[tnull]

Rebased and squashed.

[jkczyz]

Looks like github actions are acting up. Could you run the following?

git commit --amend && git push -f

[TheBlueMatt]

Rebased and squashed.

Hmm, did you have to rebase? Please don't rebase unless there's an actual conflict as it means reviewers can't as easily compare to the previous state.

[tnull]

Looks like github actions are acting up. Could you run the following?

Done!

Hmm, did you have to rebase? Please don't rebase unless there's an actual conflict as it means reviewers can't as easily compare to the previous state.

Ah, sorry, didn't realize that. Will keep in mind.

[tnull]

This PR is a follow-up to #1286: it avoids 'shadow route' random walks that loop back on the actual payment path, since such paths are implausible from an adversary's perspective.

With this I also simplified the code a bit and opted to always generate a random shadow route of a certain length. When no suitable offset can be retrieved from the (unvisited) public network graph, the hop offset is filled with the default of MEDIAN_HOP_CLTV_EXPIRY_DELTA == 40 to simulate a partially private path.

As however already noted in #1286, such loopless random walks tend to be less diverse. For example, in the small test scenario generated by build_graph(), this implementation now basically always uses the fallback value since it's running out of options. It should also be noted that in order not to reduce the set of candidate paths even further, I now opted to account for visited nodes on per-path basis, i.e., crossing the streams is still possible for MPPs.

[codecov-commenter]

Codecov Report

Merging #1360 (11c3120) into main (cb1d795) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@ Coverage Diff @@## main #1360 +/- ##
==========================================
- Coverage 90.77% 90.75% -0.02% 
==========================================
Files 73 73 Lines 41062 41069 +7 Branches 41062 41069 +7 ==========================================
+ Hits 37272 37273 +1 - Misses 3790 3796 +6 

Impacted Files	Coverage Δ
lightning/src/routing/router.rs	92.39% <100.00%> (+0.02%)	⬆️
lightning/src/ln/functional_tests.rs	97.06% <0.00%> (-0.10%)	⬇️

---

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 cb1d795...11c3120. Read the comment docs.

[tnull]

Since the fuzzer was failing at a related point, I now rebased this off #1358.

[TheBlueMatt]

Sorry about the fuzz failure, #1358 landed now so feel free to rebase.

[tnull]

Sorry about the fuzz failure, #1358 landed now so feel free to rebase.

No apology necessary, especially in this case... Rebased on main now.

[jkczyz]

Overall looks good. Fine with squashing and merging as is. Just want to note my observation that Node::from_pubkey calls PublicKey::serialize, which allocates a Vec. So while we avoid one allocation by using an array, there is still three allocations there.

[tnull]

Rebased and squashed.

[jkczyz]

Looks like github actions are acting up. Could you run the following?

git commit --amend && git push -f

[TheBlueMatt]

Rebased and squashed.

Hmm, did you have to rebase? Please don't rebase unless there's an actual conflict as it means reviewers can't as easily compare to the previous state.

[tnull]

Looks like github actions are acting up. Could you run the following?

Done!

Hmm, did you have to rebase? Please don't rebase unless there's an actual conflict as it means reviewers can't as easily compare to the previous state.

Ah, sorry, didn't realize that. Will keep in mind.