routerrpc: add validation to MPP params - #9603
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 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (
|
91c0f3d to
f4513f2
Compare
yyforyongyu
left a comment
yyforyongyu
left a comment
There was a problem hiding this comment.
Thanks for the PR! Left a few comments re the format and test.
| FeeLimitMsat: noFeeLimitMsat, | ||
| PaymentRequest: payReq, | ||
| MaxParts: 10, | ||
| MaxShardSizeMsat: 30_000_000, |
There was a problem hiding this comment.
why do we need to set MaxShardSizeMsat here?
There was a problem hiding this comment.
To validate that when the mpp params permit sending the full payment amount, the payment is successfully settled. I'll add a comment in the test to make this clear.
There was a problem hiding this comment.
Instead of just adding here the MaxShardSizeMsat please create a separate test which tests exactly the validation. This separates the test cases nicely and can be build integrating your test above testValidateMPPParams. First trying a lower shard amount and then increasing it and succeeding.
| // testValidateMPPParams validates that a payment with multipath parameters | ||
| // exceeding the allowed maximum amount fails with the expected error. | ||
| func testValidateMPPParams( | ||
| ht *lntest.HarnessTest, mts *mppTestScenario, paymentAmt btcutil.Amount, |
There was a problem hiding this comment.
the format is not quite right, see https://github.com/lightningnetwork/lnd/blob/master/docs/code_formatting_rules.md#wrapping-long-function-definitions
| ) | ||
|
|
||
| // Validate that multipath payment parameters are enforced correctly. | ||
| testValidateMPPParams(ht, mts, paymentAmt) |
There was a problem hiding this comment.
Also check this in testSendToRouteMultiPath?
f4513f2 to
480c880
Compare
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
Thanks for the fix, left some comments.
| // exceeding the allowed maximum amount fails with the expected error. | ||
| func testValidateMPPParams(ht *lntest.HarnessTest, mts *mppTestScenario, | ||
| paymentAmt btcutil.Amount) { | ||
| // Create an invoice from Bob for the payment. |
| FeeLimitMsat: noFeeLimitMsat, | ||
| PaymentRequest: payReq, | ||
| MaxParts: 10, | ||
| MaxShardSizeMsat: 30_000_000, |
There was a problem hiding this comment.
Instead of just adding here the MaxShardSizeMsat please create a separate test which tests exactly the validation. This separates the test cases nicely and can be build integrating your test above testValidateMPPParams. First trying a lower shard amount and then increasing it and succeeding.
ae6dfb4 to
791dd97
Compare
| // payment amount. In other words, the parameters are invalid if | ||
| // they do not permit sending the full payment amount. | ||
| if rpcPayReq.MaxShardSizeMsat > 0 { | ||
| if payIntent.MaxParts > MaxPartsUpperLimit { |
There was a problem hiding this comment.
this check should be moved to where we assign payIntent.MaxParts at L863 so we can exit early.
There was a problem hiding this comment.
Let's put it at L873, since the MPP will only be used when MaxShardSizeMsat > 0. So, if MaxShardSizeMsat is not being set, then there is no issue with the value of MaxParts?
| // Validate that the MPP parameters are compatible with the | ||
| // payment amount. In other words, the parameters are invalid if | ||
| // they do not permit sending the full payment amount. | ||
| if rpcPayReq.MaxShardSizeMsat > 0 { |
There was a problem hiding this comment.
think we should check payIntent.MaxShardAmt for clarity.
| // \ / | ||
| // \__ Dave ____/ | ||
| // | ||
| paymentAmt := mts.setupSendPaymentCase() |
There was a problem hiding this comment.
For a simple test like this, I'd say we use ht.CreateSimpleNetwork instead as we are not testing the routing behavior here.
| // | ||
| paymentAmt := mts.setupSendPaymentCase() | ||
|
|
||
| // First, test that Alice's payment to Bob fails when the multipath |
There was a problem hiding this comment.
The comment needs to be updated.
|
|
||
| success := ht.Run(test.name, func(t *testing.T) { | ||
| // Bob creates an invoice for the payment. | ||
| payReqs, _, _ := ht.CreatePayReqs(mts.bob, paymentAmt, |
There was a problem hiding this comment.
I think we can instead create a single invoice for the whole test case as we know only the last send payment will succeed.
| require.Equal(ht, hex.EncodeToString(invoices[0].RPreimage), | ||
| payment.PaymentPreimage, "preimage doesn't match") | ||
|
|
||
| // Verify that Bob records the invoice as settled for the full amount. |
There was a problem hiding this comment.
Can use ht.AssertInvoiceSettled instead.
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
I think we should just add a unit-test instead of adding a itest.
Moreover let's remove the minRequired parts check yy has a valid point.
| DefaultMaxParts = 16 | ||
|
|
||
| // MaxPartsUpperLimit defines the maximum allowable number of splits | ||
| // for MPP when the user is attempting to send a payment. |
| // fails with the expected error when the payment amount exceeds the allowed | ||
| // maximum, and that when the multipath parameters permit the full payment | ||
| // amount, the payment is successfully settled. | ||
| func testValidateMPPParams(ht *lntest.HarnessTest) { |
There was a problem hiding this comment.
Personally I think a itest for this change might be a bit overkill, can we not just test the extractIntentFromSendRequest via a unit-test in router_backend_test.go ?
791dd97 to
7ae2f39
Compare
NishantBansal2003
changed the title
7ae2f39 to
314b7b1
Compare
ziggie1984
left a comment
ziggie1984
left a comment
There was a problem hiding this comment.
Nice, left some comments.
| maxPartsInput uint32 | ||
| shardAmountMsat uint64 |
There was a problem hiding this comment.
Nit: Why not keep the naming here from the above code ?
maxShardAmt, maxParts ?
| valid bool | ||
| expectedErrorMsg string | ||
| } | ||
|
|
There was a problem hiding this comment.
Missing godoc, would be great if you could properly test the whole extract func. otherwise create a comment which specifies that you are in specific only testing the shard multipart payment logic.
There was a problem hiding this comment.
I have added tests for the entire extract function.
| } | ||
|
|
||
| for _, testCase := range testCases { | ||
| testCase := testCase |
There was a problem hiding this comment.
I think this iis not necessary anymore with the go compiler option loopvar
| for _, testCase := range testCases { | ||
| testCase := testCase | ||
| t.Run(testCase.name, func(t *testing.T) { | ||
| runExtractIntentTest(t, backend, testCase) |
There was a problem hiding this comment.
add t.Parallel() to speed up testing.
314b7b1 to
49cd630
Compare
MPins
left a comment
MPins
left a comment
There was a problem hiding this comment.
Well done 👍
Left some comments that might help make the test a bit cleaner.
| } | ||
| } | ||
|
|
||
| type ExtractIntentTestCase struct { |
There was a problem hiding this comment.
nit: this struct doesn't need to be exported, so it can start with a lowercase letter.
| backend: &RouterBackend{ | ||
| MaxTotalTimelock: 1000, | ||
| }, |
There was a problem hiding this comment.
nit: Setting MaxTotalTimelock is not needed.
| backend: &RouterBackend{ | ||
| MaxTotalTimelock: 1000, | ||
| }, |
There was a problem hiding this comment.
nit: Setting MaxTotalTimelock is not needed.
| backend: &RouterBackend{ | ||
| MaxTotalTimelock: 1000, | ||
| }, |
There was a problem hiding this comment.
nit: Setting MaxTotalTimelock is not needed.
| backend: &RouterBackend{ | ||
| MaxTotalTimelock: 1000, | ||
| }, |
There was a problem hiding this comment.
nit: Setting MaxTotalTimelock is not needed.
1adc9fd to
e92ae2a
Compare
There was a problem hiding this comment.
Looking closer, I found some missing tests and suggest a few naming changes to make it clearer what each test is actually doing.
Besides the suggested additions I made, we could also add a test for the case where the invoice has an amount specified and the payer is not allowed to override it. However, to test that properly, we would need a non-expired invoice. I don't think it's strictly necessary, though.
| "outgoing_chan_ids are mutually exclusive", | ||
| }, | ||
| { | ||
| name: "Invalid last hop pubkey", |
There was a problem hiding this comment.
nit: I would change the name of the test to "Invalid last hop pubkey length".
| name: "Invalid last hop pubkey", | ||
| backend: &RouterBackend{}, | ||
| sendReq: &SendPaymentRequest{ | ||
| OutgoingChanId: 38484, |
There was a problem hiding this comment.
nit: Setting OutgoingChanId is not needed.
| "mutually exclusive", | ||
| }, | ||
| { | ||
| name: "Dest custom records with type below minimum", |
There was a problem hiding this comment.
nit: I would change the name of the test to "Dest custom records with type below minimum range"
| name: "Custom record entry with TLV type below " + | ||
| "minimum", |
There was a problem hiding this comment.
nit: I would change the name of the test to "Custom record entry with TLV type below minimum range"
| "payment_request cannot appear together", | ||
| }, | ||
| { | ||
| name: "Invalid payment request", |
There was a problem hiding this comment.
nit: I would change the name of the test to "Invalid payment request length"
| "support AMP payments", | ||
| }, | ||
| { | ||
| name: "Invalid payment hash", |
There was a problem hiding this comment.
nit: I would change the name of the test to "Invalid payment hash length"
| }, | ||
| valid: false, | ||
| expectedErrorMsg: "invalid vertex length", | ||
| }, |
There was a problem hiding this comment.
You can add the test below to increase the code coverage:
{
name: "total time lock exceeds max allowed",
backend: &RouterBackend{
MaxTotalTimelock: 1000,
},
sendReq: &SendPaymentRequest{
CltvLimit: 1001,
},
valid: false,
expectedErrorMsg: "total time lock of 1001 exceeds "+
"max allowed 1000",
},
| valid: false, | ||
| expectedErrorMsg: "sat and msat arguments are " + | ||
| "mutually exclusive", | ||
| }, |
There was a problem hiding this comment.
You can add the test below to increase the code coverage:
{
name: "Fee limit cannot be negative",
backend: &RouterBackend{},
sendReq: &SendPaymentRequest{
FeeLimitSat: -1,
},
valid: false,
expectedErrorMsg: "amount cannot be negative",
},
Yes, we require a non-expired invoice, which is why I was unable to add a test for it. Perhaps, in such cases, "itest" would suffice? |
e92ae2a to
f659db7
Compare
|
|
you can use: |
f659db7 to
8bd91d5
Compare
Thanks, this helped! I have also updated the tests for the latest changes. |
|
@ziggie1984: review reminder |
| if payIntent.MaxShardAmt != nil { | ||
| maxPossibleAmount := (*payIntent.MaxShardAmt) * | ||
| lnwire.MilliSatoshi(payIntent.MaxParts) | ||
| if payIntent.Amount > maxPossibleAmount { |
| } | ||
| } | ||
|
|
||
| type extractIntentTestCase struct { |
There was a problem hiding this comment.
Nit: missing godoc for this struct
8bd91d5 to
48e33c9
Compare
|
Hmm, CI seems to have gotten stuck on this one. Can you please push again to re-trigger (rebase on master for example)? |
Adds validation to ensure that MPP parameters are compatible with the payment amount before attempting the payment. This prevents payments from entering a path finding loop that would eventually timeout. Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
|
Rebased and pushed the commits to re-trigger CI, so we can merge this. |
7 participants
Change Description
Fixes: #8916
Adds validation to ensure that MPP parameters are compatible with the payment amount before attempting the payment. This prevents payments from entering a path finding loop that would eventually timeout.
Steps to Test
Added itest to validate that a payment with multipath parameters exceeding the allowed maximum amount fails with the expected error.
Pull Request Checklist
Testing
Code Style and Documentation
[skip ci]in the commit message for small changes.📝 Please see our Contribution Guidelines for further guidance.