Persist entire monitor if there is an error while applying monitor_update

[G8XSU]

Commit 1: Correctly mark chain_sync updates in test_utils

We were incorrectly marking updates as chain_sync
or not in test_utils based on whether monitor_update
is None or not. Instead, use UpdateOrigin to determine it.

Commit 2: Persist full monitor if there is an error while applying monitor_update

Motivation: When there is an error while applying monitor_update to a
channel_monitor, we don't want to persist a 'monitor_update' which
results in a failure to apply later while reading 'channel_monitor' with
updates from storage. Instead, we should persist the entire 'channel_monitor'
here, otherwise we might be stuck in unrecoverable state after restart.

// TODO: Write test-cases for this change.

[codecov-commenter]

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (db41b87) 88.96% compared to head (976acd8) 89.00%.
Report is 34 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2621      +/-   ##
==========================================
+ Coverage   88.96%   89.00%   +0.03%     
==========================================
  Files         112      112              
  Lines       86337    86642     +305     
  Branches    86337    86642     +305     
==========================================
+ Hits        76812    77113     +301     
- Misses       7289     7290       +1     
- Partials     2236     2239       +3     

Files	Coverage Δ
lightning/src/util/test_utils.rs	77.09% <100.00%> (+0.12%)	⬆️
lightning/src/chain/chainmonitor.rs	89.27% <75.00%> (+0.02%)	⬆️

... and 19 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[G8XSU]

Marking this review ready, will squash on request (and add PR description to commit msgs.)

[TheBlueMatt]

Yes, feel free to squash IMO. This patch has very little risk, so I think we shouldn't worry too much about landing it. Please also update the documentation on Persist::update_persisted_channel to note something about "During blockchain synchronization operations, and in some rare cases, ..." rather than just "During blockchain synchronization operations".

[G8XSU]

Will be squashing.

[TheBlueMatt]

Please swap the test_utils changes for the first so that the first commit doesn't break tests.

[TheBlueMatt]

Otherwise LGTM.

[wpaulino]

CI is not happy:

error: unresolved link to `ChannelMonitorUpdate::update_id`
  --> lightning/src/chain/chainmonitor.rs:56:56
   |
56 |         /// implementation). This corresponds to an actual [`ChannelMonitorUpdate::update_id`] field
   |                                                              ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no item named `ChannelMonitorUpdate` in scope
   |

error: unresolved link to `ChannelMonitor::get_latest_update_id`
  --> lightning/src/chain/chainmonitor.rs:57:13
   |
57 |         /// and [`ChannelMonitor::get_latest_update_id`].
   |                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no item named `ChannelMonitor` in scope

error: unresolved link to `ChainMonitor`
  --> lightning/src/chain/chainmonitor.rs:60:20
   |
60 |         /// generating [`ChainMonitor`] and does *not* correspond to any on-disk IDs.
   |                          ^^^^^^^^^^^^ no item named `ChainMonitor` in scope
   |

[G8XSU]

Can make this "Instead, we should persist the entire channel_monitor here, otherwise we will fail to initialize this monitor during restart."

But wasn't sure if it adds more confusion.

[TheBlueMatt]

The previous sentence makes that clear imo.