Finish ChannelMonitor case handling and add more testing

[TheBlueMatt]

There's one or two TODOs in ChannelMonitor to handle all the listed cases, plus some TODOs in channel-force-close in ChannelManager to update ChannelMonitor, and the test in ChannelManager which construct networks, do various things to them, and then check monitor validity should be fleshed out way way more.

[ariard]

Was looking on it, seems to me than for some chain_monitor TODO we need to pass an Arc to ChannelMonitor struct and if so, maybe it's make sense to drop it from SimpleManyChannelMonitor?

[TheBlueMatt]

I assume you're referring to "TODO: Need to register the given script here with a chain_monitor" and the two "TODO: Register all outputs in commitment_tx with the ChainWatchInterface!"? I'm not 100% sure by what you mean by "drop it from SimpleManyChannelMonitor", though? In any case, yea, not sure if it makes more sense to return such values to the callers (ala the Channel::block_connected telling ChannelManager::block_connected how it should handle it) or if it makes more sense to give it yet another Arc.

[ariard]

Yes I'm referring to this ones, I prefer your suggestion with Channel - ChannelManager because I started on the other side and passing ChainWatchInterface means we have to deal with it in channel_monitor serialization/deserialization. Will open a PR soon on this

[ariard]

Following BOLT 5 there is a lot of different cases to claim in case of remote node broadcasting a commitment tx.
The Bad Way :

• to_remote outputs via P2WPKH
• offered htlc outputs via payment preimage tx
• received htlc outputs via timeout tx

The Ugly Way :

• to_remote outputs via P2WPKH (same as above)
• to_local outputs via revocation key (tested in Fix simple to_local revoked output claim and rebase #184 #199 )
• offered/received htlc outputs via revocation key (tested in Add channel_monitor_claim_revoked_commitment_tx test #184 )
• HTLC-timeout/HTLC-success outputs via revocation key (with Implement claiming of revoked HTLC transactions by ChannelMonitor #163)

Seems to me that currently, channel_monitor_network_test currently does only Bad Way 2,3 and Ugly Way 4, do I forget any case ?

[ariard]

(Alice broadcasting a commitment tx, speaking as Bob viewpoint here)

[TheBlueMatt]

"to_remote outputs via P2WPKH" should be covered by #81 (ie once we have a way for clients to specify the keys they want they should be monitoring the chain for at least the non-derived keys, and obviously documentation should mention this, this also applies to shutdown keys).

The "via revocation key" HTLC outputs are implemented, probably, but aren't tested yet (which you indicated, just being thourough).

The to_local outputs via revocation key I think we impelement? Maybe I'm misunderstanding what the BOLT is referring to, but note that I think we need an interface so that we can tell the client about the availability of an output+the key needed to claim it when we believe the remote side cannot take it (eg its a timelocked output for our latest commitment transaction which we don't think we've ever revoked...we are in no rush to spend it and can let the client do so at their leisure).

[ariard]

Yes sorry I checked the code in fact the to_local outputs via revocation is implemented, but seems to me there is not test only for it because in some tests (all?) it's trimmed so it doesn't appear specifically.

#184 is tests for "via revocation key" HTLC outputs, need review. While I was writing tests, I was wandering maybe we could cache tx given to check_spend_remote_tx after 1st pass in case of block re-scanning to avoid generate again same punishment tx.

"to_remote_outputs via P2WPKH", seems fine to be covered by #81, noted.

[ariard]

And yeah for stuff in force_close about ChannelManager I think we need an interface to have the ChainWatchInterface to pass up the payment_preimage extracted from on-chain HTLC-Success input to the ChannelManager and this last one claiming backwards. Seems for HTLC-timeout case, failing backward. Working on it

[TheBlueMatt]

I'd hope it doesn't need to get added in ChainWatchInterface, maybe ChannelMonitors should have a different block_connected fn that ChannelManager calls that returns more information and watchtowers (or other calls into ChannelMonitor via ChainWatchInterface) can just ignore the extra data.
The which-object-is-registered-with-ChainWatchInterface stuff is already a mess (see #80) so I don't think rejiggering what calls what is that big a deal right now. Eventually we'll need to rewrite it all and make it harder to misuse but that doesn't need to happen ASAP.

[ariard]

Hmm, just a side-note to not forget to add the case of a revoked commitment tx solved by a justice tx, detect it and fail backward. Will go in #198

[ariard]

Some notes to check if we have covered everything on on-chain Channel resolution.

Local Node seeing a Local Commitment Tx :

• commitment tx :

  • to_local output, resolution by output descriptor generated, implemented by ChannelMonitor::broadcast_by_local_state, tested by test_claim_sizeable_push_msat
  • to_remote output, nothing to be done
  • offered_htlc_output : resolution by HTLC-Timeout tx generated and failure passed backward if needed, implemented by ChannelMonitor::broadcast_by_local_state, tested by channel_monitor_network_test, test_htlc_on_chain_timeout
  • received_htlc_output : resolution by HTLC-Success tx generated, implemented by ChannelMonitor::broadcast_by_local_state, tested by channel_monitor_network_test

• HTLC-Timeout tx (from any commitment tx offered_htlc_output) :

  • unique output : resolution by output descriptor generated for unique output, implemented by ChannelMonitor::broadcast_by_local_state, tested by test_dynamic_spendable_outputs_local_htlc_timeout_tx

• HTLC-Success tx (from any commitment tx received_htlc_output) :

  • unique output : resolution by output descriptor generated for unique output, implemented by ChannelMonitor::broadcast_by_local_state, tested by test_dynamic_spendable_outputs_local_htlc_success_tx

Local Node seeing a Non-Revoked Remote Commitment Tx :

[WIP]

[ariard]

Killed by #336 IMO, not sure if bumping engine is in the scope.

[TheBlueMatt]

Nah, I think its fine as-is. I'd like to get better test coverage on it, but this bug itself is probably done.