Skip to content

Upgrade LDK Node #45

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

Merged
merged 10 commits into from
Feb 25, 2025
Merged

Conversation

G8XSU
Copy link
Contributor

@G8XSU G8XSU commented Feb 19, 2025

Upgrade LDK Node to commit [6de350040e0fc5eb9cfcd15fad3919f5a79b82b9]

@G8XSU G8XSU changed the title WIP: Ldk node upgrade Upgrade LDK Node Feb 20, 2025
@G8XSU G8XSU requested a review from arik-so February 20, 2025 23:17
@G8XSU G8XSU marked this pull request as ready for review February 20, 2025 23:17
@arik-so
Copy link
Contributor

arik-so commented Feb 21, 2025

Is this meant to include all the commits from #44? Because it seems to be doing much more than just increasing the LDK Node version.

@G8XSU
Copy link
Contributor Author

G8XSU commented Feb 21, 2025

@arik-so It doesn't include PR-44 commits, these are just the interface changes required to upgrade ldk/ldk-node.

@G8XSU G8XSU force-pushed the ldk-node-upgrade-for-log branch 2 times, most recently from 0c26c06 to 8f64073 Compare February 21, 2025 23:15
@arik-so
Copy link
Contributor

arik-so commented Feb 24, 2025

Looks fine to me, just adding some context to some of the commits might be a good idea.

LDK Node MSRV has changed to 1.75.0, hence we update
our MSRV as well.
In PaymentForwarded events, we now have access to prev_node_id
and next_node_id.
In PaymentForwarded events, we now have access to prev_node_id
and next_node_id.
Since they are not required for existing logging.
Bolt11 Send API now supports setting description_hash
instead of description, account for this api change in ldk.
Onchain sends now support setting a custom fee-rate,
account for this api change in ldk.
LDK Node now supports logging through log facade and
there are minor associated api changes.
@G8XSU G8XSU force-pushed the ldk-node-upgrade-for-log branch from 8f64073 to b887021 Compare February 24, 2025 22:27
@G8XSU
Copy link
Contributor Author

G8XSU commented Feb 24, 2025

Added.

// The on-chain transaction is confirmed in the best chain.
message Confirmed {
// The hash of the block in which the transaction was confirmed.
string block_hash = 1;
Copy link
Contributor

Choose a reason for hiding this comment

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

should this be a string? If so, should we point out it's hex, and whether it's big endian? Because bitcoind can make that a pain with their little endian shenanigans

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it should be a string, reasoning: #14

IIUC, hex representation itself isn't endian specific,
since hashes are just sequence of bytes, we directly use the hex representation. (and no endianess involved in seq of bytes.)
it could be called "big-endian" but will mention it only as hex representation.

clarified to indicate hex representation.

Onchain payments now provide additional information such as
transaction_id and confirmation_status.
@G8XSU G8XSU force-pushed the ldk-node-upgrade-for-log branch from b887021 to 50f9f6b Compare February 25, 2025 01:06
@G8XSU G8XSU merged commit cd2a532 into lightningdevkit:main Feb 25, 2025
6 checks passed
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.

2 participants