Skip to content

Conversation

@TheBlueMatt
Copy link
Collaborator

Fixes #125
Fixes #109

Also adds lightning-transaction-sync to the bindings.

Up until now we'd always relied on `Vec`s being imported through a
prelude, but LDK sometimes forgets to keep doing that. Thus, here
we move to also accepting `alloc::vec::Vec` by mapping it to `Vec`
during name resolution.
@TheBlueMatt TheBlueMatt force-pushed the main branch 5 times, most recently from 671222d to 636543d Compare June 6, 2024 00:25
@TheBlueMatt
Copy link
Collaborator Author

Ugh, looks like we can't do lightning-transaction-sync. It currently depends on rustls, which depends on ring, which depends on the latest version of cc (with no reason given aside from that its "the minimum we've tested" 🤦...ring is so stupid) which we can't use because it breaks -Zbuild-std which we require for macOS builds.

@tnull
Copy link

tnull commented Jun 6, 2024

Ugh, looks like we can't do lightning-transaction-sync. It currently depends on rustls, which depends on ring, which depends on the latest version of cc (with no reason given aside from that its "the minimum we've tested" 🤦...ring is so stupid) which we can't use because it breaks -Zbuild-std which we require for macOS builds.

Hum, I'm a bit confused by this:
a) locally I can't reproduce the build failure, genbindings.sh seems to run just fine (although this is on an aarch64 mac)
b) I tried building cc with -Zbuild-std which also succeeds without issue.
c) Why is the inclusion of cc an issue in this case, but not for others (e.g., bitcoin also depends on it I believe?)

Are we positive we still need the pin? Also, I'm a bit confused why we need it exactly and why need to build with -Zbuild-std, which we however only seem to do in the follow-up tests?

@TheBlueMatt
Copy link
Collaborator Author

The issue will only trigger when trying to build for macOS from Linux (which seems to be the only way to get deterministic builds for macOS, see the CI file). It's cause -Zbuild-std doesn't use the Cargo lock file and fails with newer cc. The only way I know how to fix this is to delete the new cc out of the local cargo registry and build with --offline.

@TheBlueMatt
Copy link
Collaborator Author

We can figure out lightning-transaction-sync in a later release.

@TheBlueMatt TheBlueMatt merged commit 05c3f9d into lightningdevkit:main Jun 6, 2024
@tnull
Copy link

tnull commented Jun 6, 2024

We can figure out lightning-transaction-sync in a later release.

😩

@TheBlueMatt
Copy link
Collaborator Author

Doesn't have to take long, I'm just at a loss for what we can practically do here, at least until we get a different HTTP client that doesn't need ring.

@tnull
Copy link

tnull commented Jun 6, 2024

Doesn't have to take long, I'm just at a loss for what we can practically do here, at least until we get a different HTTP client that doesn't need ring.

Yeah, although it's not the HTTP client that pulls in the dependency here, but rustls, which is the most reliable (and actually ~only) TLS option to provide TLS support across different architectures.

@TheBlueMatt
Copy link
Collaborator Author

rustls has support for various crypto providers, though, at least in newer versions, so maybe we can use that? aws-lc-rs may not have the same issues as ring.

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.

ChannelDetails::new is now no longer exposed Expose Hostname as_str

2 participants