multi: route blinding for hodl invoices #9034
Conversation
WalkthroughThe changes introduced in this pull request enhance the handling of invoices with blinded paths in the Lightning Network. New command-line flags related to blinded paths have been added to the Changes
Assessment against linked issues
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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
ellemouton
added
the
size/micro
|
Labeling as Micro review load since this does not require any route blinding knowledge/context. All this does is a bit of refactor of the AddInvoice flow to make the same stuff re-usable for AddHoldInvoice |
ellemouton
force-pushed
the
routeblindHoldInvoice
branch
2 times, most recently
from
0b16e81 to
d2d9f3a
Compare
We will use it again once we add blinded path support to hold invoices.
To make it reusable when we add support to hold invoices.
So that they are re-usable for other commands.
ellemouton
force-pushed
the
routeblindHoldInvoice
branch
from
d2d9f3a to
6dcfab8
Compare
|
cc @saubyk |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (9)
lnrpc/invoicesrpc/invoices.proto (3)
131-143: LGTM! Consider a minor comment improvement.The addition of
is_blindedandblinded_path_configfields to theAddHoldInvoiceRequestmessage aligns well with the PR objectives to support route blinding for hodl invoices. The implementation looks correct and well-documented.Consider updating the comment for
blinded_path_configto clarify the relationship withis_blinded:/* Config values to use when creating blinded paths for this invoice. These can be used to override the defaults config values provided in by the - global config. This field is only used if is_blinded is true. + global config. This field is only applicable when is_blinded is set to true. */ lnrpc.BlindedPathConfig blinded_path_config = 12;This minor change emphasizes the dependency between the two fields more clearly.
Line range hint
1-143: Changes are consistent and maintain backward compatibility.The additions to the
AddHoldInvoiceRequestmessage are well-integrated into the existing protocol definition:
- Backward compatibility is maintained as the new fields are optional.
- The
AddHoldInvoiceRPC method can now support blinded paths without modifying its signature.- The changes are consistent with the existing structure and naming conventions in the file.
- Clients using the old version of the protocol will not be affected.
To ensure smooth integration, consider updating any client-side code that generates
AddHoldInvoiceRequestmessages to include the new fields when appropriate. Also, update the server-side implementation ofAddHoldInvoiceto handle these new fields correctly.
Line range hint
1-143: Changes align well with PR objectives.The additions to
AddHoldInvoiceRequestdirectly address the PR objectives of makingAddHoldInvoicecompatible with route blinding. The implementation approach is consistent with the suggestion in the linked issue (#9024).Consider updating the following documentation to reflect these changes:
- The comment for the
AddHoldInvoiceRPC method (around line 40) to mention support for blinded paths.- Any relevant API documentation or developer guides that describe the usage of
AddHoldInvoice.This will ensure that developers are aware of the new functionality and how to use it.
cmd/commands/cmd_invoice.go (1)
Line range hint
206-246: LGTM: Well-implemented parseBlindedPathCfg function with a minor suggestionThe
parseBlindedPathCfgfunction is well-implemented, correctly handling the blinded path configuration options. The initial check for theblindflag and the error handling for misconfigurations are good additions.One minor suggestion for improvement:
Consider enhancing the error handling when decoding the node public keys in the
NodeOmissionList. Currently, if the hex decoding fails, the error doesn't provide context about which public key caused the issue. You could improve this by including the problematic public key in the error message:for _, pubKey := range ctx.StringSlice(blindedPathNodeOmissions.Name) { pubKeyBytes, err := hex.DecodeString(pubKey) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to decode node public key %s: %w", pubKey, err) } blindCfg.NodeOmissionList = append( blindCfg.NodeOmissionList, pubKeyBytes, ) }This change would make it easier for users to identify and correct any invalid public keys in their command.
lnrpc/invoicesrpc/config_active.go (3)
67-69: Update function comment to reflect new parameterThe comment for
GenInvoiceFeaturesdoes not mention the newblindedparameter. Please update the documentation to explain the purpose and effect of this parameter.
84-85: Add parameter names for clarity inBlindedPathCfgIn the
BlindedPathCfgfunction signature, consider naming the parameters to improve readability. For example:BlindedPathCfg func(useGlobalPolicy bool, config *lnrpc.BlindedPathConfig) (*BlindedPathConfig, error)
93-94: Include parameter names inQueryBlindedRoutesfor better readabilityFor the
QueryBlindedRoutesfunction, adding parameter names enhances code clarity. For example:QueryBlindedRoutes func(restrictions *routing.BlindedPathRestrictions, amt lnwire.MilliSatoshi) ([]*route.Route, error)itest/lnd_route_blinding_test.go (2)
611-611: Clarify the added comment for test documentationThe comment on line 611 could be clearer. Consider rephrasing for better readability.
Apply this change:
-// payment. It also tests that blinded paths work for hodl invoices. +// This test verifies lnd's ability to handle blinded payment paths in invoices, +// including the creation and settlement of HODL invoices with blinded paths.
695-699: Use dynamic fee limit instead of a hardcoded valueOn line 698, the fee limit is hardcoded as
FeeLimitSat: 1000000. This may not be suitable for all payment amounts and could cause issues if the payment amount changes.Consider calculating the fee limit based on the payment amount:
FeeLimitSat: uint64(paymentAmt / 10), // e.g., 10% of the payment amountOr set it to
noFeeLimitif appropriate for the test context.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
lnrpc/invoicesrpc/invoices.pb.gois excluded by!**/*.pb.go,!**/*.pb.golnrpc/invoicesrpc/invoices.swagger.jsonis excluded by!**/*.json
📒 Files selected for processing (10)
- cmd/commands/cmd_invoice.go (5 hunks)
- cmd/commands/invoicesrpc_active.go (2 hunks)
- docs/release-notes/release-notes-0.19.0.md (1 hunks)
- itest/lnd_route_blinding_test.go (3 hunks)
- lnrpc/invoicesrpc/addinvoice.go (6 hunks)
- lnrpc/invoicesrpc/config_active.go (3 hunks)
- lnrpc/invoicesrpc/invoices.proto (1 hunks)
- lnrpc/invoicesrpc/invoices_server.go (3 hunks)
- rpcserver.go (5 hunks)
- subrpcserver_config.go (4 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/release-notes/release-notes-0.19.0.md
[grammar] ~29-~29: There seems to be a noun/verb agreement error. Did you mean “receives” or “received”?
Context: ...onal Enhancements * Add route blinding receive functionality to for [hodl invoices]...(SINGULAR_NOUN_VERB_AGREEMENT)
🪛 Markdownlint
docs/release-notes/release-notes-0.19.0.md
29-29: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
🔇 Additional comments (29)
docs/release-notes/release-notes-0.19.0.md (2)
29-29: Maintain consistent list style.The static analysis tool suggests using a dash instead of an asterisk for the unordered list item. However, the current usage of asterisks is consistent with other list items in the document.
Ignore the static analysis hint in this case to maintain consistency throughout the release notes.
🧰 Tools
🪛 LanguageTool
[grammar] ~29-~29: There seems to be a noun/verb agreement error. Did you mean “receives” or “received”?
Context: ...onal Enhancements * Add route blinding receive functionality to for [hodl invoices]...(SINGULAR_NOUN_VERB_AGREEMENT)
🪛 Markdownlint
29-29: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
Line range hint
1-31: Overall, the release notes accurately reflect the PR objectives.The addition of route blinding receive functionality for hodl invoices is well-documented in the release notes. This aligns perfectly with the PR objectives and linked issue #9024. The changes are concise and informative, providing users with a clear understanding of the new feature.
Great job on keeping the release notes up-to-date with the latest changes!
🧰 Tools
🪛 LanguageTool
[uncategorized] ~27-~27: Possible missing preposition found.
Context: ...esponse is accurate. # New Features ## Functional Enhancements * Add route blinding rece...(AI_HYDRA_LEO_MISSING_OF)
[grammar] ~29-~29: There seems to be a noun/verb agreement error. Did you mean “receives” or “received”?
Context: ...onal Enhancements * Add route blinding receive functionality to for [hodl invoices]...(SINGULAR_NOUN_VERB_AGREEMENT)
🪛 Markdownlint
29-29: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
34-34: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
cmd/commands/cmd_invoice.go (4)
13-44: LGTM: Well-defined flags for blinded path configurationThe new flags for blinded path configuration are well-defined and clearly explained. The types and naming conventions are appropriate and consistent with the existing codebase.
120-124: LGTM: New flags correctly added to AddInvoiceCommandThe new blinded path flags have been correctly added to the
Flagsslice in theAddInvoiceCommandstructure. The order is logical, with the new flags appended to the existing ones.
192-193: LGTM: Proper handling of blinded path options in addInvoiceThe
addInvoicefunction has been correctly updated to handle the new blinded path options. The mutually exclusive check forprivateandblindflags is a good addition to prevent conflicting configurations.
Line range hint
1-246: Summary: Excellent implementation of blinded path support for invoicesThe changes introduced in this file successfully add support for blinded paths in invoice creation. The new flags and their handling are well-implemented and consistent with the existing codebase style. These changes provide users with clear options to configure blinded paths, enhancing the privacy features of the invoicing system.
Key improvements:
- Well-defined flags for blinded path configuration
- Proper integration of new flags into the
AddInvoiceCommandstructure- Correct handling of blinded path options in the
addInvoicefunction- A comprehensive
parseBlindedPathCfgfunction for processing blinded path configurationsThese changes align well with the PR objectives of introducing route blinding functionality for receiving hodl invoices and making the
AddHoldInvoicefunction compatible with route blinding.lnrpc/invoicesrpc/config_active.go (1)
67-67: Ensure all usages ofGenInvoiceFeaturesare updatedThe function signature of
GenInvoiceFeatureshas changed to accept ablindedboolean parameter. Please verify that all implementations and calls to this function are updated to include the new parameter.Run the following script to identify all usages of
GenInvoiceFeatures:✅ Verification successful
All usages of
GenInvoiceFeaturesare updated to include the newblindedparameter.
lnrpc/invoicesrpc/addinvoice.go: invoiceFeatures = cfg.GenInvoiceFeatures(blind)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of GenInvoiceFeatures in the codebase. rg 'GenInvoiceFeatures\('Length of output: 109
cmd/commands/invoicesrpc_active.go (3)
265-266: Confirm thatIsBlindedandBlindedPathConfigare correctly added toAddHoldInvoiceRequestThe fields
IsBlindedandBlindedPathConfigare added to theAddHoldInvoiceRequeststruct. Ensure that these fields are defined in theinvoicesrpc.AddHoldInvoiceRequeststruct and are compatible with the server-side implementation.To verify that these fields are defined in the struct, you can run the following script:
#!/bin/bash # Description: Check that `IsBlinded` and `BlindedPathConfig` are fields in `AddHoldInvoiceRequest`. # Test: Search for the struct definition and look for the new fields. Expect: Fields are present. rg --type go --no-heading --line-number 'type AddHoldInvoiceRequest struct' -A 20 | rg -E 'IsBlinded|BlindedPathConfig'
249-253: EnsureparseBlindedPathCfgfunction is defined and implemented correctlyThe function
parseBlindedPathCfg(ctx)is called to parse the blinded path configuration from the CLI context. Verify that this function is defined and correctly processes the new flags, returning an appropriateblindedPathCfgand handling errors properly.To confirm the existence and correctness of
parseBlindedPathCfg, you can run the following script:
201-205: Verify that new flags are properly defined and initializedThe new flags
blindFlag,minRealBlindedHops,numBlindedHops,maxBlindedPaths, andblindedPathNodeOmissionsare added to theaddHoldInvoiceCommand. Ensure that these flags are properly defined with appropriate names, usage descriptions, and default values if necessary.To confirm the definitions of these flags, you can run the following script:
subrpcserver_config.go (1)
286-294: Verify that new fields exist in 'invoicesrpc.Config'Ensure that the fields
BlindedPathCfg,BestHeight, andQueryBlindedRoutesexist in theinvoicesrpc.Configstruct and are properly defined for use. This is crucial to avoid runtime errors due to missing or misspelled fields.Run the following script to check if these fields are defined in
invoicesrpc.Config:#!/bin/bash # Description: Verify that 'invoicesrpc.Config' has the new fields. # Search for the definition of invoicesrpc.Config and check for the fields. ast-grep --lang go --pattern $'type Config struct { $$$ BlindedPathCfg func($_) BestHeight func() ($_) QueryBlindedRoutes func($_) $$$ }'lnrpc/invoicesrpc/invoices_server.go (3)
342-347: Addition ofblindedPathCfgwith proper error handlingThe initialization of
blindedPathCfgusings.cfg.BlindedPathCfgand the accompanying error handling are correctly implemented. This ensures that the blinded path configuration is properly set up for the invoice.
360-361: Inclusion ofBestHeightandQueryBlindedRoutesinAddInvoiceConfigThe new fields
BestHeightandQueryBlindedRoutesare appropriately added to theAddInvoiceConfigstruct. This enhances the configuration by providing necessary context for invoice creation with respect to the blockchain height and routing queries.
391-391: Assignment ofblindedPathCfgtoAddInvoiceDataThe assignment of
blindedPathCfgto theBlindedPathCfgfield inAddInvoiceDatais correctly done. This ensures that the blinded path configuration is included when adding the invoice data.lnrpc/invoicesrpc/addinvoice.go (6)
81-84: Change toGenInvoiceFeaturesfunction signature is acceptableThe function
GenInvoiceFeatureshas been updated to accept ablindedboolean parameter, enabling feature generation based on whether the invoice includes a blinded path.
100-101: Update toQueryBlindedRoutesfunction signature is appropriateThe function
QueryBlindedRoutesnow accepts a*routing.BlindedPathRestrictionsparameter, allowing for more precise control over route queries for blinded paths.
179-181: Addition ofRestrictionsfield toBlindedPathConfigThe
Restrictionsfield has been added toBlindedPathConfig, enabling more flexible and precise rules for constructing blinded paths.
489-489: Update toGenInvoiceFeaturescall withblindargumentIn the
AddInvoicefunction, the call tocfg.GenInvoiceFeaturesnow includes theblindparameter, ensuring that the correct invoice features are generated based on whether the invoice contains a blinded path.
526-532: Proper handling ofblindCfg.RestrictionsinFindRoutesThe
FindRoutesfunction now correctly passesblindCfg.Restrictionstocfg.QueryBlindedRoutes, ensuring that route queries adhere to the specified restrictions for blinded paths.
550-550: SetMinNumHopsfromblindCfg.Restrictions.NumHopsIn the call to
BuildBlindedPaymentPaths,MinNumHopsis now set toblindCfg.Restrictions.NumHops, ensuring that the blinded path construction adheres to the minimum number of hops specified in the restrictions.itest/lnd_route_blinding_test.go (2)
14-14: Importinvoicesrpcpackage is necessary forAddHoldInvoiceThe import of
github.com/lightningnetwork/lnd/lnrpc/invoicesrpcon line 14 is necessary as theAddHoldInvoicefunction from this package is used later in the code (e.g., line 676).
714-715: Ensure payment status assertion is reliableThe assertion on line 715 checks that Alice's payment status is
SUCCEEDED. Verify that there are no race conditions that might cause intermittent failures in test environments.To confirm, you might inspect the payment status before asserting:
status := ht.AssertPaymentStatus(ht.Alice, preimage, lnrpc.Payment_SUCCEEDED) require.Equal(ht, lnrpc.Payment_SUCCEEDED, status)rpcserver.go (7)
5988-6001: Ensure proper initialization ofAddInvoiceConfigThe
AddInvoiceConfigstruct is being initialized with new fieldsGenInvoiceFeatures,GenAmpInvoiceFeatures,BestHeight, andQueryBlindedRoutes. Verify that these fields are correctly handled downstream and that they integrate seamlessly with existing logic.Ensure that the functions
r.genInvoiceFeaturesandr.queryBlindedRoutesare properly implemented and that they align with the expected interfaces.
6015-6019: Handle possible errors fromblindedPathCfgWhen calling
r.blindedPathCfg, the error is checked immediately after the call. This is good practice to ensure any issues are caught promptly.
8727-8736: Verify feature bits adjustments for blinded pathsWhen
blindistrue, the code unsetsPaymentAddrRequiredand setsPaymentAddrOptional. Ensure that this adjustment complies with the protocol and that all clients interacting with these invoices can handle the absence of a payment address.Confirm that unsetting
PaymentAddrRequiredand settingBolt11BlindedPathsRequiredalign with the BOLT specifications for blinded payments.
8798-8801: Ensure proper type conversion for fee parametersWhen assigning values to
FeeRateandBaseFee, ensure that the data types match and that value ranges are appropriate.Check that
r.server.cfg.Bitcoin.FeeRateandr.server.cfg.Bitcoin.BaseFeefit withinuint32without loss of precision or overflow.
774-775:⚠️ Potential issueConsider handling possible negative
bestHeightThe code converts
bestHeighttouint32without checking for potential negative values. Although block heights are typically non-negative, adding a check can prevent unexpected behavior if an error occurs.Consider adding a check for negative
bestHeightbefore casting:func() (uint32, error) { _, bestHeight, err := r.server.cc.ChainIO.GetBestBlock() + if bestHeight < 0 { + return 0, fmt.Errorf("bestHeight cannot be negative") + } return uint32(bestHeight), err }
795-795: 🛠️ Refactor suggestion
⚠️ Potential issueTypo in function name
updateAndBrodcastSelfNodeThere is a typo in the function name
updateAndBrodcastSelfNode. It should beupdateAndBroadcastSelfNode.Apply this diff to correct the typo:
- s.getNodeAnnouncement, s.updateAndBrodcastSelfNode, parseAddr, + s.getNodeAnnouncement, s.updateAndBroadcastSelfNode, parseAddr,Ensure that all references to this function are updated accordingly.
Likely invalid or redundant comment.
8751-8754: 🛠️ Refactor suggestion
⚠️ Potential issueUnreachable condition due to prior return
The condition
if cfg != nil && !blind(line 8751) is unreachable because the function returns early if!blindistrue(line 8747). This makes the condition redundant.Remove the unreachable condition to clean up the code:
- if cfg != nil && !blind { - return nil, fmt.Errorf("blinded path config provided but IsBlinded not set") - }Likely invalid or redundant comment.
|
|
||
| * Add route blinding receive functionality to for [hodl | ||
| invoices](https://github.com/lightningnetwork/lnd/pull/9034). | ||
|
|
There was a problem hiding this comment.
Improve grammar and clarity in the new feature description.
The sentence structure in this new feature description can be improved for better clarity and grammatical correctness.
Consider applying the following changes:
-* Add route blinding receive functionality to for [hodl
- invoices](https://github.com/lightningnetwork/lnd/pull/9034).
+* Add route blinding receive functionality for [hodl
+ invoices](https://github.com/lightningnetwork/lnd/pull/9034).This change removes the redundant "to" and improves the overall readability of the sentence.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| * Add route blinding receive functionality to for [hodl | |
| invoices](https://github.com/lightningnetwork/lnd/pull/9034). | |
| * Add route blinding receive functionality for [hodl | |
| invoices](https://github.com/lightningnetwork/lnd/pull/9034). | |
🧰 Tools
🪛 LanguageTool
[grammar] ~29-~29: There seems to be a noun/verb agreement error. Did you mean “receives” or “received”?
Context: ...onal Enhancements * Add route blinding receive functionality to for [hodl invoices]...(SINGULAR_NOUN_VERB_AGREEMENT)
🪛 Markdownlint
29-29: Expected: dash; Actual: asterisk
Unordered list style(MD004, ul-style)
| genInvoiceFeatures func(bool) *lnwire.FeatureVector, | ||
| genAmpInvoiceFeatures func() *lnwire.FeatureVector, | ||
| getNodeAnnouncement func() lnwire.NodeAnnouncement, | ||
| updateNodeAnnouncement func(features *lnwire.RawFeatureVector, | ||
| modifiers ...netann.NodeAnnModifier) error, | ||
| parseAddr func(addr string) (net.Addr, error), | ||
| rpcLogger btclog.Logger, aliasMgr *aliasmgr.Manager, | ||
| auxDataParser fn.Option[AuxDataParser], | ||
| invoiceHtlcModifier *invoices.HtlcModificationInterceptor) error { | ||
| invoiceHtlcModifier *invoices.HtlcModificationInterceptor, | ||
| bestHeight func() (uint32, error), | ||
| blindedPathCfg func(blind bool, cfg *lnrpc.BlindedPathConfig) ( | ||
| *invoicesrpc.BlindedPathConfig, error), | ||
| queryBlindedRoutes func(*routing.BlindedPathRestrictions, | ||
| lnwire.MilliSatoshi) ([]*route.Route, error)) error { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring 'PopulateDependencies' to reduce parameter list
The PopulateDependencies function now has over 30 parameters. This long parameter list can affect readability and maintainability. Consider refactoring by grouping related parameters into structs or passing a configuration object to simplify the function signature.
| invoiceHtlcModifier *invoices.HtlcModificationInterceptor, | ||
| bestHeight func() (uint32, error), | ||
| blindedPathCfg func(blind bool, cfg *lnrpc.BlindedPathConfig) ( | ||
| *invoicesrpc.BlindedPathConfig, error), | ||
| queryBlindedRoutes func(*routing.BlindedPathRestrictions, | ||
| lnwire.MilliSatoshi) ([]*route.Route, error)) error { |
There was a problem hiding this comment.
💡 Codebase verification
PopulateDependencies Calls Not Updated
The following calls to PopulateDependencies do not match the updated function signature:
subrpcserver_config.go:107- Arguments passed: 2rpcserver.go:787- Arguments passed: 1
Please update these function calls to include all required parameters.
🔗 Analysis chain
Ensure all callers of 'PopulateDependencies' are updated
With the addition of new parameters to the PopulateDependencies function, ensure that all function calls are updated accordingly to match the new signature. This will prevent build errors and potential runtime issues.
Run the following script to search for all calls to PopulateDependencies and verify they have the correct number of arguments:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all usages of PopulateDependencies and display their function signatures.
# Search for the function definition to get the parameter count.
rg --type go --max-depth 1 -A 5 'func \(s \*subRPCServerConfigs\) PopulateDependencies'
# Search for all calls to PopulateDependencies in the codebase.
rg --type go 'PopulateDependencies\('
Length of output: 430
Script:
#!/bin/bash
# Description: Find all calls to PopulateDependencies and count the number of arguments passed in each call.
# Search for all calls to PopulateDependencies and extract the lines containing the calls
rg --type go 'PopulateDependencies\(' -n | while IFS= read -r line; do
filepath=$(echo "$line" | cut -d: -f1)
lineno=$(echo "$line" | cut -d: -f2)
call=$(echo "$line" | cut -d: -f3-)
# Count the number of commas to estimate the number of arguments
arg_count=$(echo "$call" | grep -o "," | wc -l)
# Add one to represent the total number of arguments
total_args=$((arg_count + 1))
echo "$filepath:$lineno - Arguments passed: $total_args"
done
Length of output: 1340
| minNumRealHops = 2 | ||
| numHops = 2 | ||
| hodlResp := testCase.dave.RPC.AddHoldInvoice( | ||
| &invoicesrpc.AddHoldInvoiceRequest{ | ||
| Memo: "test", | ||
| Hash: hash[:], | ||
| ValueMsat: 10_000_000, | ||
| IsBlinded: true, | ||
| BlindedPathConfig: &lnrpc.BlindedPathConfig{ | ||
| MinNumRealHops: &minNumRealHops, | ||
| NumHops: &numHops, | ||
| }, | ||
| }, | ||
| ) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract repeated code into a helper function to avoid duplication
The code block from lines 673-687 duplicates the logic for creating a HODL invoice with blinded path configuration. Extracting this into a helper function enhances maintainability.
Consider adding a helper function:
func addBlindedHoldInvoice(dave *node.HarnessNode, preimage lntypes.Preimage) *invoicesrpc.AddHoldInvoiceResp {
var (
minNumRealHops uint32 = 2
numHops uint32 = 2
)
hash := preimage.Hash()
return dave.RPC.AddHoldInvoice(
&invoicesrpc.AddHoldInvoiceRequest{
Memo: "test",
Hash: hash[:],
ValueMsat: 10_000_000,
IsBlinded: true,
BlindedPathConfig: &lnrpc.BlindedPathConfig{
MinNumRealHops: &minNumRealHops,
NumHops: &numHops,
},
},
)
}Then replace lines 673-687 with:
hodlResp := addBlindedHoldInvoice(testCase.dave, preimage)| restrictions *routing.BlindedPathRestrictions, | ||
| amt lnwire.MilliSatoshi) ([]*route.Route, error) { | ||
|
|
||
| return r.server.chanRouter.FindBlindedPaths( | ||
| r.selfNode, amt, r.server.missionControl.GetProbability, | ||
| restrictions, | ||
| ) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error context in queryBlindedRoutes
The function queryBlindedRoutes returns the result of FindBlindedPaths without adding context to potential errors. Enhancing error messages can aid in debugging.
Modify the function to wrap the error for more clarity:
func (r *rpcServer) queryBlindedRoutes(
restrictions *routing.BlindedPathRestrictions,
amt lnwire.MilliSatoshi) ([]*route.Route, error) {
- return r.server.chanRouter.FindBlindedPaths(
+ routes, err := r.server.chanRouter.FindBlindedPaths(
r.selfNode, amt, r.server.missionControl.GetProbability,
restrictions,
)
+ if err != nil {
+ return nil, fmt.Errorf("failed to find blinded paths: %w", err)
+ }
+ return routes, nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| restrictions *routing.BlindedPathRestrictions, | |
| amt lnwire.MilliSatoshi) ([]*route.Route, error) { | |
| return r.server.chanRouter.FindBlindedPaths( | |
| r.selfNode, amt, r.server.missionControl.GetProbability, | |
| restrictions, | |
| ) | |
| } | |
| restrictions *routing.BlindedPathRestrictions, | |
| amt lnwire.MilliSatoshi) ([]*route.Route, error) { | |
| routes, err := r.server.chanRouter.FindBlindedPaths( | |
| r.selfNode, amt, r.server.missionControl.GetProbability, | |
| restrictions, | |
| ) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to find blinded paths: %w", err) | |
| } | |
| return routes, nil | |
| } |
There was a problem hiding this comment.
I think this log makes sense wdyt @ellemouton
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| if cfg.MinNumRealHops != nil { | ||
| restrictions.MinDistanceFromIntroNode = | ||
| uint8(*cfg.MinNumRealHops) | ||
| } | ||
| if cfg.NumHops != nil { | ||
| restrictions.NumHops = uint8(*cfg.NumHops) | ||
| } | ||
| if cfg.MaxNumPaths != nil { | ||
| restrictions.MaxNumPaths = uint8(*cfg.MaxNumPaths) | ||
| } |
There was a problem hiding this comment.
Check for value overflows when casting to uint8
Casting configuration values to uint8 without validation may result in overflow if the values exceed 255.
Add validation to ensure values are within uint8 range:
if cfg.MinNumRealHops != nil {
+ if *cfg.MinNumRealHops > 255 {
+ return nil, fmt.Errorf("MinNumRealHops exceeds uint8 max value")
+ }
restrictions.MinDistanceFromIntroNode = uint8(*cfg.MinNumRealHops)
}
if cfg.NumHops != nil {
+ if *cfg.NumHops > 255 {
+ return nil, fmt.Errorf("NumHops exceeds uint8 max value")
+ }
restrictions.NumHops = uint8(*cfg.NumHops)
}
if cfg.MaxNumPaths != nil {
+ if *cfg.MaxNumPaths > 255 {
+ return nil, fmt.Errorf("MaxNumPaths exceeds uint8 max value")
+ }
restrictions.MaxNumPaths = uint8(*cfg.MaxNumPaths)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if cfg.MinNumRealHops != nil { | |
| restrictions.MinDistanceFromIntroNode = | |
| uint8(*cfg.MinNumRealHops) | |
| } | |
| if cfg.NumHops != nil { | |
| restrictions.NumHops = uint8(*cfg.NumHops) | |
| } | |
| if cfg.MaxNumPaths != nil { | |
| restrictions.MaxNumPaths = uint8(*cfg.MaxNumPaths) | |
| } | |
| if cfg.MinNumRealHops != nil { | |
| if *cfg.MinNumRealHops > 255 { | |
| return nil, fmt.Errorf("MinNumRealHops exceeds uint8 max value") | |
| } | |
| restrictions.MinDistanceFromIntroNode = | |
| uint8(*cfg.MinNumRealHops) | |
| } | |
| if cfg.NumHops != nil { | |
| if *cfg.NumHops > 255 { | |
| return nil, fmt.Errorf("NumHops exceeds uint8 max value") | |
| } | |
| restrictions.NumHops = uint8(*cfg.NumHops) | |
| } | |
| if cfg.MaxNumPaths != nil { | |
| if *cfg.MaxNumPaths > 255 { | |
| return nil, fmt.Errorf("MaxNumPaths exceeds uint8 max value") | |
| } | |
| restrictions.MaxNumPaths = uint8(*cfg.MaxNumPaths) | |
| } |
There was a problem hiding this comment.
I think overflow checks are reasonable.
There was a problem hiding this comment.
@ziggie1984, I'm glad you agree. Adding overflow checks will help prevent potential value overflows.
(_/)
( •ᴗ•)
(ᵔᵕᵔ)
There was a problem hiding this comment.
Can we add itests for the blinded hodlinvoice payment ?
Found buggy behaviour when testing with LND instances:
Created hodlinvoice on Node A
Paid it from Node C (NODE B is the intronode)
Payment reaches Node A - so far so good.
Now I cancel the hodlinvoice on Node A:
Now weird stuff happens, Node C goes crazy and retries the payment all over again:
Description:
Amount (in satoshis): 1000
Fee limit (in satoshis): 1000
Destination: 02acfc299123c6f01d3a116ebbcd10460a26b56b4067c2c5652f467fa2e3d233e4
Confirm payment (yes/no): yes
+----------------------------------+--------------+--------------+--------------+-------+----------+-----------------+-----------------------+
| HTLC_STATE | ATTEMPT_TIME | RESOLVE_TIME | RECEIVER_AMT | FEE | TIMELOCK | CHAN_OUT | ROUTE |
+----------------------------------+--------------+--------------+--------------+-------+----------+-----------------+-----------------------+
| INVALID_ONION_BLINDING @ 1st hop | 0.031 | 11.271 | 1000 | 2.204 | 700 | 479387069775872 | alice->032e4b->02efe1 |
| INVALID_ONION_BLINDING @ 1st hop | 11.295 | 11.909 | 1000 | 2.204 | 700 | 479387069775872 | alice->032e4b->02efe1 |
| INVALID_ONION_BLINDING @ 1st hop | 11.938 | 12.556 | 1000 | 2.204 | 700 | 479387069775872 | alice->032e4b->02efe1 |
| INVALID_ONION_BLINDING @ 1st hop | 12.605 | 13.198 | 1000 | 2.204 | 700 | 479387069775872 | alice->032e4b->02efe1 |
| IN_FLIGHT | 13.226 | - | 1000 | 2.204 | 700 | 479387069775872 | alice->032e4b->02efe1 |
+----------------------------------+--------------+--------------+--------------+-------+----------+-----------------+-----------------------+
Amount + fee: 0 + 0 sat
Payment hash: c472a6497526c9ed8e59ba3063e20b8040580c52135d90390e533e2177081db4
Payment status: IN_FLIGHT
^C[lncli] rpc error: code = Canceled desc = context canceled
if I do not cancel this goes on for a very long time.
==
Success Case works like charm.
| // containing the blinded path, so we switch | ||
| // the bit to required. | ||
| v.Unset(lnwire.Bolt11BlindedPathsOptional) | ||
| v.Set(lnwire.Bolt11BlindedPathsRequired) |
There was a problem hiding this comment.
I think we should also check for dependencies here maybe ?
Will be handled in #9143
| // understand the new BOLT 11 tagged field | ||
| // containing the blinded path, so we switch | ||
| // the bit to required. | ||
| v.Unset(lnwire.Bolt11BlindedPathsOptional) |
There was a problem hiding this comment.
I wonder if we could remove this bit from defaultSetDesc and only set it and its defaults here ?
| restrictions *routing.BlindedPathRestrictions, | ||
| amt lnwire.MilliSatoshi) ([]*route.Route, error) { | ||
|
|
||
| return r.server.chanRouter.FindBlindedPaths( | ||
| r.selfNode, amt, r.server.missionControl.GetProbability, | ||
| restrictions, | ||
| ) | ||
| } |
There was a problem hiding this comment.
I think this log makes sense wdyt @ellemouton
| bpConfig := r.server.cfg.Routing.BlindedPaths | ||
| defaultDelta := r.cfg.Bitcoin.TimeLockDelta | ||
|
|
||
| globalBlindCfg := r.server.cfg.Routing.BlindedPaths |
There was a problem hiding this comment.
why 2 times the same variable ?
| if cfg.MinNumRealHops != nil { | ||
| restrictions.MinDistanceFromIntroNode = | ||
| uint8(*cfg.MinNumRealHops) | ||
| } | ||
| if cfg.NumHops != nil { | ||
| restrictions.NumHops = uint8(*cfg.NumHops) | ||
| } | ||
| if cfg.MaxNumPaths != nil { | ||
| restrictions.MaxNumPaths = uint8(*cfg.MaxNumPaths) | ||
| } |
There was a problem hiding this comment.
I think overflow checks are reasonable.
| if !blind { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
could we define the config as an option so we can remove the blinded param which feels somehow hacky ?
|
Hmm thought about it now and I think the cancel payment can really be a problem for Hodlinvoices, because nodes will convert the error of the final node into an |
There was a problem hiding this comment.
Very nice, did a first pass, nothing that stands out rn 👍. As ziggie mentioned, an itest for invoice cancellation would be great, but will also do some manual tests in the next round.
Hmm thought about it now and I think the cancel payment can really be a problem for Hodlinvoices, because nodes will convert the error of the final node into an INVALID_ONION_BLINDING error, so we have no chance to know whether the destination canceled or an intermediate node failed the payment ?
Right, there's nothing we can really do I think, but the sender needs to deal with this and be smart enough to recognize it's not possible to pay, by exhausting all blinded paths. If it results in an endless loop, we'd need to revisit the error attribution.
| // blindedPathCfg takes the global routing blinded path policies and the given | ||
| // per-payment blinded path config values and uses these to construct the config | ||
| // values passed to the invoice server. | ||
| func (r *rpcServer) blindedPathCfg(blind bool, cfg *lnrpc.BlindedPathConfig) ( |
| } | ||
|
|
||
| bpConfig := r.server.cfg.Routing.BlindedPaths | ||
| defaultDelta := r.cfg.Bitcoin.TimeLockDelta |
There was a problem hiding this comment.
nit: we could set that directly
I believe we should explore the “attribute error” approach if we want to implement hold-blinded invoices. It seems unlikely that we can devise a smart way to detect when the receiver cancels the invoice. While we could theoretically try all possible paths, we also need to consider the behavior of our MPP logic. Currently, it would attempt to send smaller chunks through paths that had previously failed for the original amount, which could significantly increase processing time. Additionally, this could lead to unnecessary payments, and in the worst case, result in on-chain resolutions. From my current perspective, we should avoid introducing this change without a reliable way to identify the final node and handle the error message for blinded payments. |
|
@ellemouton, remember to re-request review from reviewers when ready |
|
!lightninglabs-deploy mute |
Adds route blinding receive functionality to hodl invoices.
Fixes #9024