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.

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

cmd/commands: add validation for MPP parameters #9238

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
30 changes: 29 additions & 1 deletion lnrpc/routerrpc/router_backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -800,6 +800,30 @@
return route, nil
}

// maybeValidateMPPParams validates that the MPP parameters are compatible with the
// payment amount. It returns an error if the parameters don't allow the full
// payment amount to be sent.
// maybeValidateMPPParams could be enhanced with additional checks

Check failure on line 806 in lnrpc/routerrpc/router_backend.go

View workflow job for this annotation

GitHub Actions / lint code

Comment should end in a period (godot)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont think we need this last line. Validation functions can always be enhanced - this is implied

func maybeValidateMPPParams(amt lnwire.MilliSatoshi, maxParts uint32,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you can just call it validateMPPParams

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, i think it may be worth adding a test for this. Perhaps there is an itest we can expand on just to make sure the call to SendPaymentV2 fails as expected

maxShardSize lnwire.MilliSatoshi) error {

// Early return if MPP is not being used
if maxShardSize == 0 {
return nil
}

// Calculate maximum possible amount
maxPossibleAmount := lnwire.MilliSatoshi(maxParts) * maxShardSize

if amt > maxPossibleAmount {
return fmt.Errorf("payment amount %v exceeds maximum possible "+
"amount %v with max_parts=%v and max_shard_size_msat=%v",
amt, maxPossibleAmount, maxParts, maxShardSize)
}

return nil
}

// extractIntentFromSendRequest attempts to parse the SendRequest details
// required to dispatch a client from the information presented by an RPC
// client.
Expand Down Expand Up @@ -1174,7 +1198,11 @@

payIntent.DestFeatures = features
}

err = maybeValidateMPPParams(
payIntent.Amount, maxParts, lnwire.MilliSatoshi(rpcPayReq.MaxShardSizeMsat))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: formatting

if err != nil {
return nil, fmt.Errorf("invalid MPP parameters: %v", err)

Check failure on line 1204 in lnrpc/routerrpc/router_backend.go

View workflow job for this annotation

GitHub Actions / lint code

non-wrapping format verb for fmt.Errorf. Use `%w` to format errors (errorlint)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/%v/%w

}
// Do bounds checking with the block padding so the router isn't
// left with a zombie payment in case the user messes up.
err = routing.ValidateCLTVLimit(
Expand Down
Loading