Bump electrsd to 0.36.1, fix CI

[tnull]

We bump our electrsd dependency to the latest version, allowing us to remove the home pin.

We also added a commit pinning proptest again, as the most-recent release had our CI fail.

[ldk-reviews-bot]

👋 Thanks for assigning @TheBlueMatt as a reviewer!
I'll wait for their review and will help manage the review process.
Once they submit their review, I'll check if a second reviewer would be helpful.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 88.79%. Comparing base (fdc8731) to head (9753e6d).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4180      +/-   ##
==========================================
- Coverage   88.82%   88.79%   -0.04%     
==========================================
  Files         180      180              
  Lines      137278   137278              
  Branches   137278   137278              
==========================================
- Hits       121944   121894      -50     
- Misses      12522    12573      +51     
+ Partials     2812     2811       -1     

Flag	Coverage Δ
fuzzing	21.49% <ø> (ø)
tests	88.63% <ø> (-0.04%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TheBlueMatt]

This is gonna conflict with #4175, can we just do that?

[tnull]

Not quite sure I understand what you mean? In #4175 you just move the pin over to be even more general, but we can just drop it now?

[TheBlueMatt]

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)?

[tnull]

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.

[TheBlueMatt]

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

[tnull]

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.

[TheBlueMatt]

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.

[TheBlueMatt]

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 {

[tnull]

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.

[TheBlueMatt]

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.

[ldk-reviews-bot]

👋 The first review has been submitted!

Do you think this PR is ready for a second reviewer? If so, click here to assign a second reviewer.