Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Router: Ensure used liquidity is always limited by hop's htlc_max #3553

Closed
wants to merge 1 commit into from
Closed

Router: Ensure used liquidity is always limited by hop's htlc_max #3553

wants to merge 1 commit into from

Conversation

tnull
Copy link
Contributor

@tnull tnull commented Jan 21, 2025 •
edited
Loading

Previously, when recomputing fees for bottleneck hops, we might allow the tracked used liquidity values to surpass the actual hop capacity, which is bogus. Here, we assert we'd always limit the amount spent on a hop by its capacity/htlc_max.

Found by a fuzz test hitting the related debug_assert below.

@tnull tnull changed the title Router: Ensure used liquidity is always limited by hop's capacity Router: Ensure used liquidity is always limited by hop's htlc_max Jan 21, 2025
@tnull
Copy link
Contributor Author

tnull commented Jan 21, 2025 •
edited
Loading

To this reproduces the fuzz failure for TARGET="router":

export HEX="0306000000000000000000009800484800000000000000003fbd15d59ac0
e3008044fffffffffe2dd4bea42ea40100080098011d24b220ac520000db
dbd30000000000000040dbdba42edbdbdbdbdb0000010000000000380800
00000000000002bb1153d806000000000000020000000000200000000000
00000d5f00000000000200ed99000000004790006334616a00dbd3000000
0000000040dbdba42edbdbdbdbdb00000100000000003808000000000000
0002bb1153d806000000000000020000000000200000000000000000bbda
cc1b56740200000000000000ea7039e8a30bd64138550107a726192dfdc5
e5f9074c525216c7c38a69d7272727272727272700000000010000000000
4839b2be8bdd36ac3dd8342fa106b6dc950d76f1fdb2f5efbb59000e6b5f
edd523b0f8c55cc1600aa9366a0ed4b3f6f19bd7bc77250eb05e32a98440
e08c582136c35d0cd7da003eb80302df1d8dca974427e1c579bc36f112d1
0d4358c83a64ace3b95a23c5fb0248ca7b2c25c16bbd336e267291b2e242
bfe55a6ca7dbcca32b446311e6b6d3e06fc69ae0b91abbad0f9afa10cff6
2555b1bdee48f230addfc96b936c9587e7a7b485f7f43848f91b1ef26161
42e5f805dc0000000000006c3a8c26260000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000010000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000031000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
00000000008000000000000000000000000000000000ff00000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000110000000000
000000000000000000000000000000000000000000000000000000000000
000000fffffffffffffff000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000600000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000120000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000098fb99d8835ba506d58d710a166f64da8bc80b7be812
000000000000000000000009000000000000000000000000000000000000
000000000000000000020000000000000000000000110000000000000000
000000000000000000006b65000000000000000000000000000000000000
000000000000000000010400000000000000000000000000000000000000
000000000000000000000000000000000000000000000100000000000000
000000000000000000000000000000000000004800000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
00000000000000001c0000000000000000000000000000000000ff000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000ff0100000000000000000000000000cfb760c30e9f26f984fcd1dd
025cd9bc25db20c14d295c5c53e393b7431f28967591d40ccfa5d763a135
a79767aec130b2f7583046b8a852b73883ab31c28bd66f86e168d05e79f5
791f1ac32b5f725d3991547d726470aa26062cc866e8249362f6a5f7232e
8593a073e8a27498f54b768d963131961888a6a69a9562cfb0a3ce63d459
e3430c00df83a399916d35f90271eb1c87e1085a81d0a06a8d8f3d885f15
9cc6bc3133d61810b301000000727cd80076bd0b5e7a7e8390ff4015a26b
79420b25c5a80215ecd6c3180291bd7089197a7217a35d8ac1389447e0a2
eb311aba216ca826d6af0c642e554fbe75b02254155a00111db22f45245b
41dc3403a71fa77e7249656f855f5d76e8105430f35f5430f35f08cf7ad3
c77d30967e259fb1f1d9678392b5c6f863fa0f83f3893bb11a63d609e001
0000000000008029dab402c6b425c2350e6dc9d0345b93a2fd87bc04d644
33b6ed6d03f3e4c6f0d71439af7448fe9fac2f2c2c13415a9d639e776ed6
7ad4398359e39e327600000000000000000000000000000f000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
00000000000000000000f1ffffffffffff7f000000000000000000000000
000000000000000000000000000008000000000000000000000000090909
090909090900000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000012000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000400000000000ff0000
00000000000000000000000000000000000000000000ffff000000000000
0000000000000000000000000f0000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000f0ffffffffffffff00000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000100000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000cf303a09c0ddee83901cabf26141c0
5b949f6587654431bf6b6c33ea01d9581ddfb5d57b01675b2a57710f96d7
51501cf4fac68ce9d80300000000000000ada6a8b4cf8456174a08000000
000000006f08782ab600040100000000009a6bc3a08ff5311760ca9015cf
1ad2569b6df97b95a3fb5bc65668d2ea197e2a1c9c85f73119504a6f1a01
d5b0bf72a4765b66474dc30384100cbb9400572c86108cdf1248689b09bb
dacc1b567494edb63ec4e397f4ea7039e8a30bd64138550107a726192dfd
c5e5f9074c525216c7c38a69d71504abf4637e3f69349a7d6b127f586fea
174839b2be8bdd36ac3dd8342fa106b6dc950d76f1fdb2f5efbb59000e6b
5fedd523b0f8c55cc1600aa9366a0ed4b3f6f19bd7bc77250eb05e32a984
40e08c582136c35d0cd725003eb80302df1d8dca974427e1c579bc36f112
d10d4358c83a64ace3b95a23c5fb0248ca7b2c25c16bbd336e267291b2e2
42bfe55a6ca7dbcca32b446311e6b6d31f00000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000330000000000006b05
69fddb341c6a6a6a6a696a6a6a6a6a6a6a6a6a000000000000555dfd6a95
5b5d58e03e3975b57d6c48601c5230c9dbdb030000000000010000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
00000000000000000000000000000000dbdbe7dbdb070000000000000000
010400008000001b1b1b1bf9f9f9f9c001000000000000010700000000ff
fffffffffffffef9f9f9f9f9f90000000000000003767f00000000007500
00022d320000000000c0e2f94d44030000bdbdbdbdbdbdbdbd000000e2f9
000000ff000000dbdb870000000000000020202020202020200100000000
000000202020202020203179202082d0d0d0d0d0d0d0d0d0d06868686868
686800000000000000842377a40300000000000000000000000000000000
00000000000000001a000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000008000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000001000000ff0000000000000000009800
000000000000000000000000000000000000000000000000000000000000
000000000000040000000000000808000000000000000000000000003300
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000002100000000
00000000000000000000f0ffffffffffffff000000000000000000000000
000000000000000000000000000000000000000000000000000000000f00
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000048758071
58611b29476bd9a3cf26f7c12932b15394f732872ca0e13bac16c0b9fa00
2bd523bca65d95230f38c7b4f08eef00103d9a35a620272032ae40000000
00767a11d8c323f90629c19547b1249a4488bbb246e60c4140a921000000
9ff300000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000010000000000008000000000000000000000000000000000000000
00ffff000000000000000000000000000000000000000000000000000000
0052acdfb2241d0000000000000000000000000000000000000000000000
000000000000000000ffffff000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000100000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000900000000000000000000
000000000000000000000000000000000000000000000000000000800000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
00000000000000000000000000000000000000000000000127cf9627f900
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000110000000000000000
0000000000000000000000000000000000000000000000000000000001c0
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000001000000000000000000000000000000000000000000000000
000000000000000000000000000000800000fffffffffffffff000000000
010000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000000000000000000000000000000000000000000000
000000000000000000022d36200000000000c0e2a54d44030000bdbdbdbd
bdbdbdbd000000e2f9000000ff000000dbdb070000000000000020202020
202020200100000000000000202020202020203179202082d0d0d0d0d0d0
d0d0d0d06868686868686800000000000000842377a40300000000000000
0000"

