Fix Two Bugs around ChannelManager serialization round-trip by TheBlueMatt · Pull Request #503 · lightningdevkit/rust-lightning

TheBlueMatt · 2020-02-17T19:58:54Z

Based on #455 since one of the new tests builds on the tests added there. The pre-block-connected bug I found while playing with rust-lightning-bitcoinrpc, the other one I found while trying to write a test for the first.

ariard

Good catch! More comments but otherwise looks good

TheBlueMatt · 2020-02-17T22:53:57Z

Added comments everywhere requested.

ariard · 2020-02-18T05:45:41Z

ACK bc90c80

Upon deserialization/reload we need to be able to register each outpoint which spends the commitment txo which a channelmonitor believes to be on chain. While our other internal tracking is likely sufficient to regenerate these, its much easier to simply track all outpouts we've ever generated, so we do that here.

This tests, after each functional test, that if we serialize and reload all of our ChannelMonitors we end up tracking the same set of outputs as before.

Previously, when attempting to write out a channel with some RemoteAnnounced pending inbound HTLCs, we'd write out the count without them, but write out some of their fields. We should drop them as intended as they will need to be reannounced upon reconnection. This was found while attempting to simply reproduce a different bug by adding tests for ChannelManager serialization rount-trip at the end of each functional_test (in Node::drop). That test is included here to prevent some classes of similar bugs in the future.

Previously, if we have a live ChannelManager (that has seen blocks) and we open a new Channel, if we serialize that ChannelManager before a new block comes in, we'll fail to deserialize it. This is the result of an overly-ambigious last_block_connected check which would see 0s for the new channel but the previous block for the ChannelManager as a whole. We add a new test which catches this error as well as hopefully getting some test coverage for other similar issues in the future.

TheBlueMatt · 2020-02-17T19:58:54Z

Based on #455 since one of the new tests builds on the tests added there. The pre-block-connected bug I found while playing with rust-lightning-bitcoinrpc, the other one I found while trying to write a test for the first.

ariard

Good catch! More comments but otherwise looks good

TheBlueMatt · 2020-02-17T22:53:57Z

Added comments everywhere requested.

ariard · 2020-02-18T05:45:41Z

ACK bc90c80

Upon deserialization/reload we need to be able to register each outpoint which spends the commitment txo which a channelmonitor believes to be on chain. While our other internal tracking is likely sufficient to regenerate these, its much easier to simply track all outpouts we've ever generated, so we do that here.

This tests, after each functional test, that if we serialize and reload all of our ChannelMonitors we end up tracking the same set of outputs as before.

Previously, when attempting to write out a channel with some RemoteAnnounced pending inbound HTLCs, we'd write out the count without them, but write out some of their fields. We should drop them as intended as they will need to be reannounced upon reconnection. This was found while attempting to simply reproduce a different bug by adding tests for ChannelManager serialization rount-trip at the end of each functional_test (in Node::drop). That test is included here to prevent some classes of similar bugs in the future.

Previously, if we have a live ChannelManager (that has seen blocks) and we open a new Channel, if we serialize that ChannelManager before a new block comes in, we'll fail to deserialize it. This is the result of an overly-ambigious last_block_connected check which would see 0s for the new channel but the previous block for the ChannelManager as a whole. We add a new test which catches this error as well as hopefully getting some test coverage for other similar issues in the future.

TheBlueMatt · 2020-02-17T19:58:54Z

Based on #455 since one of the new tests builds on the tests added there. The pre-block-connected bug I found while playing with rust-lightning-bitcoinrpc, the other one I found while trying to write a test for the first.

ariard

Good catch! More comments but otherwise looks good

TheBlueMatt · 2020-02-17T22:53:57Z

Added comments everywhere requested.

ariard · 2020-02-18T05:45:41Z

ACK bc90c80

Upon deserialization/reload we need to be able to register each outpoint which spends the commitment txo which a channelmonitor believes to be on chain. While our other internal tracking is likely sufficient to regenerate these, its much easier to simply track all outpouts we've ever generated, so we do that here.

This tests, after each functional test, that if we serialize and reload all of our ChannelMonitors we end up tracking the same set of outputs as before.

Previously, when attempting to write out a channel with some RemoteAnnounced pending inbound HTLCs, we'd write out the count without them, but write out some of their fields. We should drop them as intended as they will need to be reannounced upon reconnection. This was found while attempting to simply reproduce a different bug by adding tests for ChannelManager serialization rount-trip at the end of each functional_test (in Node::drop). That test is included here to prevent some classes of similar bugs in the future.

