blindedpath: Minor bug fixes and feature addition. #9026
Conversation
|
Important Review skippedAuto reviews are limited to specific labels. Labels to auto review (1)
Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
ziggie1984
force-pushed
the
bugfix-minHTLC-blindedpath
branch
from
0290e64 to
f7dc1c4
Compare
ziggie1984
changed the title
| if cfg.MinFinalCLTVExpiryDelta >= cfg.BlocksUntilExpiry { | ||
| return nil, fmt.Errorf("blinded path CLTV expiry delta (%d) "+ | ||
| "must be greater than the minimum final CLTV expiry "+ | ||
| "delta (%d)", cfg.BlocksUntilExpiry, | ||
| cfg.MinFinalCLTVExpiryDelta) | ||
| } |
There was a problem hiding this comment.
can we put this in it's own commit pls 🙏
There was a problem hiding this comment.
one thing, one commit :)
| @@ -148,7 +150,7 @@ func BuildBlindedPaymentPaths(cfg *BuildBlindedPathCfg) ( | |||
| continue | |||
| } else if err != nil { | |||
| log.Errorf("Not using route (%s) as a blinded path: %v", | |||
| err) | |||
| route, err) | |||
routing/blindedpath/blinded_path.go
Outdated
| if policy.MinHTLCMsat > cfg.ValueMsat { | ||
| return nil, 0, 0, fmt.Errorf("minHTLC of hop policy "+ | ||
| "larger than payment amt: sentAmt(%v), "+ | ||
| "minHTLC(%v)", cfg.ValueMsat, | ||
| policy.MinHTLCMsat) |
Removes a check where we would NOT allow to create a blinded invoice with an expiry (invoice expiry in seconds considered as block time) lower than the min_final_ctlv_delta.
ziggie1984
force-pushed
the
bugfix-minHTLC-blindedpath
branch
from
f7dc1c4 to
12bbf81
Compare
| // hops cannot have a higher cltv delta otherwise it will get rejected | ||
| // by the forwarding nodes or the final node. | ||
| // | ||
| // This number should at least be greater than the invoice expiry time |
There was a problem hiding this comment.
Q: can this value be equal to the invoice's final cltv delta?
There was a problem hiding this comment.
Yes this value is not related to the final_cltv value, the final_cltv_value is added on top of this expiry when calculating the payment constraint for each blinded hop.
routing/blindedpath/blinded_path.go
Outdated
| @@ -435,11 +437,27 @@ func collectRelayInfo(cfg *BuildBlindedPathCfg, path *candidatePath) ( | |||
| } | |||
| } | |||
|
|
|||
| policy, err = cfg.AddPolicyBuffer(policy) | |||
| if policy.MinHTLCMsat > cfg.ValueMsat { | |||
| return nil, 0, 0, fmt.Errorf("minHTLC of hop policy "+ | |||
There was a problem hiding this comment.
I think this should wrap an errInvalidBlindedPath, like how we handle if minHTLC > maxHTLC { below.
| // NOTE: We don't check this for maxHTLC, because the payment | ||
| // amount can always be splitted using MPP. | ||
| if bufferPolicy.MinHTLCMsat <= cfg.ValueMsat { | ||
| policy = bufferPolicy |
There was a problem hiding this comment.
IIUC we should not choose this hop if the buffered policy cannot meet our requirement? I think we can do a single check on this if bufferPolicy.MinHTLCMsat > cfg.ValueMsat { and return a wrapped errInvalidBlindedPath here if bufferPolicy.MinHTLCMsat is at least policy.MinHTLCMsat
There was a problem hiding this comment.
I thought about this as well, but I changed my mind because based on how we figured out this behaviour. Sometimes you just want to test quickly and select 1 sat as an amount, in this case we would fail creating this invoice so I decided for this rare case to just not use the policy wdyt, at the end I think this behaviour is very unlikely in general but more likely when testing ?
There was a problem hiding this comment.
yeah no strong opinion here (since i don't know it well😂), more like a question so feel free to ignore it.
We will not add a buffer to the chan policy for blinded paths in case the sender amount violates the minHTLC restriction in the first place. Moreover we disgard a route fast if the payment amount is smaller than the minHTLC along the route.
ziggie1984
force-pushed
the
bugfix-minHTLC-blindedpath
branch
from
12bbf81 to
816b25e
Compare
Fixes 2 bugs:
Planning to also add theSendToBlindedPathfeature when when the sending node is the introduction point.