Skip to content

Bump electrsd to 0.36.1, fix CI #4180

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

Merged
merged 2 commits into from
Oct 30, 2025
Merged

Bump electrsd to 0.36.1, fix CI #4180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
98c83df
Bump `electrsd` to 0.36.1
tnull Oct 28, 2025
9753e6d
Pin `proptest` again to fix CI
tnull Oct 28, 2025
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
5 changes: 4 additions & 1 deletion ci/ci-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
set -eox pipefail

# Currently unused as we don't have to pin anything for MSRV:
#RUSTC_MINOR_VERSION=$(rustc --version | awk '{ split($2,a,"."); print a[2] }')
RUSTC_MINOR_VERSION=$(rustc --version | awk '{ split($2,a,"."); print a[2] }')

# Some crates require pinning to meet our MSRV even for our downstream users,
# which we do here.
Expand All @@ -17,6 +17,9 @@ PIN_RELEASE_DEPS # pin the release dependencies in our main workspace
# The backtrace v0.3.75 crate relies on rustc 1.82
[ "$RUSTC_MINOR_VERSION" -lt 82 ] && cargo update -p backtrace --precise "0.3.74" --verbose

# proptest 1.9.0 requires rustc 1.82.0
[ "$RUSTC_MINOR_VERSION" -lt 82 ] && cargo update -p proptest --precise "1.8.0" --verbose
Copy link
Collaborator

@TheBlueMatt TheBlueMatt Oct 28, 2025

Choose a reason for hiding this comment

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

