channeldb: Fix migration 20 incorrect outpoint serialization #203
Conversation
The version of this migration ported from upstream lnd lacked the Tree fields for the read and write outpoint functions, which made the data written by the migration incompatible to the standard channeldb. This adds comments to indicate such.
This function is incompatible to the standard channeldb writeOutpoint in 2 ways: it does not include decred's Tree field and it uses WriteVarBytes instead of a fixed-sized hash. Therefore, to prevent this being erroneously used anywhere in the code, we make it private.
This adds comments on {read,write}Outpoint function definitions to note
the versions where the Tree field is included or not.
This field is only present in decred, therefore when porting code from
upstream lnd, care must be taken that an appropriate implementation
with/without this field is used.
matheusd
force-pushed
the
fix-migration-20
branch
from
e0cc6bd to
9c47db7
Compare
This migration fixes the prior migration 20, introduced when porting the upstream code. That migration erroneously encoded outpoints without the Tree field that exists in decred code, thus rendering the index incorrect. The new migration corrects the issue by assuming the tree of every entry is zero, which is true because the channels can only reside in regular (as opposed to stake) transactions.
matheusd
force-pushed
the
fix-migration-20
branch
from
9c47db7 to
0187a43
Compare
|
Manually tested by running a v0.4.0 simnet node connect to v0.5.0 nodes, create channels, go offline and have those channels closed (SPV mode). After this runs, eventually the channels are detected as closed and other running channels can be run without issues. Unfortunately, since this requires a migration, the current integration tests don't catch this sorts of errors (channel created on one version being closed on another). I'll investigate the possibility of creating a new set of integration tests that exercise these types of scenarios, but for this particular issue I'll expedite merging and releasing a new version with the fix (v0.5.1) in order avoid having users with broken channels. Bison Relay devs will also want to release a new version soon(ish). To reiterate: funds are safe and any channels that get closed in the mean time are eventually fixed, but it's not great to have these channels hanging around in the wallet, compromising pathfinding and breaking channel listings. |
This migration fixes the prior migration 20, introduced when porting the upstream code.
That migration erroneously encoded outpoints without the Tree field that exists in decred code, thus rendering the index incorrect.
The new migration corrects the issue by assuming the tree of every entry is zero, which is true because the channels can only reside in regular (as opposed to stake) transactions.
One way this could cause problems was when closing or detecting closed channels that were created before the migration took place. Closing them could fail with "missing outpoint from index".
While this fixes the issue for nodes that upgrade, this does not handle channels that were already closed for nodes that are already running v0.5.0.