Replace default hash with birthday hash #823

jkczyz · 2021-03-03T20:51:54Z

When ChannelMonitors are persisted, they need to store the most recent block hash seen. However, for newly created channels the default block hash is used. If persisted before a block is connected, the funding output may be missed when syncing after a restart. Instead, initialize ChannelManager with a "birthday" hash so it can be used later when creating channels.

Fixes #820.

jkczyz · 2021-03-03T20:57:32Z

I kept the field names the same across ChannelManager, Channel, and ChannelMonitor. But this might be a good opportunity to make them consistent (i.e. last vs latest and block_hash vs block_connected).

codecov · 2021-03-03T21:06:31Z

Codecov Report

Merging #823 (8730148) into main (1a8b9be) will increase coverage by 0.05%.
The diff coverage is 94.38%.

@@            Coverage Diff             @@
##             main     #823      +/-   ##
==========================================
+ Coverage   90.97%   91.02%   +0.05%     
==========================================
  Files          48       48              
  Lines       26547    26538       -9     
==========================================
+ Hits        24151    24157       +6     
+ Misses       2396     2381      -15

Impacted Files	Coverage Δ
lightning-block-sync/src/init.rs	`93.71% <ø> (ø)`
lightning/src/ln/channel.rs	`87.80% <86.84%> (-0.04%)`	⬇️
lightning-persister/src/lib.rs	`92.52% <100.00%> (ø)`
lightning/src/chain/channelmonitor.rs	`95.52% <100.00%> (-0.01%)`	⬇️
lightning/src/ln/chanmon_update_fail_tests.rs	`97.56% <100.00%> (ø)`
lightning/src/ln/channelmanager.rs	`85.47% <100.00%> (+0.02%)`	⬆️
lightning/src/ln/functional_test_utils.rs	`95.02% <100.00%> (+0.02%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.10% <100.00%> (+0.24%)`	⬆️
lightning/src/ln/reorg_tests.rs	`99.58% <100.00%> (ø)`
lightning/src/util/test_utils.rs	`83.60% <100.00%> (ø)`
... and 1 more

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 1a8b9be...8730148. Read the comment docs.

jkczyz · 2021-03-04T17:17:50Z

lightning/src/ln/channelmanager.rs

-		let channel = Channel::new_outbound(&self.fee_estimator, &self.keys_manager, their_network_key, channel_value_satoshis, push_msat, user_id, config)?;
+		let block_hash = *self.last_block_hash.lock().unwrap();
+		let channel = Channel::new_outbound(&self.fee_estimator, &self.keys_manager, their_network_key, channel_value_satoshis, push_msat, user_id, config, block_hash)?;


@TheBlueMatt It occurred to me while working on a follow-up that we may want to hold the last_block_hash lock for the duration of creating the channel (i.e., also covering the duration channel_state lock is held here). And that the same should be done in block_connected / block_disconnected.

Otherwise, we may create a channel with a block hash, process a new block, then add the new channel after the block has been processed but with the previous block hash. But maybe this is not a problem?

However, holding locks in this manner means create_channel / internal_open_channel would block block_connected / block_disconnected. And this includes calling out to the fee estimator.

(Note that both already involve holding these locks only in a non-overlapping manner.)

Does arranging locking like this sound reasonable? If not clear, I'm proposing:

lock last_block_hash

lock channel_state

unlock channel_state

unlock last_block_hash

Ah! That is a problem as we'll refuse to deserialize a ChannelManager that has Channels with different current block hashes set (as long as they aren't 0). I think an easier fix is to just move the last_block_hash update to the top of block_connected/disconnected, which avoids taking locks for longer.

Won't channels still get different block hashes in the following scenario?

create_channel locks last_block_hash and uses this for the new channel

create_channel unlocks last_block_hash

block_connected locks last_block_hash and updates its value

block_connected unlocks last_block_hash

block_connected locks channel_state and calls block_connected on each channel