If proptest doesnt have an MSRV, can we just remove the two small proptests we have and make them fuzzers (or drop them entirely, its not clear to me they're really worth it)?

Copy link
Contributor Author

@tnull tnull Oct 28, 2025

Choose a reason for hiding this comment

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

No, as I generally find it a good tool to have on our belt. FWIW, many unit/integration tests would benefit from some extended coverage based on proptest.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Oct 28, 2025

Choose a reason for hiding this comment

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

I mean its really just a glorified fuzz test, and one that's slower/generates less coverage. What's the motivation for that?

Copy link
Contributor Author

@tnull tnull Oct 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

It's very easy to convert a simple unit/functional test to a proptest, i.e., very easy to add additional coverage. Meanwhile many contributors wouldn't dare attempt to add fuzz coverage if not explicitly prompted.

The process is entirely different. It's usually just writing a test, and then at the end deciding "hey this could benefit from some randomness to increase coverage". And instead of pulling out some rand::thread_rng, you can just use proptest and get shrinking for free.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Oct 28, 2025

Choose a reason for hiding this comment

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

I'm pretty skeptical that contributors are all that familiar with proptest any more or less than fuzzing, and our fuzzing framework isn't all that crazy to use? IMO way simpler cause you don't have to go digging into some proptest-specific language for specifying inputs. Looking through our current proptests, I'm not really sure much would be lost with dropping them entirely (they're mostly just re-implementing a simple fn and checking that the prod method is equivalent), but if we want the coverage, I'm not actually convinced a non-coverage-guided fuzzer is gonna get very much coverage. A proptest, afaiu, will shove in MAX/MIN/0/-1/etc but not actually look for other paths, and there's enough in calculate_amount_to_forward_per_htlc that I dunno that it even finds its way into everything.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Oct 28, 2025

Choose a reason for hiding this comment

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

For example, this diff currently passes proptest for me, indicating proptest is actually giving us a false sense of security, arguably worse than not having it

$ git diff
diff --git a/lightning-liquidity/src/lsps2/service.rs b/lightning-liquidity/src/lsps2/service.rs
index c715d40502..215a2d7a15 100644
--- a/lightning-liquidity/src/lsps2/service.rs
+++ b/lightning-liquidity/src/lsps2/service.rs
@@ -2094,8 +2094,8 @@ fn calculate_amount_to_forward_per_htlc(
                        / total_expected_outbound_msat as u128) as u64;

                let mut actual_fee_amt_msat = core::cmp::min(fee_remaining_msat, proportional_fee_amt_msat);
-               actual_fee_amt_msat =
-                       core::cmp::min(actual_fee_amt_msat, htlc.expected_outbound_amount_msat);
+               /*actual_fee_amt_msat =
+                       core::cmp::min(actual_fee_amt_msat, htlc.expected_outbound_amount_msat);*/
                fee_remaining_msat -= actual_fee_amt_msat;

                if index == htlcs.len() - 1 {

Copy link
Contributor Author

@tnull tnull Oct 29, 2025

Choose a reason for hiding this comment

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

I'm pretty skeptical that contributors are all that familiar with proptest any more or less than fuzzing, and our fuzzing framework isn't all that crazy to use?

I'm pretty certain about that. Quite a few new contributors were okay writing proptests that wouldn't touch LDK's custom fuzz setup with a ten foot pole.

IMO way simpler cause you don't have to go digging into some proptest-specific language for specifying inputs. Looking through our current proptests, I'm not really sure much would be lost with dropping them entirely (they're mostly just re-implementing a simple fn and checking that the prod method is equivalent), but if we want the coverage, I'm not actually convinced a non-coverage-guided fuzzer is gonna get very much coverage.

Well, we basically just started adding some proptests, IMO we should get into the habit of utilizing it more. Note that you can also easily write proptests for unit tests that wouldn't work for the fuzzer (which essentially does blackbox testing without (easy) access to internals), and of course proptests can make use of our internal test utilities more easily.

Copy link
Collaborator

@TheBlueMatt TheBlueMatt Oct 29, 2025

Choose a reason for hiding this comment

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

Given the above experience, I'm quite skeptical that proptests aren't just going to give us a false sense of security. I was wrong above, proptest doesn't even bother passing 0/MAX/MIN/-1, it only pulls random input and passes that in. Looking at the three proptests we have currently, I think proptest is the wrong tool for all of them:

  • the test of calculate_amount_to_forward_per_htlc clearly needs either fuzzing or specific input testing, as it doesn't hit much of the logic,
  • compute_opening_fee clearly needs specific input testing as the proptest is highly unlikely to check the boundary case, which is maybe the most interesting one (and given the simplicity of the logic under test I'm not convinced a prop/fuzz test is worth it),
  • the LSPSUrl parsing test doesn't really generate any interesting test-cases that aren't pretty thoroughly covered with the specific-input testing (and goes to a lot of regex-effort to do so).

I'm open to being convinced by some future test case where (a) random-input non-coverage-guided fuzzing is sufficient on some method and (b) its not trivially possible to test all the boundary conditions by just writing them out, but I'm honestly not sure that such cases are gonna be a thing almost ever.

Copy link
Contributor Author

@tnull tnull Oct 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

IMO, the discussion on whether or not we want to continue with proptest is out-of-scope for this PR. Can we just land this to finally fix CI, and then we can continue the discussion elsewhere? Esp. since even if we decided against proptest, we surely won't convert the test cases to fuzz in this PR.


export RUST_BACKTRACE=1

echo -e "\n\nChecking the workspace, except lightning-transaction-sync."
Expand Down
3 changes: 0 additions & 3 deletions ci/ci-tx-sync-tests.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,6 @@ function PIN_RELEASE_DEPS {

PIN_RELEASE_DEPS # pin the release dependencies

# Starting with version 0.5.11, the `home` crate has an MSRV of rustc 1.81.0.
[ "$RUSTC_MINOR_VERSION" -lt 81 ] && cargo update -p home --precise "0.5.9" --verbose

# Starting with version 1.2.0, the `idna_adapter` crate has an MSRV of rustc 1.81.0.
[ "$RUSTC_MINOR_VERSION" -lt 81 ] && cargo update -p idna_adapter --precise "1.1.0" --verbose

Expand Down
4 changes: 2 additions & 2 deletions lightning-transaction-sync/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,8 +44,8 @@ lightning = { version = "0.2.0", path = "../lightning", default-features = false
tokio = { version = "1.35.0", features = ["macros"] }

[target.'cfg(not(target_os = "windows"))'.dev-dependencies]
electrsd = { version = "0.35.0", default-features = false, features = ["legacy"] }
corepc-node = { version = "0.8.0", default-features = false, features = ["28_0"] }
electrsd = { version = "0.36.0", default-features = false, features = ["legacy"] }
corepc-node = { version = "0.10.0", default-features = false, features = ["28_0"] }

[lints.rust.unexpected_cfgs]
level = "forbid"
Expand Down
Loading