Reorganize internal codecs (rework-closing-commit-published) #1731
Conversation
This file contains 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
Re-work the `CommitPublished` types to work better with anchor outputs. We previously stored the txs spending utxos that we could claim: this doesn't make sense anymore if these txs may be RBF-ed, because the final tx will be different from the initial one. We instead track what `OutPoint`s we can claim, and the information necessary to claim them. This way we can in the future let a different actor finalize the txs that spend these outpoints (set the fees and sign). We also add information on mutual close txs to immediately identify our output and its amount: this makes auditing how much sats we'll get back very easy from the API when we have many channels to watch. This commit contains a DB migration of the channel data types, but in a backwards-compatible way: we can still read from old data. The only scenario impacted is channels that started force-closing before the migration. They need special care to handle the fact that they had less data than migrated channels, which is why we keep some legacy code around.
Add tests for the changes in the business logic. Add a few tests in places that were previously lacking a bit.
This allows us to strictly segregate each codec version (to make sure that we don't accidentally mix codec versions), while still letting tests access each unitary codecs (which using inner `private classes` would have prevented). Relevant tests only need to be moved to the appropriate package. ``` wire | `-- ChannelCodecs | `-- legacy | | | `-- legacy0 | | | | | `-- LegacyChannelCodecs0 | | | `-- legacy1 | | | | | `-- LegacyChannelCodecs1 | | | `-- ChannelTypes | `-- others ```
This saves us from repeating `private[package]` for each individual codec and is more foolproof (we only need to make sur that the codec is inside the inner class).
This more rigorous organization showed that we were using some codecs in an unrelated wallet database used with electrum. We should probably introduce a `InternalCommonCodecs` class that is more or less immutable (elementary codecs not impacted by versions changes, such as `txCodec` or `bool8`). ``` wire | `-- internal | | | `-- channel | | | | | `-- legacy | | | | | | | `-- legacy0 | | | | | | | | | `-- LegacyChannelCodecs0 | | | | | | | `-- legacy1 | | | | | | | | | `-- LegacyChannelCodecs1 | | | | | | | `-- ChannelTypes | | | | | `-- ChannelCodecs | | | `-- CommandCodecs | `-- others ```
This means that when adding a new version of codecs, we will add a new file instead of overwriting previous files. It's easier to keep track of modifications. Also, the `stateDataCodec` is now more clearly separated. ``` wire | `-- internal | | | `-- channel | | | | | `-- legacy | | | | | | | `-- version0 | | | | | | | | | `-- ChannelCodecs0 | | | | | | | `-- version1 | | | | | | | | | `-- ChannelCodecs1 | | | | | | | `-- ChannelTypes | | | | | `-- version2 | | | | | | | `-- ChannelCodecs2 | | | | | `-- ChannelCodecs | | | `-- CommandCodecs | `-- others ```
All versions are now at the same level, which means adding a new version is really just adding a new package. This may remove a lot of headaches for further migration. The downside is that we can't restrict the visibility of `MigrationUtils` (formerly `ChannelTypes`) to versions 0 and 1, but we would have had the same issue as soon as we introduce version 3 anyway (because the current version 2 codecs would have been moved to the legacy package). ``` wire | `-- internal | | | `-- channel | | | | | `-- MigrationTools | | | | | `-- version0 | | | | | | | `-- ChannelCodecs0 | | | | | `-- version1 | | | | | | | `-- ChannelCodecs1 | | | | | `-- version2 | | | | | | | `-- ChannelCodecs2 | | | | | `-- ChannelCodecs | | | `-- CommandCodecs | `-- others ```
pm47
force-pushed
the
rework-closing-commit-published-pm
branch
from
d5991fa
to
41bf1fa
Compare
pm47
changed the title
|
Superseded by #1732. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Builds on #1728.
The goal is to separate internal codecs from protocol codecs, and to reduce risk during migrations.
Commits should be read one by one.