block_connected unlocks channel_state

create_channel locks channel_state and inserts the new channel but with the previous last_block_hash

create_channel unlocks channel_state

Another question that came to mine: does it matter that a new Channel is given last_block_hash even though block_connected was not called on it for that hash?

Won't channels still get different block hashes in the following scenario?

Ugh, indeed. Seems fine enough to just hold the lock until the channel is inserted.

Another question that came to mine: does it matter that a new Channel is given last_block_hash even though block_connected was not called on it for that hash?

I don't believe so. The channel-current-hash is basically unused, its now just used to initialize the ChannelMonitor. Its checked at deserialization time, but that's about it. Maybe an easier way to solve the locking issue is just to remove it.

I don't believe so. The channel-current-hash is basically unused, its now just used to initialize the ChannelMonitor. Its checked at deserialization time, but that's about it. Maybe an easier way to solve the locking issue is just to remove it.

Ah, good idea! So just pass it to Channel when needed to create a ChannelMonitor.

Sounds good to me, no races there.

TheBlueMatt · 2021-03-04T20:11:22Z

Were you also going to fix the bug you found where we incorrectly track the current block hash in ChannelManager::block_disconnected? Could go in a separate pr but just checking if you wanted to throw it in this one.

jkczyz · 2021-03-04T21:08:46Z

Were you also going to fix the bug you found where we incorrectly track the current block hash in ChannelManager::block_disconnected? Could go in a separate pr but just checking if you wanted to throw it in this one.

Yeah, I have it ready but wanted to resolve the locking question first. I can include in this PR without any problem.

jkczyz · 2021-03-05T03:41:03Z

All should be good for review now. I included some lock acquisition ordering in 1c948a5, which may not be strictly necessary but gives consistency. Additionally, I removed some block_connected de-duplication detection in 1316265 since I don't believe this is necessary anymore (and it relied on last_connected_block).

Were you also going to fix the bug you found where we incorrectly track the current block hash in ChannelManager::block_disconnected? Could go in a separate pr but just checking if you wanted to throw it in this one.

Yeah, I have it ready but wanted to resolve the locking question first. I can include in this PR without any problem.

Included in 390ff52.

TheBlueMatt · 2021-03-05T17:18:47Z

I included some lock acquisition ordering in 1c948a5, which may not be strictly necessary but gives consistency.

This actually scares me - Rust has no lock-order-consistency checking, as far as I know, so any time we take two locks at once we have to manually track and ensure correctness of lock ordering. Its definitely simpler to just avoid it where possible, but if we can't we at least need to document the lock order requirements carefully (in this case I think its fine, lock order must always be channel_state followed by block hash).

All the other changes look good.

Do you think it would make sense to take this to add runtime assertions to ensure users connect in-order (would likely need documentation to the effect that block events may panic if you screw up ordering)? https://git.bitcoin.ninja/?p=rust-lightning;a=commitdiff;h=c60560291c1f426ade3c42863dd2a008525a3f73

jkczyz · 2021-03-05T17:30:36Z

I included some lock acquisition ordering in 1c948a5, which may not be strictly necessary but gives consistency.

This actually scares me - Rust has no lock-order-consistency checking, as far as I know, so any time we take two locks at once we have to manually track and ensure correctness of lock ordering. Its definitely simpler to just avoid it where possible, but if we can't we at least need to document the lock order requirements carefully (in this case I think its fine, lock order must always be channel_state followed by block hash).

Fair point. I consider moving last_block_hash into ChannelHolder in order to avoid this, but it didn't seem to belong -- also it seemed the height update should be in-sync. Not sure if you have opinions on either matter, but happy to document the ordering requirements.

All the other changes look good.

Do you think it would make sense to take this to add runtime assertions to ensure users connect in-order (would likely need documentation to the effect that block events may panic if you screw up ordering)? https://git.bitcoin.ninja/?p=rust-lightning;a=commitdiff;h=c60560291c1f426ade3c42863dd2a008525a3f73

