Fix fork CI - #9
Merged
Merged
Conversation
…D warnings lib.rs imports bitcoin::OutPoint directly (added with the coin control and remote-closed sweep work), which shadows the ffi glob re-export. Under --features uniffi the re-export is unused, and CI's -D warnings build fails on it. Both names refer to the same type.
rustdoc::private_intra_doc_links is denied in lib.rs, so cargo doc (and CI's docs-rs job) fails on these links. Keep the name in code formatting without the link.
c38d28f added a third payment_timeout_secs parameter to Bolt11Payment::send, and 02c62cb updated the Rust integration tests and benches, but the CLN and LND integration tests and the Kotlin and Python binding tests still called the two-argument form, so check-cln, check-lnd, check-kotlin and check-python failed to compile or errored.
Backport of upstream bfad8e6: vss-server moved its Rust server out of the rust/ subdirectory, so the VSS integration job failed at cd.
Rust 1.99 reports trailing semicolons inside macros used in expression position, which fails CI's -D warnings build on stable. The upstream 0.7.0-rc.1 base fails the same way under 1.99, so this is toolchain drift, not a fork regression. Same approach as upstream b6e37fa, which does not cherry-pick cleanly onto this base.
vss-server main gates NoopAuthorizer behind --cfg noop_authorizer (vss-server 52fd919) and otherwise requires an Authorization header, so the fork's VSS tests, which send none, fail with 'Authorization header not found'. Upstream ldk-node moved its tests to signature auth instead (e4da11e), which depends on VssStore changes this fork does not have. Start the server the way upstream's no-auth workflow does.
The crate-level example (compiled as a doctest) and the README still used the two-argument Bolt11Payment::send.
Append the trailing per-call retry-timeout argument (None) at every bolt11/bolt12/spontaneous send call site in tests/ and benches/. These targets stopped compiling when the configurable payment retry timeout parameter was added; with this, cargo check --all-targets is green again. (cherry picked from commit 02c62cb)
CI's format check fails on files changed by fork commits; the upstream base (3ed1d13) is clean under the same rustfmt. Formatting only.
Author
|
/athena review |
![athena-zeusln[bot]](https://avatars.githubusercontent.com/in/4862658?s=60&v=4){kind=small}
There was a problem hiding this comment.
Athena Review
Pinned commit: e0e4fd8
anthropic:claude-opus-5-5: completed openai:gpt-6-sol: completed
1 validated findings. This is a nonblocking COMMENT review, not an approval. No findings does not establish correctness.
Limitations:
- src/lib.rs: patch omitted by context bound
- src/liquidity.rs: patch omitted by context bound
- tests/common/mod.rs: patch omitted by context bound
- tests/integration_tests_cln.rs: patch omitted by context bound
- tests/integration_tests_lnd.rs: patch omitted by context bound
- tests/integration_tests_rust.rs: patch omitted by context bound
- Patches for src/lib.rs, src/liquidity.rs, tests/common/mod.rs, tests/integration_tests_cln.rs, tests/integration_tests_lnd.rs and tests/integration_tests_rust.rs were omitted. API changes behind the new extra
Noneargument to the Bolt11 and spontaneous send calls could not be verified. - src/ffi/types.rs drops
OutPointfrom the publicbitcoinre-export. Without the UDL file or the rest of the ffi module, it cannot be confirmed whether OutPoint is still exported for uniffi bindings and downstream users, or whether removing it breaks the bindings. - The VSS workflow now builds from the vss-server root with
RUSTFLAGS=--cfg=noop\_authorizerand--no-default-features, using the config pathserver/vss-server-config.toml. No upstream vss-server layout or feature evidence was supplied to confirm this path and these flags are valid for the checked-out revision. - The remaining Rust changes appear to be rustfmt-only reformatting, import reordering and rustdoc link-to-code-span edits. No behavioral change was identified in the visible hunks.
- The omitted src/lib.rs patch prevents verification of whether another OutPoint re-export preserves the API.
- The VSS server manifest and feature configuration are not supplied, so the workflow command's compatibility with the new working directory cannot be verified.
| use bitcoin::hashes::Hash; | ||
| use bitcoin::secp256k1::PublicKey; | ||
| pub use bitcoin::{Address, BlockHash, FeeRate, Network, OutPoint, ScriptBuf, Txid}; | ||
| pub use bitcoin::{Address, BlockHash, FeeRate, Network, ScriptBuf, Txid}; |
There was a problem hiding this comment.
MEDIUM: OutPoint public re-export removed
Downstream code importing OutPoint through this public re-export will fail to compile after the removal. Preserve the re-export, or provide a compatible alias if it must move.
Reviewers: openai:gpt-6-sol
Author
There was a problem hiding this comment.
This doesn't apply. The removed line is not a public export.
ffiis a private module (mod ffi;insrc/lib.rs), and its contents are only pulled in by a privateuse ffi::*;under#[cfg(feature = "uniffi")]. No downstream crate could importOutPointthrough it.lib.rsalso importsbitcoin::OutPointdirectly, and that explicit import shadows the glob. That is why rustc reported theffire-export as unused under--features uniffi, which failed CI's-D warningsbuild.- The public path is
ldk_node::bitcoin::OutPoint, through the existingpub use { bitcoin, ... }. This PR doesn't change it. - The UDL
OutPointdictionary still resolves to the samebitcoin::OutPointthrough the direct import. check-kotlin, check-python and check-swift pass on this PR.
Merged
7 of 27 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
CI on the fork has been failing on every workflow except Swift, Benchmarks and SemVer. This makes all nine workflows pass again on
zeus. #8 will be rebased on top of this afterwards.The upstream base (0.7.0-rc.1,
3ed1d13) passed all of these jobs upstream. Most of the failures came from fork changes; three came from external changes since then.Caused by fork changes:
cargo fmtonly, no code changes.-D warningsuniffi build:lib.rsimportsbitcoin::OutPointdirectly (added with the coin control work), which shadows theffire-export and leaves it unused under--features uniffi. I dropped the re-export.DualStoredocs linked to the privatevss_push_verdict, andrustdoc::private_intra_doc_linksis denied.sendcall sites:payment_timeout_secs(c38d28f) was never threaded through the tests. I updated the Rust integration tests and benches (cherry-picked from02c62cbon Re-pin rust-lightning to zeus-0.2 (v0.2.5 + Zeus patches) #5, conflict hunks only), the CLN and LND integration tests, the Kotlin and Python binding tests, the crate-level doctest and the README.External drift:
rust/: backport of upstreambfad8e6.52fd919it accepts unauthenticated requests only when built with--cfg noop_authorizer. The fork's VSS tests send no auth, so CI now starts the server the way upstream's no-auth workflow does. Upstream moved its own tests to signature auth (e4da11e), which needs VssStore changes this fork doesn't have.-D warningsbuild on stable. The untouched upstream base fails the same way. This uses the same approach as upstreamb6e37fa, applied by hand because it doesn't cherry-pick onto this base.Testing:
cargo fmt --check. The-D warningsbuild passes with and withoutuniffion 1.99.0 and 1.85.0 (MSRV), and the-D warningsuniffi release check passes.cargo docis clean, lib tests pass 23/23, doctests 2/2, and all test and bench targets compile.ci/fix-fork-ci), passed all nine workflows. That includes the regtestintegration_tests_rustsuite (32/32), CLN, LND, VSS, Kotlin, Python and Swift.