Uh oh!
There was an error while loading. Please reload this page.
Add a simple test of upgrading from LDK 0.1 and correct in_flight_monitor_updates on upgrade - #3584
Conversation
708a5cc to
a237be2CompareThere was a problem hiding this comment.
Would it make sense to have this live in a dedicated tests crate or in the integration tests (tests) folder? This would also allow us to keep our testing script untouched, IIUC?
There was a problem hiding this comment.
Hmmmm, yea, we definitely could. I'm kinda on the fence. On the one hand, you're right, we'd be able to do cargo -p lightning which is really nice, on the other hand we wouldn't run the upgrade tests with cargo test, which kinda sucks (and means yet more cases of things passing trivially but failing in CI). Do you have a strong opinion here?
There was a problem hiding this comment.
we wouldn't run the upgrade tests with
cargo test, which kinda sucks (and means yet more cases of things passing trivially but failing in CI).
Huh, but integration tests are run via cargo test, just after normal unit tests are run?
Do you have a strong opinion here?
Yeah, kinda tbh, I'd prefer to not interweave yet another layer of complexity into CI & dependency scripts. I agree we should see that tests are always run in CI and easy to run locally, but would prefer to have them live separately from the code and unit tests.
Note we could either have integration tests live as lightning/tests, or, especially if we think that we'd want to add more cross-crate tests there over time, could add them as a new member crate to the workspace at /lightning-tests. While the former is a bit more focused on lightning, the latter would open the door for moving some of the tests in lightning-background-processor there, which currently acts as the defacto place for integration tests as it's the only place in the dependency tree that allows for accessing all the necessary objects.
There was a problem hiding this comment.
Huh, but integration tests are run via cargo test, just after normal unit tests are run?
Note we could either have integration tests live as lightning/tests
I don't see a way to add a separate dependency to the normal rust integration test files. We'd have to do it as a proper crate with a proper Cargo.toml, which I assume wouldn't get run like integration tests would.
There was a problem hiding this comment.
Hmm, that's true. Then IMO it might be preferable to just add a lightning-tests crate to the workspace? Yes, it wouldn't get run by cargo test, but by the ci-tests.sh, which we already need to run anyways to cover all feature variants, etc?
There was a problem hiding this comment.
We chatted a bit more offline. To me the question is whether most devs run cargo test as their usual workflow or run ci-tests.sh. Indeed, devs need to run ci-tests.sh to hit "everything" but most things get hit with cargo test normally. With upgrade/downgrade tests, I fear that they're going to be as likely to fail as most other tests, if not more, so making devs (especially new devs who tend to hit this often) wait for a CI cycle before they find the bug is gonna be pretty annoying.
the latter would open the door for moving some of the tests in lightning-background-processor there, which currently acts as the defacto place for integration tests as it's the only place in the dependency tree that allows for accessing all the necessary objects.
That's a fair point, and I did go ahead and create a new crate for this reason. For now I left it in the workspace until we resolve the above question.
valentinewallace
commented
Feb 10, 2025
LGTM after @tnull's feedback is resolved (and it looks like this need rebase). Nice to have this testing infra! |
a237be2 to
44e1a54CompareRebased. I pinged @tnull offline but dunno when/if he'll respond, there's also no rush. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@## main #3584 +/- ##
==========================================
+ Coverage 89.22% 90.99% +1.77%
==========================================
Files 155 157 +2 Lines 118973 134041 +15068 Branches 118973 134041 +15068 ==========================================
+ Hits 106154 121975 +15821 + Misses 10239 9675 -564 + Partials 2580 2391 -189 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
44e1a54 to
b099963Comparewpaulino
commented
Mar 5, 2025
CI is still sad |
wpaulino
commented
Mar 10, 2025
|
8c96d90 to
f9fb216Comparewpaulino
commented
Mar 11, 2025
|
In e8854f9 we changed the type of `ChannelManager::in_flight_monitor_updates`, writing a legacy version and reading the new version as a new TLV field. Sadly, we spuriously marked the new TLV as `required`, breaking upgrade from 0.1. Here we fix the oversight by simply marking it `option`al.
The code was a bit too long to begin with, and rustfmt made a total mess of it, so instead we should be building with more intermediate variables.
The signer we use in tests tracks the state of the channel and refuses to sign when the channel attempts an invalid state transition. In the next commit, however, we'll add an upgrade test which will fail these checks as the the state won't get copied from previous versions of LDK to this version. Thus, here, we add the ability to disable all state-based checks in the signer.
171401f to
c3f8144CompareTheBlueMatt
commented
May 16, 2025
Ooookayyyyyy. We discussed offline and it seems many folks who work on LDK do use |
| lightning-types = { version = "0.3.0", path = "../lightning-types", features = ["_test_utils"] } | ||
| lightning-invoice = { version = "0.34.0", path = "../lightning-invoice", default-features = false } | ||
| lightning-macros = { version = "0.2", path = "../lightning-macros" } | ||
| lightning = { version = "0.2", path = "../lightning", features = ["_test_utils"] } |
There was a problem hiding this comment.
Should these be using the +git version numbers, which also might help remembering to bump them post-release?
There was a problem hiding this comment.
Actually we don't even need the version tag, removed them.
Uh oh!
There was an error while loading. Please reload this page.
One major hole in our test coverage historically has been tests covering upgrades or downgrades across LDK versions. Luckily, these aren't particularly hard to write as cargo lets us depend on previous versions of the `lightning` crate directly, which we can use in tests. Here we add a simple initial test of upgrading from LDK 0.1 while there's a pending payment to be claimed.
Running `cargo test -p crate` for each crate in the workspace results in re-building the `lightning` crate for each workspace crate, which can be quite slow. This adds nontrivial time to our total CI runs. Here, instead, we just run `cargo test` and let it build the whole workspace crate list in one go. This does result in combined features which may leave some issues with `cargo test -p crate` undetected, but there's not a ton of harm to that.
c3f8144 to
86c661aCompareTheBlueMatt
commented
May 21, 2025
Squash pushed a quick $ git diff-tree -U1 c3f8144222 86c661a5e6
diff --git a/lightning-tests/Cargo.toml b/lightning-tests/Cargo.toml
index 01ad635580..75c68e03f5 100644
--- a/lightning-tests/Cargo.toml+++ b/lightning-tests/Cargo.toml@@ -12,6 +12,6 @@ edition = "2021"
[dependencies]
-lightning-types = { version = "0.3.0", path = "../lightning-types", features = ["_test_utils"] }-lightning-invoice = { version = "0.34.0", path = "../lightning-invoice", default-features = false }-lightning-macros = { version = "0.2", path = "../lightning-macros" }-lightning = { version = "0.2", path = "../lightning", features = ["_test_utils"] }+lightning-types = { path = "../lightning-types", features = ["_test_utils"] }+lightning-invoice = { path = "../lightning-invoice", default-features = false }+lightning-macros = { path = "../lightning-macros" }+lightning = { path = "../lightning", features = ["_test_utils"] }
lightning_0_1 = { package = "lightning", version = "0.1.1", features = ["_test_utils"] } |
TheBlueMatt
commented
May 21, 2025
@tnull already said "LGTM" and the one remaining question was addressed, landing since i want to base more PRs on this. |
TheBlueMatt
commented
May 23, 2025
Partially backported in #3794 |
One major hole in our test coverage historically has been tests
covering upgrades or downgrades across LDK versions. Luckily, these
aren't particularly hard to write as cargo lets us depend on
previous versions of the
lightningcrate directly, which we canuse in tests.
Here we add a simple initial test of upgrading from LDK 0.1 while
there's a pending payment to be claimed.