Make max_total_cltv_expiry_delta include the final CLTV#1358
Make max_total_cltv_expiry_delta include the final CLTV#1358TheBlueMatt merged 2 commits intolightningdevkit:mainfrom
max_total_cltv_expiry_delta include the final CLTV#1358Conversation
TheBlueMatt
force-pushed
the
2022-03-max-cltv
branch
from
71c163b to
f3a7720
Compare
Codecov Report
@@ Coverage Diff @@
## main #1358 +/- ##
==========================================
+ Coverage 90.60% 91.86% +1.25%
==========================================
Files 72 73 +1
Lines 40324 47508 +7184
==========================================
+ Hits 36536 43643 +7107
- Misses 3788 3865 +77
Continue to review full report at Codecov.
|
|
Yikes, of course the final CLTV delta needs to be accounted for at that point! My bad, glad the fuzzer caught this.. |
TheBlueMatt
force-pushed
the
2022-03-max-cltv
branch
from
f3a7720 to
17cfc1a
Compare
|
Added a new commit which fixes the benchmark. |
lightning/src/routing/router.rs
Outdated
| for (first_hop, params, amt) in route_endpoints.iter() { | ||
| assert!(get_route(&payer, params, &graph.read_only(), Some(&[first_hop]), *amt, 42, &DummyLogger{}, &scorer, &random_seed_bytes).is_ok()); | ||
| } |
There was a problem hiding this comment.
I believe this will significantly increase the benchmark runtime. I tried doing something similar when refactoring the benchmarks without much luck.
I have the benchmarks running on my machine now for over an hour and have only seen one result so far:
running 6 tests
test routing::router::benches::generate_mpp_routes_with_default_scorer ... bench: 10,138,522,397 ns/iter (+/- 541,770,074)
There was a problem hiding this comment.
Grrr, I'd somehow thought the bencher was smarter than that :(.
| let route = get_route(&our_id, &feasible_payment_params, &network_graph, None, 100, 0, Arc::clone(&logger), &scorer, &random_seed_bytes).unwrap(); | ||
| let path = route.paths[0].iter().map(|hop| hop.short_channel_id).collect::<Vec<_>>(); | ||
| assert_ne!(path.len(), 0); | ||
|
|
||
| // But not if we exclude all paths on the basis of their accumulated CLTV delta | ||
| let fail_max_total_cltv_delta = 23; | ||
| let fail_payment_params = PaymentParameters::from_node_id(nodes[6]).with_route_hints(last_hops(&nodes)) | ||
| .with_max_total_cltv_expiry_delta(fail_max_total_cltv_delta); | ||
| match get_route(&our_id, &fail_payment_params, &network_graph, None, 100, 42, Arc::clone(&logger), &scorer, &random_seed_bytes) | ||
| match get_route(&our_id, &fail_payment_params, &network_graph, None, 100, 0, Arc::clone(&logger), &scorer, &random_seed_bytes) |
There was a problem hiding this comment.
Why make final_cltv_expiry_delta zero here?
There was a problem hiding this comment.
Cause the test was failing, and setting it to 0 is the most obvious change that doesn't change the test semantics at all by emulating the previous behavior.
| let max_total_cltv_expiry_delta = payment_params.max_total_cltv_expiry_delta | ||
| let max_total_cltv_expiry_delta = (payment_params.max_total_cltv_expiry_delta - final_cltv_expiry_delta) | ||
| .checked_sub(2*MEDIAN_HOP_CLTV_EXPIRY_DELTA) | ||
| .unwrap_or(payment_params.max_total_cltv_expiry_delta); | ||
| .unwrap_or(payment_params.max_total_cltv_expiry_delta - final_cltv_expiry_delta); |
There was a problem hiding this comment.
To help clarify my understanding, does $next_hops_cltv_delta include final_cltv_expiry_delta? Or is this needed because $candidate.cltv_expiry_delta() is actually for the previous hop as adjusted later on line 1534 and thus final_cltv_expiry_delta was not accounted for?
There was a problem hiding this comment.
If I understand correctly, the latter is the case: at this point the final_cltv_expiry_delta has not been added and therefore needs to be accounted for. This is in contrast to later usages of payment_params.max_total_cltv_expiry_delta, as for example in add_random_cltv_offset(), where final_cltv_expiry_delta is already part of the paths' CTLV deltas.
TheBlueMatt
force-pushed
the
2022-03-max-cltv
branch
from
17cfc1a to
1ab3454
Compare
If the scoring in the routing benchmark causes us to take a different path from the original scan, we may end up deciding that the only path to a node has a too-high total CLTV delta, causing us to panic in the benchmarking phase. Here we simply check for that possibility and remove paths that fail post-scoring.
This fixes an integer underflow found by the `router` fuzz target in CI.
TheBlueMatt
dismissed stale reviews from valentinewallace and jkczyz
via
bb4413c
TheBlueMatt
force-pushed
the
2022-03-max-cltv
branch
from
b8c9029 to
bb4413c
Compare
|
Squashed without change. |
5 participants
This fixes an integer underflow found by the
routerfuzz targetin CI.