I started down this route but a significant number of tests broke as it seems you've noticed by configuring against it. Looks like we use the default hash for prev_blockhash in many places.

Eventually, we may want shore up our testing code, but for now conditionally checking and documenting is definitely worthwhile. I'll add it.

TheBlueMatt · 2021-03-05T17:36:17Z

Fair point. I consider moving last_block_hash into ChannelHolder in order to avoid this, but it didn't seem to belong -- also it seemed the height update should be in-sync. Not sure if you have opinions on either matter, but happy to document the ordering requirements.

Hmm, I'm ok with whatever. I kinda prefer just leaving things be out-of-sync (that's what total_consistency_lock is for!) but we could move into the ChannelHolder too, though ChannelHolder will hopefully eventually be turned into per-channel locks, too, so that may end up being split out. If you feel strongly that it needs to be in the same lock, I'd prefer to move it to ChannelHolder over the current version, I think, though.

TheBlueMatt · 2021-03-05T20:08:56Z

Going to merge #808 which is going to (rather trivially) conflict with this one, sorry.

…03-810-rebased" This reverts commit 793de5f, reversing changes made to 03a5189.

Co-authored-by: Matt Corallo <[email protected]> Co-authored-by: Jeffrey Czyz <[email protected]>

jkczyz · 2021-03-05T22:57:16Z

Hmm, I'm ok with whatever. I kinda prefer just leaving things be out-of-sync (that's what total_consistency_lock is for!) but we could move into the ChannelHolder too, though ChannelHolder will hopefully eventually be turned into per-channel locks, too, so that may end up being split out. If you feel strongly that it needs to be in the same lock, I'd prefer to move it to ChannelHolder over the current version, I think, though.

Ended up holding the locks independently of one another.

Going to merge #808 which is going to (rather trivially) conflict with this one, sorry.

Rebased.

TheBlueMatt · 2021-03-05T23:07:32Z

lightning/src/ln/channelmanager.rs

+/// Chain-related parameters used to construct a new `ChannelManager`.
+///
+/// Typically, the block-specific parameters are derived from the best block hash for the network,
+/// as a newly constructed `ChannelManager` will not have created any channels yet. These parameters


Does it make sense to note that once you instantiate the ChannelManager, you have to start connecting blocks starting from the ones provided?

Yeah, though added to ChannelManager::new docs since that is where block_connected is discussed and ChainParameters is used.

TheBlueMatt · 2021-03-05T23:25:39Z

lightning/src/ln/channelmanager.rs

@@ -3353,6 +3384,12 @@ impl<Signer: Sign, M: Deref, T: Deref, K: Deref, F: Deref, L: Deref> ChannelMana
 		// during initialization prior to the chain_monitor being fully configured in some cases.
 		// See the docs for `ChannelManagerReadArgs` for more.
 		let _persistence_guard = PersistenceNotifierGuard::new(&self.total_consistency_lock, &self.persistence_notifier);