@tnull tnull force-pushed the 2025-01-fix-spending-more-than-cap branch from a8aeaed to 9a8df10 Compare January 21, 2025 13:21
@codecov Codecov
Copy link

codecov bot commented Jan 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.39%. Comparing base (86308e1) to head (9a8df10).

Additional details and impacted files 
@@            Coverage Diff             @@
##             main    #3553      +/-   ##
==========================================
- Coverage   88.40%   88.39%   -0.02%     
==========================================
  Files         149      149              
  Lines      113874   113880       +6     
  Branches   113874   113880       +6     
==========================================
- Hits       100674   100660      -14     
- Misses      10690    10699       +9     
- Partials     2510     2521      +11

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@TheBlueMatt
Copy link
Collaborator

I think this is pointing to an error propagating fees when paying to blinded paths, not something to fix at the end when patching up paths.

@tnull
Copy link
Contributor Author

tnull commented Jan 21, 2025 •
edited
Loading

I think this is pointing to an error propagating fees when paying to blinded paths, not something to fix at the end when patching up paths.

Good point, I also wondered why this would only surface now after all the fuzzing the router has already seen. I'll see to dig deeper to see where we introduced the regression. That said, seems the given patch is still reasonable nonetheless to make the router logic more robust going forward?

@TheBlueMatt
Copy link
Collaborator

Yea, so the issue is that in our loop where we add blinded route hints we immediately add the first-hop targets which connect to the blinded intro points. In this test case we first add a blinded path, then the first-hop that links to it, then we replace that blinded path with a different one that has a better score, but higher fee, and as a result we end up with a path that spends more than the remaining liquidity on the path.

Not 100% clear to me what the best way to fix this is. We could either try to sort the blinded hints (the same way we do first-hops in sort_first_hop_channels) so that such a replacement is not possible or we could delay adding all the first-hop hints until after we've added all the blinded paths. Either way, we should be setting was_processed when we set the first-hops as the hop was now processed (this would have cause the router to behave correctly in production but triggered a fuzzing panic in a much more easily-understandable place).

