Skip to content

Update to upstream LDK after 4263 and 4370 #780

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

Closed
TheBlueMatt wants to merge 1 commit into from
+47 −28
Closed

Update to upstream LDK after 4263 and 4370 #780

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
25 changes: 14 additions & 11 deletions Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,17 +154,20 @@ harness = false
#lightning-liquidity = { git = "https://github.com/lightningdevkit/rust-lightning", branch = "main" }
#lightning-macros = { git = "https://github.com/lightningdevkit/rust-lightning", branch = "main" }

#lightning = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "21e9a9c0ef80021d0669f2a366f55d08ba8d9b03" }
#lightning-types = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "21e9a9c0ef80021d0669f2a366f55d08ba8d9b03" }
#lightning-invoice = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "21e9a9c0ef80021d0669f2a366f55d08ba8d9b03" }
#lightning-net-tokio = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "21e9a9c0ef80021d0669f2a366f55d08ba8d9b03" }
#lightning-persister = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "21e9a9c0ef80021d0669f2a366f55d08ba8d9b03" }
#lightning-background-processor = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "21e9a9c0ef80021d0669f2a366f55d08ba8d9b03" }
#lightning-rapid-gossip-sync = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "21e9a9c0ef80021d0669f2a366f55d08ba8d9b03" }
#lightning-block-sync = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "21e9a9c0ef80021d0669f2a366f55d08ba8d9b03" }
#lightning-transaction-sync = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "21e9a9c0ef80021d0669f2a366f55d08ba8d9b03" }
#lightning-liquidity = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "21e9a9c0ef80021d0669f2a366f55d08ba8d9b03" }
#lightning-macros = { git = "https://github.com/lightningdevkit/rust-lightning", rev = "21e9a9c0ef80021d0669f2a366f55d08ba8d9b03" }
[patch.'https://github.com/lightningdevkit/rust-lightning']
# Cargo won't let us patch a git dependency with another rev from the same git dependency, so we
# have to pick another copy of `rust-lightning` and pin the rev.
lightning = { git = "https://git.bitcoin.ninja/rust-lightning", rev = "0bc5c95484345c6289dffd9a379982a9b66d688f" }
Copy link
Collaborator

@tnull tnull Feb 2, 2026

Choose a reason for hiding this comment

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

I don't see how this is preferable. We'll still need to keep that third repository in-sync.

Copy link
Contributor

@joostjager joostjager Feb 3, 2026

Choose a reason for hiding this comment

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

It doesn't work for me out of the box also

    Updating git repository `https://git.bitcoin.ninja/rust-lightning`
warning: spurious network error (3 tries remaining): unexpected http status code: 413; class=Http (34)
warning: spurious network error (2 tries remaining): unexpected http status code: 413; class=Http (34)
warning: spurious network error (1 try remaining): unexpected http status code: 413; class=Http (34)
error: failed to load source for dependency `lightning`

Caused by:
  Unable to update https://git.bitcoin.ninja/rust-lightning?rev=0bc5c95484345c6289dffd9a379982a9b66d688f

Caused by:
  failed to fetch into: /Users/joost/.cargo/git/db/rust-lightning-94d3943701373680

Caused by:
  network failure seems to have happened
  if a proxy or similar is necessary `net.git-fetch-with-cli` may help here
  https://doc.rust-lang.org/cargo/reference/config.html#netgit-fetch-with-cli

Caused by:
  unexpected http status code: 413; class=Http (34)

Copy link
Contributor Author

@TheBlueMatt TheBlueMatt Feb 3, 2026

Choose a reason for hiding this comment

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

I don't see how this is preferable. We'll still need to keep that third repository in-sync.

FWIW this repository is ~always in sync. Annoying, yes, but this shouldn't be an issue. We could also have a repo that's more automated.

Copy link
Contributor Author

@TheBlueMatt TheBlueMatt Feb 3, 2026

Choose a reason for hiding this comment

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

It doesn't work for me out of the box also

Wat? I've never seen this....I bumped the max but...weird, honestly.

Copy link
Collaborator

@tnull tnull Feb 3, 2026

Choose a reason for hiding this comment

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

It doesn't work for me out of the box also

Wat? I've never seen this....I bumped the max but...weird, honestly.

I think at the time Joost tested it lightningdevkit/rust-lightning#4370 wasn't merged. I then went reviewed and landed that PR, bumped bitcoin-payment-instructions, opened the other PR, and asked him to review so we can be unblocked, as he in particular was held up today due to LDK's API breaks. IMO it shows once more that we need to go back to the drawing board on depedency mgmt and merge policy. Maybe we need a hard CI check in LDK that requires fixes are up before we merge any breaking change.

Copy link
Contributor Author

@TheBlueMatt TheBlueMatt Feb 3, 2026

Choose a reason for hiding this comment

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

Hmm, should still have worked. the Commit here was the top commit on that PR, rather than the future merge commit. I think it was just a git oddity hitting an nginx max request body size issue.

As for the general issue, I'm a bit confused why someone was blocked on an update to LDK Node syncing to two trivial upstream PRs? Testing against upstream without those PRs should have been pretty easy, and waiting a day for the update doesn't seem like an issue to me.

But, either way, the limitation in this case wasn't a desire for rust-lightning devs to update LDK Node, but rather that it currently requires you to do something on your bitcoin-payment-instructions fork, which means you're the only one who actually can update LDK Node.

I wonder if we can't automate that part - just have a git repo that maintains a bitcoin-payment-instructions that gets its Cargo.toml auto-updated (as long as no API changes have impacted it) and maintains branches for the last few week's worth of commits to rust-lightning so that there's always a commit to point to.

Copy link
Collaborator

@tnull tnull Feb 4, 2026

Choose a reason for hiding this comment

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

As for the general issue, I'm a bit confused why someone was blocked on an update to LDK Node syncing to two trivial upstream PRs? Testing against upstream without those PRs should have been pretty easy, and waiting a day for the update doesn't seem like an issue to me.

I'll let @joostjager speak to that, but: Yes, it's all doable of course, but if you have an LDK PR that's based on current main that you want to test, it costs you at least 10-15 minutes every time to figure out what's going on on the LDK Node side, which exact PRs broke something, check which API changes have already been accounted for, then create a new branch with the changes you want to test, rebase that, account for potential rebase conflicts. Then every time you make changes to that forked branch you'll need to re-apply them to your actual PR branch, potentially running into conflicts again, which is all pretty messy an error prone. Again, all doable, but I can see that it's just an annoying procedure to deal with that forces you to pay attention to things that you actually don't want spend time/brain resources on right now and you end up cleaning up after the API breaks that others introduced.

But, either way, the limitation in this case wasn't a desire for rust-lightning devs to update LDK Node, but rather that it currently requires you to do something on your bitcoin-payment-instructions fork, which means you're the only one who actually can update LDK Node.

Yeah, all ears for alternatives.

I wonder if we can't automate that part - just have a git repo that maintains a bitcoin-payment-instructions that gets its Cargo.toml auto-updated (as long as no API changes have impacted it) and maintains branches for the last few week's worth of commits to rust-lightning so that there's always a commit to point to.

Right, that's basically automating what I've been doing over at https://github.com/tnull/bitcoin-payment-instructions/tree/2025-12-ldk-node-base. I generally like the idea of just automating that branch an ensuring that there is a bump commit for every LDK commit, so anybody can just pick the right commit. However, that will break as soon as we break any of the APIs actually used by bitcoin-payment-instructions, and then it might become even more confusing?

Copy link
Contributor

@joostjager joostjager Feb 4, 2026

Choose a reason for hiding this comment

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

I'll let @joostjager speak to that, but: Yes, it's all doable of course [...]

Nothing to add.

I still think many of these pain points could be alleviated by moving to a monorepo. We'd trade the current problems (git rev juggling, forked branches, mirror sync, after the fact breakage cleanup) for a simpler one: whoever changes an API updates ldk-node in the same PR, enforced by CI. We might lose some release cadence flexibility, but I think we can come up with a reasonable plan for that.

lightning-types = { git = "https://git.bitcoin.ninja/rust-lightning", rev = "0bc5c95484345c6289dffd9a379982a9b66d688f" }
lightning-invoice = { git = "https://git.bitcoin.ninja/rust-lightning", rev = "0bc5c95484345c6289dffd9a379982a9b66d688f" }
lightning-net-tokio = { git = "https://git.bitcoin.ninja/rust-lightning", rev = "0bc5c95484345c6289dffd9a379982a9b66d688f" }
lightning-persister = { git = "https://git.bitcoin.ninja/rust-lightning", rev = "0bc5c95484345c6289dffd9a379982a9b66d688f" }
lightning-background-processor = { git = "https://git.bitcoin.ninja/rust-lightning", rev = "0bc5c95484345c6289dffd9a379982a9b66d688f" }
lightning-rapid-gossip-sync = { git = "https://git.bitcoin.ninja/rust-lightning", rev = "0bc5c95484345c6289dffd9a379982a9b66d688f" }
lightning-block-sync = { git = "https://git.bitcoin.ninja/rust-lightning", rev = "0bc5c95484345c6289dffd9a379982a9b66d688f" }
lightning-transaction-sync = { git = "https://git.bitcoin.ninja/rust-lightning", rev = "0bc5c95484345c6289dffd9a379982a9b66d688f" }
lightning-liquidity = { git = "https://git.bitcoin.ninja/rust-lightning", rev = "0bc5c95484345c6289dffd9a379982a9b66d688f" }
lightning-macros = { git = "https://git.bitcoin.ninja/rust-lightning", rev = "0bc5c95484345c6289dffd9a379982a9b66d688f" }

#vss-client-ng = { path = "../vss-client" }
#vss-client-ng = { git = "https://github.com/lightningdevkit/vss-client", branch = "main" }
Expand Down
26 changes: 19 additions & 7 deletions src/payment/bolt11.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,10 @@ use std::sync::{Arc, RwLock};
use bitcoin::hashes::sha256::Hash as Sha256;
use bitcoin::hashes::Hash;
use lightning::ln::channelmanager::{
Bolt11InvoiceParameters, Bolt11PaymentError, PaymentId, Retry, RetryableSendFailure,
Bolt11InvoiceParameters, OptionalBolt11PaymentParams, PaymentId,
};
use lightning::ln::outbound_payment::{
Bolt11PaymentError, Retry, RetryableSendFailure,
};
use lightning::routing::router::{PaymentParameters, RouteParameters, RouteParametersConfig};
use lightning_invoice::{
Expand Down Expand Up @@ -109,17 +112,21 @@ impl Bolt11Payment {
}
}

let route_parameters =
let route_params_config =
route_parameters.or(self.config.route_parameters).unwrap_or_default();
let retry_strategy = Retry::Timeout(LDK_PAYMENT_RETRY_TIMEOUT);
let payment_secret = Some(*invoice.payment_secret());

let optional_params = OptionalBolt11PaymentParams {
retry_strategy,
route_params_config,
..Default::default()
};
match self.channel_manager.pay_for_bolt11_invoice(
invoice,
payment_id,
None,
route_parameters,
retry_strategy,
optional_params,
) {
Ok(()) => {
let payee_pubkey = invoice.recover_payee_pub_key();
Expand Down Expand Up @@ -215,17 +222,22 @@ impl Bolt11Payment {
}
}

let route_parameters =
let route_params_config =
route_parameters.or(self.config.route_parameters).unwrap_or_default();
let retry_strategy = Retry::Timeout(LDK_PAYMENT_RETRY_TIMEOUT);
let payment_secret = Some(*invoice.payment_secret());


let optional_params = OptionalBolt11PaymentParams {
retry_strategy,
route_params_config,
..Default::default()
};
match self.channel_manager.pay_for_bolt11_invoice(
invoice,
payment_id,
Some(amount_msat),
route_parameters,
retry_strategy,
optional_params,
) {
Ok(()) => {
let payee_pubkey = invoice.recover_payee_pub_key();
Expand Down
3 changes: 2 additions & 1 deletion src/payment/bolt12.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,8 @@ use std::sync::{Arc, RwLock};
use std::time::{Duration, SystemTime, UNIX_EPOCH};

use lightning::blinded_path::message::BlindedMessagePath;
use lightning::ln::channelmanager::{OptionalOfferPaymentParams, PaymentId, Retry};
use lightning::ln::channelmanager::{OptionalOfferPaymentParams, PaymentId};
use lightning::ln::outbound_payment::Retry;
use lightning::offers::offer::{Amount, Offer as LdkOffer, OfferFromHrn, Quantity};
use lightning::offers::parse::Bolt12SemanticError;
use lightning::routing::router::RouteParametersConfig;
Expand Down
21 changes: 12 additions & 9 deletions src/payment/spontaneous.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,10 @@
use std::sync::{Arc, RwLock};

use bitcoin::secp256k1::PublicKey;
use lightning::ln::channelmanager::{PaymentId, RecipientOnionFields, Retry, RetryableSendFailure};
use lightning::ln::channelmanager::PaymentId;
use lightning::ln::outbound_payment::{
RecipientCustomTlvs, RecipientOnionFields, Retry, RetryableSendFailure,
};
use lightning::routing::router::{PaymentParameters, RouteParameters, RouteParametersConfig};
use lightning::sign::EntropySource;
use lightning_types::payment::{PaymentHash, PaymentPreimage};
Expand Down Expand Up @@ -125,15 +128,15 @@ impl SpontaneousPayment {
*max_channel_saturation_power_of_half;
}

let recipient_fields = match custom_tlvs {
Some(tlvs) => RecipientOnionFields::spontaneous_empty()
.with_custom_tlvs(tlvs.into_iter().map(|tlv| (tlv.type_num, tlv.value)).collect())
.map_err(|e| {
log_error!(self.logger, "Failed to send payment with custom TLVs: {:?}", e);
let mut recipient_fields = RecipientOnionFields::spontaneous_empty();
if let Some(tlvs) = custom_tlvs {
let tlvs_vec = tlvs.into_iter().map(|tlv| (tlv.type_num, tlv.value)).collect();
recipient_fields =
recipient_fields.with_custom_tlvs(RecipientCustomTlvs::new(tlvs_vec).map_err(|()| {
log_error!(self.logger, "Attempted to set payment custom TLVs to a spec-defined value");
Error::InvalidCustomTlvs
})?,
None => RecipientOnionFields::spontaneous_empty(),
};
})?);
}

match self.channel_manager.send_spontaneous_payment(
Some(payment_preimage),
Expand Down
Loading