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.

Sign up for GitHub

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

Jump to bottom

pay: Use the networkheight as current height #7190

Merged
merged 7 commits into from
May 9, 2024
Merged

pay: Use the networkheight as current height #7190

merged 7 commits into from
May 9, 2024

Conversation

cdecker
Copy link
Member

@cdecker cdecker commented Mar 31, 2024

We used to use the sync height, i.e., the height of the last block that
we processed as the offset for all calculations. This can result in CLTVs
that are too close to the current height while still synchronizing with
the blockchain.

This PR adds the networkheight to waitblockheight so we can retrieve
the current height of the block that our bitcoin backend has seen.

This means that CLTV calculations are correct as long as a) the backend
is in sync and b) the peer is in sync.

Sending the payment without sync is safe, because if the peer accepts the
HTLC they ensured the channel is still open and active.

@cdecker cdecker self-assigned this Mar 31, 2024
@cdecker cdecker marked this pull request as draft March 31, 2024 19:15
@cdecker cdecker force-pushed the 202404-pay-networkheight branch 2 times, most recently from 6f36435 to 8593a93 Compare April 1, 2024 07:42
rustyrussell
rustyrussell approved these changes Apr 2, 2024
View reviewed changes
Copy link
Contributor

@rustyrussell rustyrussell left a comment

Choose a reason for hiding this comment

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

Needs fixup folding, but code looks great!

@cdecker cdecker force-pushed the 202404-pay-networkheight branch from 8593a93 to dcea838 Compare April 16, 2024 13:21
@cdecker
Copy link
Member Author

cdecker commented Apr 16, 2024

I had to rework this patch a bit, since the waitblockheight call with blockheight: 0 would not actually return the bitcoin backend's blockheight, but rather the max(start blockheight, current sync height) which may be ok in most cases, but in the case of a long catch up would not (new blocks coming in while we are syncing would not be taken into consideration).

This calls both waitblockheight and getchaininfo to compute a chainlag which is then used as the offset.

@cdecker cdecker requested a review from rustyrussell April 16, 2024 13:45
@cdecker
Copy link
Member Author

cdecker commented Apr 16, 2024

Addressing a bunch of test failures that were reliant on the old behavior now.

@cdecker
Copy link
Member Author

cdecker commented Apr 16, 2024

There is at least one broken test due to the direct_pay_override not respecting the chainlag. I'll look for the correct place to apply them and undraft :-)

@cdecker
Copy link
Member Author

cdecker commented Apr 18, 2024

I found out that we use the delay field in the hops in an inconsistent manner: we use p->start_block + delay when constructing the payload, which now already contains the chainlag, but then the first parameter passed to sendonion is a raw offset from the lightningd syncheight. So I moved the offset calculation into the two spots, and now it works apparently.

It's a bit strange to have lightningd apply the current syncheight, rather than passing in absolute values for the path, but oh well :-)

cdecker added 6 commits April 18, 2024 18:08
@cdecker
pytest: Add a test for the height mismatch logic
a2b1924
@cdecker
pay: Remove the waitheight paymod
9f82613 
It is getting replaced with a paymod that adjusts the chain height
used to offset all calculations from.
@cdecker
pay: Add the chainlag to the payment
778ca80 
The `chainlag` is defined as the positive difference between the
height of the last block processed by the node and the best height
known by the bitcoin backend. The chainlag is positive when we are
still catching up with the blockchain, and `0` otherwise.

The `chainlag` is used as an additional offset to the CLTV values when
sending payments, allowing payments to be sent even before the chain
sync completes.
@cdecker
pay: Implement offset and chainlag computation when paying
6b5f9c3 
This actually uses the `chainlag` to make unsynced payments possible.
@cdecker
pytest: Remove outdated tests
b553c54 
These tests were related to sending early payments, and will now fail,
as the underlying behavior changed.
@cdecker
pay: Enable chainlag offset in direct override paymod
231b163
@cdecker cdecker force-pushed the 202404-pay-networkheight branch from 2d845cc to 231b163 Compare April 18, 2024 16:09
@cdecker cdecker marked this pull request as ready for review April 19, 2024 09:34
@cdecker
Copy link
Member Author

cdecker commented Apr 19, 2024

Ready for review @rustyrussell 🤗

@cdecker cdecker added this to the v24.05 milestone Apr 19, 2024
endothermicdev
endothermicdev approved these changes May 7, 2024
View reviewed changes
Copy link
Collaborator

@endothermicdev endothermicdev left a comment

Choose a reason for hiding this comment

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

Looks great! nits only.

tests/test_pay.py Outdated Show resolved Hide resolved
tests/test_pay.py Outdated Show resolved Hide resolved
tests/test_pay.py Outdated Show resolved Hide resolved
plugins/libplugin-pay.c
@@ -132,6 +133,7 @@ struct payment *payment_new(tal_t *ctx, struct command *cmd,
p->local_invreq_id = NULL;
p->groupid = 0;
p->mods = NULL;
p->chainlag = 0;
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 this initialization is overzealous? We shouldn't access chainlag without purposefully setting it first as far as I can tell.

@cdecker cdecker mentioned this pull request May 8, 2024
Closed
@rustyrussell rustyrussell enabled auto-merge (rebase) May 9, 2024 03:05
@rustyrussell rustyrussell merged commit f34b63f into ElementsProject:master May 9, 2024
35 checks passed
@cdecker cdecker mentioned this pull request May 14, 2024
Closed
@cdecker cdecker mentioned this pull request May 22, 2024
Closed
@cdecker cdecker mentioned this pull request Jul 18, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@endothermicdev endothermicdev endothermicdev approved these changes

@rustyrussell rustyrussell rustyrussell approved these changes

Assignees

@cdecker cdecker

Labels
None yet
Projects
None yet
Milestone
v24.05
Development

Successfully merging this pull request may close these issues.
3 participants
@cdecker @rustyrussell @endothermicdev