That said, seems the given patch is still reasonable nonetheless to make the router logic more robust going forward?

I'm not sure - its masking some invalid path we selected which we shouldn't actually be taking, so its not really clear to me that its better to munge the path into something that may be valid vs letting it fail and fixing the actual bug.

@tnull
Copy link
Contributor Author

tnull commented Jan 22, 2025

Yea, so the issue is that in our loop where we add blinded route hints we immediately add the first-hop targets which connect to the blinded intro points. In this test case we first add a blinded path, then the first-hop that links to it, then we replace that blinded path with a different one that has a better score, but higher fee, and as a result we end up with a path that spends more than the remaining liquidity on the path.

Hmm, after having another look this seems right. However, in this case the issue might be really that we don't reset/reduce used_liquidity_msat for any reconsidered paths? This likely leads to us to discarding perfectly fine candidates, even outside of blinded paths, no?

or we could delay adding all the first-hop hints until after we've added all the blinded paths.

I'm experimenting with this approach. It seems to make things 'better' but still hits the same assertion, even when adding the first hops 'in bulk'.

Either way, we should be setting was_processed when we set the first-hops as the hop was now processed (this would have cause the router to behave correctly in production but triggered a fuzzing panic in a much more easily-understandable place).

Not sure I'm following this - which was_processed are you referring to exactly? The one when adding the first_hops_target in the beginning of paths_collection_loop would have us never build any simple 1-hop private paths or similar, as we would skip the first hop, IIUC.

I'm not sure - its masking some invalid path we selected which we shouldn't actually be taking, so its not really clear to me that its better to munge the path into something that may be valid vs letting it fail and fixing the actual bug.

I see your point and agree that we should fix the root cause, but having the liquidity saturate rather than overflow is probably still 'safer' (ie would result in less bogus paths?) in production for any upcoming issues? IMO, maybe more debug_asserts and saturating arithmetics + min/max/clamp everywhere might be a good idea? Possibly the latter could be enforced by a ChannelAmount or similar type rather than requiring it to write it out in the code everywhere?

@TheBlueMatt
Copy link
Collaborator

Hmm, after having another look this seems right. However, in this case the issue might be really that we don't reset/reduce used_liquidity_msat for any reconsidered paths? This likely leads to us to discarding perfectly fine candidates, even outside of blinded paths, no?

This would be before we update used_liquidity_msat - we have the issue when we're doing add_entry (in step 2) before we hit the node_id == our_node_id case in the 'path_construction loop in step 3.

I'm experimenting with this approach. It seems to make things 'better' but still hits the same assertion, even when adding the first hops 'in bulk'.

Hmm, mind sharing? It may be that there's multiple issues here.

Not sure I'm following this - which was_processed are you referring to exactly? The one when adding the first_hops_target in the beginning of paths_collection_loop would have us never build any simple 1-hop private paths or similar, as we would skip the first hop, IIUC.

was_processed should be set on a node after we add paths from that node towards us (the sender). ie. we'll add blinded paths towards their intro nodes, but once we add a path from the intro node towards us (in this case directly to us) we should be setting was_processed on the intro node. This does mean that we won't calculate paths that go through that intro node to a different blinded path, but I think that's fine?

IMO, maybe more debug_asserts and saturating arithmetics + min/max/clamp everywhere might be a good idea?

If we're confident the resulting paths are actually valid, yea, if it may be that we end up calculating paths that just way overshoot the amount we wanted, not so much (IIRC we have a check at the very end before we return to prevent this tho).

@tnull tnull force-pushed the 2025-01-fix-spending-more-than-cap branch from 9a8df10 to 85a0e9c Compare January 28, 2025 16:17
@tnull
Copy link
Contributor Author

tnull commented Jan 28, 2025

Hmm, mind sharing? It may be that there's multiple issues here.

Yes, pushed a WIP commit that defers adding the first hops after all blinded hints have been added.

While the values are different, it still hits the same debug_assert:

---- run_test_cases stdout ----
SPENT: 2800 = VALUE 2800 + NEXT_FEE 0
CAP: 2800, USED: 2800
USED: 2800, hop_max_msat: 18000
SPENT: 2800 = VALUE 2800 + NEXT_FEE 0
CAP: 2800, USED: 2800
USED: 2800, hop_max_msat: 2600
thread '<unnamed>' panicked at /Users/erohrer/workspace/rust-lightning/lightning/src/routing/router.rs:3142:6:
assertion failed: *used_liquidity_msat <= hop_max_msat
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

@TheBlueMatt TheBlueMatt mentioned this pull request Feb 3, 2025
Open
@tnull
Copy link
Contributor Author

tnull commented Feb 4, 2025

Closing as superseded by #3586.

@tnull tnull closed this Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
backport 0.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tnull @TheBlueMatt