Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Read and write ChainMonitor using the same key #1665

Merged
merged 5 commits into from
Dec 1, 2023

Conversation

luckysori
Copy link
Contributor

@luckysori luckysori commented Dec 1, 2023

Fixes #1661.
Fixes #1565.

This fixes a kind of bug which could happen like this:

  1. Create a ChainMonitor based on version 1.4.4 (for example). This would use the number "4" as the key.

  2. Upgrade to version 1.6.5 (for example).

  3. On startup, we would identify the original ChainMonitor as backwards-incompatible and attempt to overwrite it. Instead, we would insert it under the new key "chain_monitor".

  4. Then we would build the dlc_manager::Manager, which would load the ChainMonitor internally. This would look for the first
    ChainMonitor out of all the ones in the CHAIN_MONITOR tree, ignoring the key. Because the original one was inserted first, we
    would load the original one which would once again fail to deserialise due to backwards-incompatibility.

Now we ensure that we always use the same key to read and write the ChainMonitor. This makes sense because there should only ever be one ChainMonitor per node.

@luckysori luckysori requested review from bonomat and holzeis December 1, 2023 04:39
@luckysori luckysori self-assigned this Dec 1, 2023
Copy link
Contributor

@holzeis holzeis left a comment

Choose a reason for hiding this comment

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

Thanks for the fix 🚀

crates/ln-dlc-storage/src/lib.rs Show resolved Hide resolved
This fixes a kind of bug which could happen like this:

1. Create a `ChainMonitor` based on version 1.4.4 (for example). This
would use the number "4" as the key.

2. Upgrade to version 1.6.5 (for example).

3. On startup, we would identify the original `ChainMonitor` as
backwards-incompatible and attempt to overwrite it. Instead, we would
insert it under the new key "chain_monitor".

4. Then we would build the `dlc_manager::Manager`, which would load
the `ChainMonitor` internally. This would look for the _first_
`ChainMonitor` out of all the ones in the `CHAIN_MONITOR` tree,
ignoring the key. Because the original one was inserted first, we
would load the original one which would once again fail to deserialise
due to backwards-incompatibility.

Now we ensure that we always use the same key to read and write the
`ChainMonitor`. This makes sense because there should only ever be one
`ChainMonitor` per node.
To check that the restored position can be closed.
@luckysori luckysori force-pushed the fix/chain-monitor-backwards-incompatibility branch from 8c41f0b to e7f3a4f Compare December 1, 2023 10:06
@luckysori
Copy link
Contributor Author

@holzeis: Added a changelog entry!

@luckysori luckysori enabled auto-merge December 1, 2023 10:07
@luckysori luckysori added this pull request to the merge queue Dec 1, 2023
Merged via the queue into main with commit 2fedb1d Dec 1, 2023
9 checks passed
@luckysori luckysori deleted the fix/chain-monitor-backwards-incompatibility branch December 1, 2023 10:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants