Skip to content

Peer storage#723

Merged
thomash-acinq merged 9 commits intomasterfrom
peer-storage-spec
Apr 8, 2025
Merged

Peer storage#723
thomash-acinq merged 9 commits intomasterfrom
peer-storage-spec

Conversation

@thomash-acinq
Copy link
Contributor

Replaces the custom channel data backup with the new peer storage (lightning/bolts#1110)

@thomash-acinq thomash-acinq force-pushed the peer-storage-spec branch 5 times, most recently from b3c4282 to c8fb76b Compare November 5, 2024 14:17
@t-bast t-bast mentioned this pull request Jan 23, 2025
@thomash-acinq thomash-acinq force-pushed the peer-storage-spec branch 3 times, most recently from 049b236 to f67a738 Compare April 1, 2025 14:15
Replaces the custom channel data backup with the new peer storage (lightning/bolts#1110)
@thomash-acinq thomash-acinq marked this pull request as ready for review April 4, 2025 08:01
"right": "03c57839fd412868a398bea05d01678a752661126a2d1357a0f7cb6f0c60311125"
},
"remotePerCommitmentSecrets": "<redacted>",
"remoteChannelData": {
Copy link
Member

Choose a reason for hiding this comment

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

I cannot put a comment on deleted files, but can you explain why you removed some test cases from this nonreg folder? We should always be able to keep decoding the .bin file, otherwise we may be introducing a compatibility issue. Instead of deleting a whole folder, you should always just update the json file, which shows that we can still decode the binary state (even though we're now ignoring the remoteChannelData).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test cases that I removed are using ShutdownTlv.ChannelData which I thought we wanted to remove. I've restored the tests and kept the legacy TLV.

Copy link
Member

Choose a reason for hiding this comment

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

Got it, that was the reason, because we're including a whole Shutdown message in those states. Let's keep it for now, and this will be cleaned up when we get rid of v2 and v3 in #685

@thomash-acinq thomash-acinq requested a review from t-bast April 8, 2025 11:48
Copy link
Member

@t-bast t-bast left a comment

Choose a reason for hiding this comment

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

LGTM!

@thomash-acinq thomash-acinq merged commit fac5406 into master Apr 8, 2025
2 checks passed
@thomash-acinq thomash-acinq deleted the peer-storage-spec branch April 8, 2025 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants