Serialize ChannelManager events

[sourabhmarathe]

Addresses issue #538

[codecov]

Codecov Report

Merging #617 into master will increase coverage by 0.04%.
The diff coverage is 97.95%.

@@            Coverage Diff             @@
##           master     #617      +/-   ##
==========================================
+ Coverage   91.19%   91.24%   +0.04%     
==========================================
  Files          35       35              
  Lines       20693    20787      +94     
==========================================
+ Hits        18872    18967      +95     
+ Misses       1821     1820       -1     

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	85.52% <93.33%> (+0.02%)	⬆️
lightning/src/ln/functional_tests.rs	97.10% <98.79%> (-0.08%)	⬇️
lightning/src/util/events.rs	29.03% <0.00%> (+8.60%)	⬆️

---

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 b5723c7...9c587e5. Read the comment docs.

[TheBlueMatt]

Thanks, looking good! First of all, note that your editor seems confused and the indentation is off in a few places. You should use tabs to match existing indentation.

More importantly, it would be great to get a test for, eg, FundingBroadcstSafe here (mostly because I'm not sure if those already are regenerated on load). You can probably copy/extend one of the serialize_deserialize tests in src/ln/functional_tests, eg test_simple_manager_serialize_deserialize() as a starting point. Then just add eg partial channel open by replacing the call to create_announced_chan_between_nodes with its constituent parts (most of our test utils just call other test utils, in this case you probably want to call through create_chan_between_nodes_with_value_a and then check that you can call create_chan_between_nodes_with_value_b after the reload, or something like that.

[valentinewallace]

Thanks for the PR & for adding the test! :)

[sourabhmarathe]

Thanks for the comments! I got the changes in, and this PR should be ready for another look.

[TheBlueMatt]

I did notice for the other fields, the cmp::min is either the count variable or a hard coded 128 value. Should I create that variable let MAX_ALLOC_SIZE: u64 = 54*1024; and then use that for the other values being read in as well?

Yep, totally! The old ones were just the math done manually, but there's no reason for that and its less clear.

[TheBlueMatt]

Test looks great! good job on that. One nit and one note, but otherwise ready to go. If you feel like changing all the other deserialization with_capacity() calls, feel free, otherwise it can be done at some point in the future.

[sourabhmarathe]

Test looks great! good job on that. One nit and one note, but otherwise ready to go. If you feel like changing all the other deserialization with_capacity() calls, feel free, otherwise it can be done at some point in the future.

I took care of it, I believe I mapped over what was was done for events to the other types properly, but you may want to confirm that. Otherwise, got the PR up to date with master.

[TheBlueMatt]

Oops, the inner in the hashmap isn't a HTLCForwardInfo, its a (u64, Vec<_>). The HTLCForwardInfo-relevant line is four lines down when we allocate the Vec::with_capacity(). The same goes for claimable_htlcs below.

[sourabhmarathe]

Addressed forward_htlcs and claimable_htlcs (put them back to 128) and for per_peer_state's size, used its type: <RwLock<HashMap<PublicKey, Mutex<PeerState>>>> for the denominator under MAX_ALLOC_SIZE

[TheBlueMatt]

Instead of RwLock<HashMap<PublicKey, Mutex<PeerState>>>, you probably want (PublicKey, Mutex<PeerState>).

The goal here is to capture that we only ever want to allocate up to 64KB for internal contents at a time - in a Vec the internal, heap-allocated things are of type X (where the vec is Vec). In a HashMap, the internal contents are usually (K, V) (where its a HashMap<K, V>), though I don't think Rust guarantees this, its close enough for our purposes, it doesn't have to be exact. We always allocate the RwLock<HashMap<>> data on the stack, so its not the variable part, its the number of (K, V) entries that is the variable part.