+
+		assert_eq!(*self.last_block_hash.read().unwrap(), header.block_hash(),


Oops, probably best to drop the test commit, looks like its still failing the fuzzer, we can take it in a separate PR.

Odd, was passing locally. Will drop the commit for now.

TheBlueMatt

Two comments, but otherwise good.

When ChannelMonitors are persisted, they need to store the most recent block hash seen. However, for newly created channels the default block hash is used. If persisted before a block is connected, the funding output may be missed when syncing after a restart. Instead, initialize ChannelManager with a "birthday" hash so it can be used later when creating channels.

Calling block_connected for the same block was necessary when clients were expected to re-fetch filtered blocks with relevant transactions. At the time, all listeners were notified of the connected block again for re-scanning. Thus, de-duplication logic was put in place. However, this requirement is no longer applicable for ChannelManager as of commit bd39b20, so remove this logic.

Tracking the last block was only used to de-duplicate block_connected calls, but this is no longer required as of the previous commit. Further, the ChannelManager can pass the latest block hash when needing to create a ChannelMonitor rather than have each Channel maintain an up-to-date copy. This is implemented in the next commit.

ChannelMonitor keeps track of the last block connected. However, it is initialized with the default block hash, which is a problem if the ChannelMonitor is serialized before a block is connected. Instead, pass ChannelManager's last_block_hash, which is initialized with a "birthday" hash, when creating a new ChannelMonitor.

ChannelManager reads channel_state and last_block_hash while processing funding_created and funding_signed messages. It writes these while processing block_connected and block_disconnected events. To avoid any potential deadlocks, have each site hold these locks independent of one another and in a consistent order. Additionally, use a RwLock instead of Mutex for last_block_hash since exclusive access is not needed in funding_created / funding_signed and cannot be guaranteed in block_connected / block_disconnected because of the reads in the former.

When a block is disconnected, the hash of the disconnected block was used to update the last connected block. However, this amounts to a no-op because these hashes should be equal. Successive disconnections would update the hash but leave it one block off. Normally, this not a problem because the last block_disconnected should be followed by block_connected since the former is triggered by a chain re-org. However, this assumes the user calls the API correctly and that no failure occurs that would prevent block_connected from being called (e.g., if fetching the connected block fails). Instead, update the last block hash with the disconnected block's previous block hash.

arik-so

LGTM

jkczyz force-pushed the 2021-03-birthday-hash branch 2 times, most recently from 79bb425 to 677fd37 Compare March 3, 2021 21:55

jkczyz commented Mar 4, 2021

View reviewed changes

jkczyz force-pushed the 2021-03-birthday-hash branch from 677fd37 to 390ff52 Compare March 5, 2021 03:25

jkczyz and others added 2 commits March 5, 2021 13:35

Revert "Merge pull request lightningdevkit#819 from TheBlueMatt/2021-…

4cd2e4e

…03-810-rebased" This reverts commit 793de5f, reversing changes made to 03a5189.

Update docs with correct hash type

af49a60

Co-authored-by: Matt Corallo <[email protected]> Co-authored-by: Jeffrey Czyz <[email protected]>

jkczyz force-pushed the 2021-03-birthday-hash branch from 390ff52 to 3ec662b Compare March 5, 2021 22:54

TheBlueMatt reviewed Mar 5, 2021

View reviewed changes

TheBlueMatt approved these changes Mar 5, 2021

View reviewed changes

jkczyz added 7 commits March 5, 2021 15:44

Rename header_hash to block_hash

d21d8b3

jkczyz force-pushed the 2021-03-birthday-hash branch from 3ec662b to 8730148 Compare March 5, 2021 23:45

arik-so approved these changes Mar 6, 2021

View reviewed changes

TheBlueMatt merged commit b5d88a5 into lightningdevkit:main Mar 6, 2021

TheBlueMatt mentioned this pull request Mar 7, 2021

Add assertions for in-order block [dis]connection in ChannelManager #831

Closed

Replace default hash with birthday hash #823

Replace default hash with birthday hash #823

Uh oh!

Conversation

jkczyz commented Mar 3, 2021

Uh oh!

jkczyz commented Mar 3, 2021

Uh oh!

codecov bot commented Mar 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt Mar 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Mar 4, 2021

Uh oh!

jkczyz commented Mar 4, 2021

Uh oh!

jkczyz commented Mar 5, 2021

Uh oh!

TheBlueMatt commented Mar 5, 2021

Uh oh!

jkczyz commented Mar 5, 2021

Uh oh!

TheBlueMatt commented Mar 5, 2021

Uh oh!

TheBlueMatt commented Mar 5, 2021

Uh oh!

jkczyz commented Mar 5, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

arik-so left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov bot commented Mar 3, 2021 •

edited

Loading

TheBlueMatt Mar 4, 2021 •

edited

Loading