Previously, if we have a live ChannelManager (that has seen blocks) and we open a new Channel, if we serialize that ChannelManager before a new block comes in, we'll fail to deserialize it. This is the result of an overly-ambigious last_block_connected check which would see 0s for the new channel but the previous block for the ChannelManager as a whole. We add a new test which catches this error as well as hopefully getting some test coverage for other similar issues in the future.

TheBlueMatt · 2020-02-17T19:58:54Z

Based on #455 since one of the new tests builds on the tests added there. The pre-block-connected bug I found while playing with rust-lightning-bitcoinrpc, the other one I found while trying to write a test for the first.

ariard

Good catch! More comments but otherwise looks good

TheBlueMatt · 2020-02-17T22:53:57Z

Added comments everywhere requested.

ariard · 2020-02-18T05:45:41Z

ACK bc90c80

Upon deserialization/reload we need to be able to register each outpoint which spends the commitment txo which a channelmonitor believes to be on chain. While our other internal tracking is likely sufficient to regenerate these, its much easier to simply track all outpouts we've ever generated, so we do that here.

This tests, after each functional test, that if we serialize and reload all of our ChannelMonitors we end up tracking the same set of outputs as before.

Previously, when attempting to write out a channel with some RemoteAnnounced pending inbound HTLCs, we'd write out the count without them, but write out some of their fields. We should drop them as intended as they will need to be reannounced upon reconnection. This was found while attempting to simply reproduce a different bug by adding tests for ChannelManager serialization rount-trip at the end of each functional_test (in Node::drop). That test is included here to prevent some classes of similar bugs in the future.

Previously, if we have a live ChannelManager (that has seen blocks) and we open a new Channel, if we serialize that ChannelManager before a new block comes in, we'll fail to deserialize it. This is the result of an overly-ambigious last_block_connected check which would see 0s for the new channel but the previous block for the ChannelManager as a whole. We add a new test which catches this error as well as hopefully getting some test coverage for other similar issues in the future.

TheBlueMatt · 2020-02-17T19:58:54Z

Based on #455 since one of the new tests builds on the tests added there. The pre-block-connected bug I found while playing with rust-lightning-bitcoinrpc, the other one I found while trying to write a test for the first.

ariard

Good catch! More comments but otherwise looks good

TheBlueMatt · 2020-02-17T22:53:57Z

Added comments everywhere requested.

ariard · 2020-02-18T05:45:41Z

ACK bc90c80

Upon deserialization/reload we need to be able to register each outpoint which spends the commitment txo which a channelmonitor believes to be on chain. While our other internal tracking is likely sufficient to regenerate these, its much easier to simply track all outpouts we've ever generated, so we do that here.

This tests, after each functional test, that if we serialize and reload all of our ChannelMonitors we end up tracking the same set of outputs as before.

Previously, when attempting to write out a channel with some RemoteAnnounced pending inbound HTLCs, we'd write out the count without them, but write out some of their fields. We should drop them as intended as they will need to be reannounced upon reconnection. This was found while attempting to simply reproduce a different bug by adding tests for ChannelManager serialization rount-trip at the end of each functional_test (in Node::drop). That test is included here to prevent some classes of similar bugs in the future.

Previously, if we have a live ChannelManager (that has seen blocks) and we open a new Channel, if we serialize that ChannelManager before a new block comes in, we'll fail to deserialize it. This is the result of an overly-ambigious last_block_connected check which would see 0s for the new channel but the previous block for the ChannelManager as a whole. We add a new test which catches this error as well as hopefully getting some test coverage for other similar issues in the future.

TheBlueMatt · 2020-02-17T19:58:54Z

Based on #455 since one of the new tests builds on the tests added there. The pre-block-connected bug I found while playing with rust-lightning-bitcoinrpc, the other one I found while trying to write a test for the first.

ariard

Good catch! More comments but otherwise looks good

TheBlueMatt · 2020-02-17T22:53:57Z

Added comments everywhere requested.

ariard · 2020-02-18T05:45:41Z

ACK bc90c80

Upon deserialization/reload we need to be able to register each outpoint which spends the commitment txo which a channelmonitor believes to be on chain. While our other internal tracking is likely sufficient to regenerate these, its much easier to simply track all outpouts we've ever generated, so we do that here.

This tests, after each functional test, that if we serialize and reload all of our ChannelMonitors we end up tracking the same set of outputs as before.

Previously, when attempting to write out a channel with some RemoteAnnounced pending inbound HTLCs, we'd write out the count without them, but write out some of their fields. We should drop them as intended as they will need to be reannounced upon reconnection. This was found while attempting to simply reproduce a different bug by adding tests for ChannelManager serialization rount-trip at the end of each functional_test (in Node::drop). That test is included here to prevent some classes of similar bugs in the future.

Previously, if we have a live ChannelManager (that has seen blocks) and we open a new Channel, if we serialize that ChannelManager before a new block comes in, we'll fail to deserialize it. This is the result of an overly-ambigious last_block_connected check which would see 0s for the new channel but the previous block for the ChannelManager as a whole. We add a new test which catches this error as well as hopefully getting some test coverage for other similar issues in the future.

TheBlueMatt · 2020-02-17T19:58:54Z

Based on #455 since one of the new tests builds on the tests added there. The pre-block-connected bug I found while playing with rust-lightning-bitcoinrpc, the other one I found while trying to write a test for the first.

ariard

Good catch! More comments but otherwise looks good

TheBlueMatt · 2020-02-17T22:53:57Z

Added comments everywhere requested.

ariard · 2020-02-18T05:45:41Z

ACK bc90c80

Upon deserialization/reload we need to be able to register each outpoint which spends the commitment txo which a channelmonitor believes to be on chain. While our other internal tracking is likely sufficient to regenerate these, its much easier to simply track all outpouts we've ever generated, so we do that here.

This tests, after each functional test, that if we serialize and reload all of our ChannelMonitors we end up tracking the same set of outputs as before.

Previously, when attempting to write out a channel with some RemoteAnnounced pending inbound HTLCs, we'd write out the count without them, but write out some of their fields. We should drop them as intended as they will need to be reannounced upon reconnection. This was found while attempting to simply reproduce a different bug by adding tests for ChannelManager serialization rount-trip at the end of each functional_test (in Node::drop). That test is included here to prevent some classes of similar bugs in the future.

Previously, if we have a live ChannelManager (that has seen blocks) and we open a new Channel, if we serialize that ChannelManager before a new block comes in, we'll fail to deserialize it. This is the result of an overly-ambigious last_block_connected check which would see 0s for the new channel but the previous block for the ChannelManager as a whole. We add a new test which catches this error as well as hopefully getting some test coverage for other similar issues in the future.

TheBlueMatt · 2020-02-17T19:58:54Z

Based on #455 since one of the new tests builds on the tests added there. The pre-block-connected bug I found while playing with rust-lightning-bitcoinrpc, the other one I found while trying to write a test for the first.

ariard

Good catch! More comments but otherwise looks good

TheBlueMatt · 2020-02-17T22:53:57Z

Added comments everywhere requested.

ariard · 2020-02-18T05:45:41Z

ACK bc90c80

Upon deserialization/reload we need to be able to register each outpoint which spends the commitment txo which a channelmonitor believes to be on chain. While our other internal tracking is likely sufficient to regenerate these, its much easier to simply track all outpouts we've ever generated, so we do that here.

This tests, after each functional test, that if we serialize and reload all of our ChannelMonitors we end up tracking the same set of outputs as before.

Previously, when attempting to write out a channel with some RemoteAnnounced pending inbound HTLCs, we'd write out the count without them, but write out some of their fields. We should drop them as intended as they will need to be reannounced upon reconnection. This was found while attempting to simply reproduce a different bug by adding tests for ChannelManager serialization rount-trip at the end of each functional_test (in Node::drop). That test is included here to prevent some classes of similar bugs in the future.

Previously, if we have a live ChannelManager (that has seen blocks) and we open a new Channel, if we serialize that ChannelManager before a new block comes in, we'll fail to deserialize it. This is the result of an overly-ambigious last_block_connected check which would see 0s for the new channel but the previous block for the ChannelManager as a whole. We add a new test which catches this error as well as hopefully getting some test coverage for other similar issues in the future.

ariard approved these changes Feb 17, 2020
View reviewed changes

Comment threadlightning/src/ln/channel.rs
Comment threadlightning/src/ln/channelmanager.rs
Comment threadlightning/src/ln/functional_tests.rs

TheBlueMatt force-pushed the 2020-02-chanmon-ser-roundtrip branch from fd52ba8 to bc90c80Compare February 17, 2020 22:53

TheBlueMatt added this to the 0.0.10 milestone Feb 17, 2020

TheBlueMatt added 5 commits February 18, 2020 18:20

Test that we don't forget to track any outputs at monitor-load
662f13a
This tests, after each functional test, that if we serialize and reload all of our ChannelMonitors we end up tracking the same set of outputs as before.

Add comment describing how ChannelMonitor::block_connected is called
5fceb0f

TheBlueMatt force-pushed the 2020-02-chanmon-ser-roundtrip branch from bc90c80 to 4b189bdCompare February 18, 2020 23:24

TheBlueMatt merged commit 69c3eb2 into lightningdevkit:masterFeb 19, 2020

TheBlueMatt modified the milestones: 0.0.10, 0.0.11 Feb 26, 2020