Skip to content

Fix and test in-flight HTLC scoring in between retries #2020

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

Conversation

valentinewallace
Copy link
Contributor

Turns out that untested code really is broken code.

I think we don't need to do something similar for the first_hops because retries know what scids failed previously. That doesn't cover all cases, but on the other hand it saves a vec allocation to not recompute first hops and should be rare, so I think it's good enough(?).

Partially addresses #1932.

@valentinewallace valentinewallace force-pushed the 2023-02-test-inflight-scoring branch from ec20301 to 70f2873 Compare February 7, 2023 19:28
@codecov-commenter
Copy link

codecov-commenter commented Feb 7, 2023

Codecov Report

Base: 87.33% // Head: 87.31% // Decreases project coverage by -0.03% ⚠️

Coverage data is based on head (3ddb8ff) compared to base (be4bb58).
Patch coverage: 87.87% of modified lines in pull request are covered.

❗ Current head 3ddb8ff differs from pull request most recent head 2b2965f. Consider uploading reports for the commit 2b2965f to get more accurate results

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2020      +/-   ##
==========================================
- Coverage   87.33%   87.31%   -0.03%     
==========================================
  Files         100      100              
  Lines       44160    44168       +8     
  Branches    44160    44168       +8     
==========================================
- Hits        38568    38566       -2     
- Misses       5592     5602      +10     
Impacted Files Coverage Δ
lightning/src/ln/functional_tests.rs 97.20% <ø> (-0.01%) ⬇️
lightning/src/ln/onion_route_tests.rs 95.25% <ø> (ø)
lightning/src/ln/shutdown_tests.rs 96.36% <ø> (-0.01%) ⬇️
lightning/src/util/test_utils.rs 67.02% <82.50%> (+2.07%) ⬆️
lightning/src/ln/outbound_payment.rs 80.28% <92.30%> (+0.08%) ⬆️
lightning-invoice/src/utils.rs 96.14% <100.00%> (ø)
lightning/src/ln/channelmanager.rs 86.91% <100.00%> (-0.02%) ⬇️
lightning/src/ln/functional_test_utils.rs 88.65% <100.00%> (+0.17%) ⬆️
lightning/src/ln/payment_tests.rs 96.59% <100.00%> (+0.01%) ⬆️
lightning/src/routing/router.rs 92.65% <100.00%> (-0.16%) ⬇️
... and 4 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@TheBlueMatt
Copy link
Collaborator

Ah the ancient proverbs strike agian.

Needs rebase already lol. Basically LGTM tho, but no-std is sad.

@TheBlueMatt TheBlueMatt added this to the 0.0.114 milestone Feb 7, 2023
@valentinewallace valentinewallace force-pushed the 2023-02-test-inflight-scoring branch 2 times, most recently from 0db65a2 to 6ddf6ba Compare February 8, 2023 23:20
@valentinewallace
Copy link
Contributor Author

Tacked on a small commit to remove unnecessary (as of #1996) Router trait methods

wpaulino
wpaulino previously approved these changes Feb 10, 2023
@TheBlueMatt
Copy link
Collaborator

Needs rebase :(

@valentinewallace
Copy link
Contributor Author

Rebased

@valentinewallace valentinewallace force-pushed the 2023-02-test-inflight-scoring branch from 61ec630 to 3ddb8ff Compare February 13, 2023 21:08
wpaulino
wpaulino previously approved these changes Feb 13, 2023
@TheBlueMatt
Copy link
Collaborator

Note that this only actually tests the InFlightHtlcs creation in send_payment_with_retry. It would be nice to test the two other cases where we have a similar constructor in channelmanager. Can happen later though.

@TheBlueMatt
Copy link
Collaborator

CI failure is #2032, will need to kick it after other jobs complete, nbd.

@TheBlueMatt TheBlueMatt merged commit 2edb3f1 into lightningdevkit:main Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants