Fix backwards compatibility of NodeMetrics reads
#748
Merged
+62
−5
Conversation
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
In commit dd55d47 we started ignoring the legacy `latest_channel_monitor_archival_height` field of `NodeMetrics`. However, we erroneously started reading it as `Option<u32>`, though, given it's an optional field, it should have been read as a plain `u32` that might or might not be present. Here we fix this error.
|
👋 Thanks for assigning @TheBlueMatt as a reviewer! |
We previously added a test asserting backwards compatibility for nodes reinitializing from a VSS backend. However, given VSS tests are only continously run in CI we here add the same test using the default SQLite backend, ensuring backwards compatibility breakage is also checked when running tests locally.
tnull
force-pushed
the
2026-01-fix-backwards-compat
branch
from
e07e9f5 to
93b6b42
Compare
Collaborator
Author
Sorry, force-pushed some minor fixups dropping VSS-specific comments from the testcase: > git diff-tree -U2 e07e9f5c 93b6b425
diff --git a/tests/integration_tests_rust.rs b/tests/integration_tests_rust.rs
index 2855c425..4b82d1f4 100644
--- a/tests/integration_tests_rust.rs
+++ b/tests/integration_tests_rust.rs
@@ -41,5 +41,4 @@ use lightning_invoice::{Bolt11InvoiceDescription, Description};
use lightning_types::payment::{PaymentHash, PaymentPreimage};
use log::LevelFilter;
-use rand::{rng, Rng};
#[tokio::test(flavor = "multi_thread", worker_threads = 1)]
@@ -2449,5 +2448,5 @@ async fn persistence_backwards_compatibility() {
let node_entropy = NodeEntropy::from_seed_bytes(seed_bytes);
- // Setup a v0.6.2 `Node` persisted with the v0 scheme.
+ // Setup a v0.6.2 `Node`
let (old_balance, old_node_id) = {
let mut builder_old = ldk_node_062::Builder::new();
@@ -2473,9 +2472,5 @@ async fn persistence_backwards_compatibility() {
let node_id = node_old.node_id();
- // Workaround necessary as v0.6.2's VSS runtime wasn't dropsafe in a tokio context.
- tokio::task::block_in_place(move || {
- node_old.stop().unwrap();
- drop(node_old);
- });
+ node_old.stop().unwrap();
(balance, node_id) |
TheBlueMatt
approved these changes
Jan 9, 2026
Contributor
|
CI is still sad though. |
Collaborator
Author
Yeah, that seems really unrelated this time: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
In commit dd55d47 we started ignoring the legacy
latest_channel_monitor_archival_heightfield ofNodeMetrics. However, we erroneously started reading it asOption<u32>, though, given it's an optional field, it should have been read as a plainu32that might or might not be present. Here we fix this error.We also add a test case asserting serialization backwards compatibility for non-VSS backends.