Change ChannelManager deserialization to return an optional blockhash

[TheBlueMatt]

This is just #810 rebased with one additional commit on top to update documentation.

[codecov]

Codecov Report

Merging #819 (8550bd4) into main (81c6bdc) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #819   +/-   ##
=======================================
  Coverage   90.73%   90.74%           
=======================================
  Files          48       48           
  Lines       25642    25648    +6     
=======================================
+ Hits        23267    23274    +7     
+ Misses       2375     2374    -1     

Impacted Files	Coverage Δ
lightning-block-sync/src/init.rs	93.56% <ø> (ø)
lightning-persister/src/lib.rs	92.52% <100.00%> (ø)
lightning/src/chain/channelmonitor.rs	95.53% <100.00%> (+<0.01%)	⬆️
lightning/src/ln/chanmon_update_fail_tests.rs	97.56% <100.00%> (ø)
lightning/src/ln/channelmanager.rs	85.28% <100.00%> (+0.02%)	⬆️
lightning/src/ln/functional_test_utils.rs	94.97% <100.00%> (ø)
lightning/src/ln/functional_tests.rs	96.96% <100.00%> (+0.01%)	⬆️
lightning/src/util/test_utils.rs	83.49% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 81c6bdc...8550bd4. Read the comment docs.

[TheBlueMatt]

Fixed nits and squashed. Gonna merge. Full diff from Val's original work is:

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index c989b44f..4d2d1b84 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -620,7 +620,7 @@ impl Readable for ChannelMonitorUpdateStep {
 /// reloaded at deserialize-time. Thus, you must ensure that, when handling events, all events
 /// gotten are fully handled before re-serializing the new state.
 ///
-/// Note that the deserializer is only implemented for (Sha256dHash, ChannelMonitor), which
+/// Note that the deserializer is only implemented for (Option<BlockHash>, ChannelMonitor), which
 /// tells you the last block hash which was block_connect()ed. You MUST rescan any blocks along
 /// the "reorg path" (ie disconnecting blocks until you find a common ancestor from both the
 /// returned block hash and the the current chain and then reconnecting blocks to get to the
@@ -2735,10 +2735,11 @@ impl<'a, Signer: Sign, K: KeysInterface<Signer = Signer>> ReadableArgs<&'a K>
                let mut secp_ctx = Secp256k1::new();
                secp_ctx.seeded_randomize(&keys_manager.get_secure_random_bytes());
 
-               let mut last_seen_block_hash = Some(last_block_hash.clone());
-               if last_block_hash == Default::default() {
-                       last_seen_block_hash = None;
-               }
+               let last_seen_block_hash = if last_block_hash == Default::default() {
+                       None
+               } else {
+                       Some(last_block_hash)
+               };
 
                Ok((last_seen_block_hash, ChannelMonitor {
                        inner: Mutex::new(ChannelMonitorImpl {
diff --git a/lightning/src/ln/channelmanager.rs b/lightning/src/ln/channelmanager.rs
index b64c488a..d3e520f7 100644
--- a/lightning/src/ln/channelmanager.rs
+++ b/lightning/src/ln/channelmanager.rs
@@ -380,7 +380,7 @@ pub type SimpleRefChannelManager<'a, 'b, 'c, 'd, 'e, M, T, F, L> = ChannelManage
 /// ChannelMonitors passed by reference to read(), those channels will be force-closed based on the
 /// ChannelMonitor state and no funds will be lost (mod on-chain transaction fees).
 ///
-/// Note that the deserializer is only implemented for (Sha256dHash, ChannelManager), which
+/// Note that the deserializer is only implemented for (Option<BlockHash>, ChannelManager), which
 /// tells you the last block hash which was block_connect()ed. You MUST rescan any blocks along
 /// the "reorg path" (ie call block_disconnected() until you get to a common block and then call
 /// block_connected() to step towards your best block) upon deserialization before using the
@@ -3925,7 +3925,7 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> Writeable f
 /// At a high-level, the process for deserializing a ChannelManager and resuming normal operation
 /// is:
 /// 1) Deserialize all stored ChannelMonitors.
-/// 2) Deserialize the ChannelManager by filling in this struct and calling <(Sha256dHash,
+/// 2) Deserialize the ChannelManager by filling in this struct and calling <(Option<BlockHash>,
 ///    ChannelManager)>::read(reader, args).
 ///    This may result in closing some Channels if the ChannelMonitor is newer than the stored
 ///    ChannelManager state to ensure no loss of funds. Thus, transactions may be broadcasted.
@@ -4172,10 +4172,11 @@ impl<'a, Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref>
                //TODO: Broadcast channel update for closed channels, but only after we've made a
                //connection or two.
 
-               let mut last_seen_block_hash = Some(last_block_hash.clone());
-               if last_block_hash == Default::default() {
-                       last_seen_block_hash = None;
-               }
+               let last_seen_block_hash = if last_block_hash == Default::default() {
+                       None
+               } else {
+                       Some(last_block_hash)
+               };
                Ok((last_seen_block_hash, channel_manager))
        }
 }

[ariard]

Code Review ACK 8550bd4