Refactor chain monitoring #649

jkczyz · 2020-06-23T16:53:49Z

ChainWatchInterface introduces a dependency between BlockNotifier's listeners, which can lead to complex interactions which aren't easy to understand and can be error prone. ~~This PR refactors BlockNotifier to take the entire block and leaves filtering to individual ChainListeners where required.~~

As a result, the block_connected method interfaces are identical between BlockNotifier and ChainListener. Thus, an adaptor is not needed in #614. Further refactoring around ChainWatchInterface and the reentered logic is left to a follow-up PR.

~~The PR also simplifies BlockNotifier tests by removing duplication and unnecessary dependencies.~~

This PR refactors chain monitoring in the following manner:

Add chain::Access and chain::Filter traits
Replace ManyChannelMonitor with chain::Watch trait, which is functionally the same
Replace ChainWatchInterface in NetGraphMsgHandler with an optional chain::Access
Replace ManyChannelMonitor in ChannelManager with chain::Watch
Replace ChainWatchInterface in SimpleManyChannelMonitor with an optional chain::Filter
Rename SimpleManyChannelMonitor as ChainMonitor
Remove ChainWatchInterface, ChainWatchInterfaceUtil, ChainWatchedUtil, and ChainError
Remove BlockNotifier and ChainListener

This allows for various setups like using a full node, Electrum, or a BIP157/BIP158 light client for chain source, as wells as local or remote channel monitoring.

TheBlueMatt · 2020-06-23T16:59:00Z

Wait, how does one go about implementing using this without downloading the full block?

jkczyz · 2020-06-23T20:12:30Z

Wait, how does one go about implementing using this without downloading the full block?

TL; DR: Defining an interface in terms of a ChainListener trait is only useful if something is making use of the trait.

Chatted a bit offline. My thinking is blocks are fed through BlockNotifier to the ChainListeners, so there's no reason for the interface to differ.

@TheBlueMatt noted that for bandwidth reasons users may want to feed partial blocks to a ChainListener. They would do so by bypassing BlockNotifier entirely.

I'd argue that ChainListener is only useful in the context of BlockNotifier. If a user must feed a partial block to a listener directly, then there is no need to do so through the ChainListener interface -- the user could simply call a non-trait method. Such a method should be added as part of each listener's public interface (which I'm happy to do).

Alternatively, a new abstraction can be added to notify listeners of partial blocks or possibly individual transactions. This could use a separate listener interface (e.g., perhaps TransactionListener vs BlockListener) or expand upon the current ChainListener interface by adding another method. Or possibly using some other variation of these.

TheBlueMatt · 2020-06-23T20:34:30Z

I’m certainly happy with either - the original goal was a generic interface across potentially many structs that listen for chain data, but after all was said and done we ended up with only two that care - ChannelMonitors, which may need the rescanning logic and a single ChannelManager which (I believe) does not. Given that, it may not make any sense to have a trait at all and just focus on utilities to do the rescanning for ChannelMonitors.

…

On Jun 23, 2020, at 13:12, Jeffrey Czyz ***@***.***> wrote: Wait, how does one go about implementing using this without downloading the full block? TL; DR: Defining an interface in terms of a ChainListener trait is only useful if something is making use of the trait. Chatted a bit offline. My thinking is blocks are fed through BlockNotifier to the ChainListeners, so there's no reason for the interface to differ. @TheBlueMatt noted that for bandwidth reasons users may want to feed partial blocks to a ChainListener. They would do so by bypassing BlockNotifier entirely. I'd argue that ChainListener is only useful in the context of BlockNotifier. If a user must feed a partial block to a listener directly, then there is no need to do so through the ChainListener interface -- the user could simply call a non-trait method. Such a method should be added as part of each listener's public interface (which I'm happy to do). Alternatively, a new abstraction can be added to notify listeners of partial blocks or possibly individual transactions. This could use a separate listener interface (e.g., perhaps TransactionListener vs BlockListener) or expand upon the current ChainListener interface by adding another method. Or possibly using some other variation of these. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or unsubscribe.

jkczyz · 2020-06-25T00:33:47Z

Rebased and resolved conflicts with #638.

From yesterday's discussions, I'm working on adding an interface to ChannelManager for partial blocks. Will ping the PR when ready.

There's also a fuzz test breakage at the following line, which I haven't had much time to dig into: https://github.com/rust-bitcoin/rust-lightning/blob/49b8c58165b0e1a6545021a0c91add92f2ebeadd/fuzz/src/full_stack.rs#L898

TheBlueMatt · 2020-06-25T17:59:28Z

Hmm, its possible you broke it, its also possible that check is too aggressive and its checking that we hit that line multiple times due to a rescan? If you compare the test output prior to the patch and after you should be able to figure out if its correct.

jkczyz · 2020-06-26T18:41:07Z

Updated to support both full and partial (filtered) blocks. Figured the cleanness way to support filtered blocks is to define ChainListener::block_connected to handle both cases. Looks a little similar to before, only using a vector of tuples rather than two separate vectors. I kept BlockNotifier's interface in terms of Block, but this could be changed later if needed.

Hmm, its possible you broke it, its also possible that check is too aggressive and its checking that we hit that line multiple times due to a rescan? If you compare the test output prior to the patch and after you should be able to figure out if its correct.

Looks like I broke it while updating MoneyLossDetector::block_connected. Fixed now after making the change mentioned above.

lightning/src/chain/chaininterface.rs

TheBlueMatt · 2020-06-27T00:18:39Z

lightning/src/chain/chaininterface.rs

-	/// This also means those counting confirmations using block_connected callbacks should watch
-	/// for duplicate headers and not count them towards confirmations!
-	fn block_connected(&self, header: &BlockHeader, height: u32, txn_matched: &[&Transaction], indexes_of_txn_matched: &[usize]);
+	/// Notifies a listener that a block was connected. Transactions may be filtered and are given


This doc could use more details, I think (eg parameter usage, etc).

lightning/src/chain/chaininterface.rs

valentinewallace · 2020-06-29T16:11:21Z

lightning/src/ln/functional_test_utils.rs

-	let mut header = BlockHeader { version: 0x20000000, prev_blockhash: Default::default(), merkle_root: Default::default(), time: 42, bits: 42, nonce: 42 };
-	notifier.block_connected_checked(&header, 1, &[tx; 1], &[chan_id as usize; 1]);
+pub fn confirm_transaction<'a, 'b: 'a>(notifier: &'a chaininterface::BlockNotifierRef<'b>, tx: &Transaction) {
+	let dummy_tx = Transaction { version: 0, lock_time: 0, input: Vec::new(), output: Vec::new() };


Is there a dummy tx included just to make the block more "realistic"?

Previously, this test utility was calling block_connected_checked directly with data already filtered by ChainWatchInterfaceUtil. I removed that function now that I've pushed ChainWatchInterface down into the listener (i.e., SimpleManyChannelMonitor), so I can no longer pass filtered data to BlockNotifier directly. Instead, I'm essentially padding the block with dummy transactions up to the desired index then adding the passed tx to the end of the vector, which has the same effect as before.

FWIW, this won't be needed if BlockNotifier supports filtered blocks directly. But as noted in #649 (comment), I've left that for a future improvement. Given our discussion, we have to iron out some details still.

lightning/src/ln/channelmonitor.rs

jkczyz · 2020-07-21T06:30:56Z

Updated the PR based on some of our offline discussions. In summary:

Replaced ChainWatchInterface in NetGraphMsgHandler with chain::Access
Replaced ManyChannelMonitor in ChannelManager with chain::Watch, which is essentially ManyChannelMonitor with the trait and functions renamed
Replaced ChainWatchInterface in SimpleManyChannelMonitor by implementing chain::WatchEventProvider for signaling back which transactions and outpoints to watch via chain::WatchEvents
Removed ManyChannelMonitor
Renamed SimpleManyChannelMonitor to ChainMonitor
Removed ChainWatchInterface and ChainWatchInterfaceUtil

I like the naming but am happy to bikeshed on it later. Will clean-up documentation and commit messages once we're happy with it.

I did not remove BlockNotifier and ChainListener (yet?). However, I did add a connect_block test utility for handling functionality that could be covered by BlockNotifier assuming it can interact with a block source for processing the new events. There appears to be a dependency between the order in which the listeners are notified of blocks in some tests. I'd like to understand this further and determine if it can be removed.

Also, since ChannelMonitors can return outpoints that were already signaled, ChainWatchedUtil is still needed but is used directly by ChainMonitor now for managing event production. This behavior is encapsulated by WatchEventQueue in channelmonitor.rs.

Block filtering is still inside what is now ChainMonitor. I'd like to explore how this might be changed, which may affect what the BlockListener interface (or what becomes of it) will look like.

I suspect we'll parameterize ChainMonitor by the forthcoming data persistence trait. Test structs may be able to use a custom parameterization instead of wrapping ChainMonitor.

jkczyz · 2020-08-04T02:13:28Z

Updated PR based on most recent offline discussions. In summary:

Parameterized ChainMonitor by a new chain::Notify trait instead of using events
Removed BlockNotifier and ChainListener
Added documentation for new traits
Updated current documentation
Updated architecture diagram

codecov · 2020-08-05T00:39:40Z

Codecov Report

Merging #649 into master will increase coverage by 0.03%.
The diff coverage is 98.06%.

@@            Coverage Diff             @@
##           master     #649      +/-   ##
==========================================
+ Coverage   91.95%   91.99%   +0.03%     
==========================================
  Files          36       37       +1     
  Lines       20200    20106      -94     
==========================================
- Hits        18575    18496      -79     
+ Misses       1625     1610      -15

Impacted Files	Coverage Δ
lightning/src/chain/transaction.rs	`100.00% <ø> (ø)`
lightning/src/ln/functional_tests.rs	`97.02% <ø> (+0.08%)`	⬆️
lightning/src/util/errors.rs	`64.70% <ø> (ø)`
lightning/src/util/macro_logger.rs	`89.79% <ø> (ø)`
lightning/src/util/test_utils.rs	`84.97% <92.30%> (-0.03%)`	⬇️
lightning/src/routing/network_graph.rs	`94.22% <92.85%> (-0.11%)`	⬇️
lightning/src/chain/channelmonitor.rs	`95.36% <93.75%> (ø)`
lightning/src/chain/chainmonitor.rs	`100.00% <100.00%> (ø)`
lightning/src/chain/mod.rs	`100.00% <100.00%> (ø)`
lightning/src/ln/chan_utils.rs	`97.37% <100.00%> (ø)`
... and 13 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 8640173...6cd6816. Read the comment docs.

TheBlueMatt

Before I get too far in, module placement feels...splitbrain right now. It'd be nice to clean that up a bit.

TheBlueMatt · 2020-08-06T17:22:11Z

lightning/src/ln/channelmonitor.rs

+	events: Vec<WatchEvent>,
+}
+
+/// An event indicating on-chain activity to watch for pertaining to a channel.


Given we're already calling flush_events reentrant-ly here, does it really make sense to build a bit cache thing? Or is this just to do the filtering inside ChainMonitor (and it will be removed in a followup?)? I'm a bit confused why we can't just drop this now if there's filtering at the callsite in tests.

This encapsulates a few behaviors currently:

filtering transactions (as you mentioned)

preventing duplicate calls to chain::Notify for the same watch events

determining if any new watch events were generated (and thus what block_connected should return)

I should probably add some documentation around this. 😄

I see your point regarding filtering. But if I remove filtering from block_connected, I see a few test failures:

failures: ln::functional_tests::test_bump_penalty_txn_on_revoked_htlcs ln::functional_tests::test_static_spendable_outputs_justice_tx_revoked_htlc_success_tx ln::functional_tests::test_static_spendable_outputs_justice_tx_revoked_htlc_timeout_tx

Let me know if you think it would be worth investigating these. I was mainly trying to maintain the same behavior, but I could definitely see removing the filtering here if desirable, whether now or in a follow-up.

Regarding the other points: Preventing duplicates will require investigating why ChannelMonitor is not just returning new outputs. Counting new events is necessary since it needs to account for these duplicates. So resolving the duplicates problem would remove the need for this -- or very least make it straightforward enough to do at each call site.

So the short answer is yes, we can get rid of this but would need to resolve a few other issues first.

preventing duplicate calls to chain::Notify for the same watch events
determining if any new watch events were generated (and thus what block_connected should return)

Hmm, can't the ChannelMonitor remove the duplicate calls itself since it now has the full monitoring data? Then if there were any returned values you know you need to rescan.

I see your point regarding filtering. But if I remove filtering from block_connected, I see a few test failures:

Right, 99% those are just the tests being overly strict, which should be fixed. Totally fine to do it separately, I'm just dubious we can't avoid this new struct even without addressing that.

Hmm, can't the ChannelMonitor remove the duplicate calls itself since it now has the full monitoring data? Then if there were any returned values you know you need to rescan.

Could you clarify what you mean by "since it now has the full monitoring data"? I didn't change ChannelMonitor materially.

Right, it's new in that its from Jan of this year in 73dce20.

I was able to remove WatchEventCache -- and also filtering -- and use the full monitoring data without difficulty, but I'm having trouble with the tests. For test_bump_penalty_txn_on_revoked_htlcs, the number of broadcasted transactions is reduced by one with the inputs of node_txn[2] and node_txn[3] combined into a single transaction (node_txn[2]). I can adjust those assertions, but the check_spends that follows fails. And commenting it out gives an overflow when subtracting in the fee_1 calculation if replacing node_txn[3] with node_txn[2] since the output value is larger now with 3 inputs instead of 2.

Any insight into why the two transactions are combined into one and how to go about modifying this test?

Hmm, not immediately, it may be that the filtering in the monitors is broken. Can you push the WIP commit(s) somewhere I can play with?

Change is in 71c7606 and the attempted fix in 5acef01.

Looks like its claiming a third (revoked) output - https://github.com/TheBlueMatt/rust-lightning/commits/2020-08-check-spends-fees calculates the fees more robustly and uses them for this specific bug. I'm not actually sure why its now spending the commitment-tx revoked output in addition to the HTLC revoked outputs, though. Maybe @ariard can shed some light?

TheBlueMatt · 2020-08-06T17:26:16Z

lightning/src/ln/channelmonitor.rs

@@ -149,8 +155,8 @@ pub struct HTLCUpdate {
 }
 impl_writeable!(HTLCUpdate, 0, { payment_hash, payment_preimage, source });

-/// A simple implementation of a ManyChannelMonitor and ChainListener. Can be used to create a
-/// watchtower or watch our own channels.
+/// A simple implementation of a [`chain::Watch`]. Can be used to create a watchtower or to watch


This seems like a strange place for this now - if Watch is in chain, why is the sample implementation in ln? Also likely worth noting that the watchtower stuff no longer makes sense in the comments here, which probably also implies we should drop the whole weird Key templating - the original thinking was that you'd Key on the OutPoint for local use and a (PublicKey, u32) for remote use where the originator signs updates with a PublicKey. Since ChannelMonitor is now never a watchtower, the Key differentiation makes no sense and we should probably go to just being a chain::Watch sample implementation.

This seems like a strange place for this now - if Watch is in chain, why is the sample implementation in ln?

Good question. I see it less as a sample implementation and more as a typically (re-)used component that also implements chain::Watch.

You may use it directly (parameterized by a data persistence implementation soon) if your channel monitoring is done locally. In which case it is used with ChannelManager as its chain::Watch implementation. But its block_connected/block_disconnected interface is also needed outside of use with ChannelManager.

Or you may reuse it on another server if channel monitoring is done remotely. This is the case where you'd have a proxy implementation of chain::Watch. And here the block interface wouldn't be used locally but would be used remotely.

Also likely worth noting that the watchtower stuff no longer makes sense in the comments here, which probably also implies we should drop the whole weird Key templating - the original thinking was that you'd Key on the OutPoint for local use and a (PublicKey, u32) for remote use where the originator signs updates with a PublicKey. Since ChannelMonitor is now never a watchtower, the Key differentiation makes no sense and we should probably go to just being a chain::Watch sample implementation.

Figured as much regarding watchtowers. Regarding the Key templating, can I assume this outside the scope of this PR?

Good question. I see it less as a sample implementation and more as a typically (re-)used component that also implements chain::Watch.

That doesn't really change the meat of the issue, though, no? If its really just a chain::Watch that you're supposed to use, why isn't it in chain, and why is it in ln?

can I assume this outside the scope of this PR?

Sure, you can do it later, I don't feel strongly either way.

That doesn't really change the meat of the issue, though, no? If its really just a chain::Watch that you're supposed to use, why isn't it in chain, and why is it in ln?

Got it. Decided to move in other thread.

Sure, you can do it later, I don't feel strongly either way.

Ah, I was confusing this with the ChannelSigner parameter which must implement ChannelKeys. I'll remove the Key parameter in this PR.

Also likely worth noting that the watchtower stuff no longer makes sense in the comments here, which probably also implies we should drop the whole weird Key templating - the original thinking was that you'd Key on the OutPoint for local use and a (PublicKey, u32) for remote use where the originator signs updates with a PublicKey. Since ChannelMonitor is now never a watchtower, the Key differentiation makes no sense and we should probably go to just being a chain::Watch sample implementation.

Actually, I'll need some clarification to better understand the watchtower case. What was the u32 meant to signify in the key?

Also, what is meant by "ChannelMonitor is now never a watchtower"? Did you mean ChainMonitor?

No, up until (I believe) this year, ChannelMonitor contained an enum that allowed it to "be in watchtower mode" (ie not have private keys), but it was never fully implemented, so removed. In that context, it would make sense to store ChannelMonitors indexed by "public key of user who signed the update" and some channel id per user.

FYI, @jkczyz we remove the watchtower mode because since then I figured out we can likely embed the watchtower mode behind and thus have multiple-mode of watchtower like WatchtowerFullProtocol or WatchtowerJusticeOnly. Or even experiment with different model for the pre-signed watchtower transactions.

It needs further refactor, see #606

lightning/src/chain/mod.rs

lightning/src/chain/channelmonitor.rs

ariard · 2020-08-21T16:36:26Z

lightning/src/chain/channelmonitor.rs

 			Ok(_) => Ok(()),
 			Err(_) => Err(ChannelMonitorUpdateErr::PermanentFailure),
 		}
 	}

-	fn update_monitor(&self, funding_txo: OutPoint, update: ChannelMonitorUpdate) -> Result<(), ChannelMonitorUpdateErr> {
-		match self.update_monitor_by_key(funding_txo, update) {
+	fn update_channel(&self, funding_txo: OutPoint, update: ChannelMonitorUpdate) -> Result<(), ChannelMonitorUpdateErr> {


I like the move to drop _monitor, that said even if we are inside channelmonitor, that's a bit confusing to have something called update_channel. It let think that ChannelMonitor is responsible for driving change of the off-chain channel. What about update_channel_view (and also replace watch_channel by start/declare_channel_view) ?

I'm not sure I'm able to parse your comment correctly.

It let think that ChannelMonitor is responsible for driving change of the off-chain channel.

What do you mean here? Might be a word missing or typo that's preventing me from understanding the comment.

Sorry that's me. ChannelMonitor doesn't "update" channel in the sense of provide signatures for the counterparty for a newer commitment transaction, it "reacts" on onchain events.

I'm aimed to have a really clear function naming to underscore well the master-slave relation between chain::Watch caller and the implementation here.

ariard · 2020-08-21T16:53:27Z

lightning/src/chain/channelmonitor.rs

-/// watchtower or watch our own channels.
-///
-/// Note that you must provide your own key by which to refer to channels.
+/// An implementation of [`chain::Watch`].


We should mark somewhere that if someone is willingly to implement a BOLT13-like chain::Watch where update needs to be authorized against some credit before to be accepted that's the component to replace.

Generally I think we should start to separate more cleanly interfaces from our default implementations. So we could have lightning/src/chain_processing/watch_interface.rs, lightning/src/chain_processing/watch_impl/chain_monitor.rs, lightning/src/chain_processing/watch_impl/bolt13_monitor.rs ?

Agree that implementations should be separated out from the interface that they are implementing. Would prefer a less verbose naming scheme though. I don't think it's necessary to have interface and impl in module names. Let's punt this to follow-up PRs as needed.

lightning/src/chain/channelmonitor.rs

ariard · 2020-08-21T17:14:26Z

lightning/src/chain/mod.rs

+	/// is unknown.
+	///
+	/// [`short_channel_id`]: https://github.com/lightningnetwork/lightning-rfc/blob/master/07-routing-gossip.md#definition-of-short_channel_id
+	fn get_utxo(&self, genesis_hash: &BlockHash, short_channel_id: u64) -> Result<TxOut, AccessError>;


I think we should deser short_channel_id to its actual block:tx-index:output-index. There is no reason that block provider (either full-node, electrum, bip157) has to know LN-specific data format. That's a layer violation adding a requirement on implementation.

Agreed. Do you mind if I leave this to a follow-up PR? I'd like to limit the scope of this PR as much as possible.

ariard · 2020-08-21T17:22:38Z

lightning/src/chain/mod.rs

+/// blocks are connected and disconnected.
+///
+/// Each channel is associated with a [`ChannelMonitor`]. Implementations are responsible for
+/// maintaining a set of monitors and updating them accordingly as channel state changes and HTLCs


Actually the set of requirements is far wider or should be more laid out with regards to fee-bumping/reorgs/punishment.... Can you add a TODO and I'll do a braindump in a follow-up ?

Those requirements are for ChannelMonitor, though. Right? These docs are simply stating that implementors must ensure that the monitors are updated (1) as channel state changes (i.e., wherever ChannelManager calls update_channel) and (2) HTLCs are resolved on chain (i.e., as blocks are connected).

I've re-worded it slightly to convey that implementors must maintain a set of monitors such that they can be updated as is stated. Essentially, I'm trying to convey that someone implementing chain::Watch must allow for calling update_monitor, block_connected, and block_disconnected on each ChannelMonitor. And I'm saying as much specifically in the docs for watch_channel and update_channel.

Sounds good to me, we can have big documentary-only follow-up to clarify latter.

lightning/src/chain/mod.rs

ariard · 2020-08-21T17:52:30Z

lightning/src/ln/channelmonitor.rs

@@ -149,8 +155,8 @@ pub struct HTLCUpdate {
 }
 impl_writeable!(HTLCUpdate, 0, { payment_hash, payment_preimage, source });

-/// A simple implementation of a ManyChannelMonitor and ChainListener. Can be used to create a
-/// watchtower or watch our own channels.
+/// A simple implementation of a [`chain::Watch`]. Can be used to create a watchtower or to watch


FYI, @jkczyz we remove the watchtower mode because since then I figured out we can likely embed the watchtower mode behind and thus have multiple-mode of watchtower like WatchtowerFullProtocol or WatchtowerJusticeOnly. Or even experiment with different model for the pre-signed watchtower transactions.

It needs further refactor, see #606

ariard

Thanks Jeff to tackle this, I think that's one of the most needed refactor. Only did a first parse for now and catching up with context.

Seeing #649 (comment), I agree we should re-organize better our crates/files tree. In my opinion we have at least 3-layers impacted by this PR:

offchain channel processing, namely channelmanager.rs, channel.rs, responsible to open/update/close channel based on interacting with peers
chain processing, namely channelmonitor.rs, onchaint.rs in charge of processing chain data structures based on control message from the offchain channel processing layer
chain, namely chain.rs everything chain data struct related like Block, Utxo, Feerate, P2PNetwork, it should be mostly accessors abstracting out to any provider (like your node fee estimator or a API one, block view from an Electrum server or your full-node, ...)

This distinction doesn't cover keyinterface.rs which IMO should go in its own keys/ as cryptographic material is something cross-layer and post-anchor impl we may have a wallet/ to access wallet utxos for CPFP bump

See also comment illustrating further my point.

ariard · 2020-08-31T01:10:04Z

@jkczyz

See this branch for fixing tests and commit message for laying out the reason. To resume, this refactoring doesn't introduce a bug, but ccbc596 by switching utilities test didn't preserve the step by step monitoring scanning of package of transactions, thus triggering OnchainTxHandler aggregation logic more aggressively and diminishing the number of broadcast.

jkczyz · 2020-08-31T02:11:38Z

@jkczyz

See this branch for fixing tests and commit message for laying out the reason. To resume, this refactoring doesn't introduce a bug, but ccbc596 by switching utilities test didn't preserve the step by step monitoring scanning of package of transactions, thus triggering OnchainTxHandler aggregation logic more aggressively and diminishing the number of broadcast.

Hmm... so I managed to fix this yesterday but without modifying the tests. I found two problems:

I wasn't filtering transactions in ChannelMonitor
The outputs_to_watch did not include the funding txo.

Also, when debugging I noticed that the newly broadcasted transactions did not look valid. They seemed to double-spend some inputs. But with the above fixes this was no longer a problem.

Will push my changes for you to review after resolving the outstanding comments.

jkczyz

Rebased and addressed most comments.

FYI, 58d3411 and b7c308b are the relevant commits that fixed the failing tests.

lightning/src/chain/channelmonitor.rs

jkczyz · 2020-08-31T20:41:55Z

lightning/src/chain/mod.rs

+	/// is unknown.
+	///
+	/// [`short_channel_id`]: https://github.com/lightningnetwork/lightning-rfc/blob/master/07-routing-gossip.md#definition-of-short_channel_id
+	fn get_utxo(&self, genesis_hash: &BlockHash, short_channel_id: u64) -> Result<TxOut, AccessError>;


Agreed. Do you mind if I leave this to a follow-up PR? I'd like to limit the scope of this PR as much as possible.

jkczyz · 2020-08-31T21:13:37Z

lightning/src/chain/mod.rs

+/// blocks are connected and disconnected.
+///
+/// Each channel is associated with a [`ChannelMonitor`]. Implementations are responsible for
+/// maintaining a set of monitors and updating them accordingly as channel state changes and HTLCs


Those requirements are for ChannelMonitor, though. Right? These docs are simply stating that implementors must ensure that the monitors are updated (1) as channel state changes (i.e., wherever ChannelManager calls update_channel) and (2) HTLCs are resolved on chain (i.e., as blocks are connected).

I've re-worded it slightly to convey that implementors must maintain a set of monitors such that they can be updated as is stated. Essentially, I'm trying to convey that someone implementing chain::Watch must allow for calling update_monitor, block_connected, and block_disconnected on each ChannelMonitor. And I'm saying as much specifically in the docs for watch_channel and update_channel.

lightning/src/chain/mod.rs

jkczyz · 2020-08-31T21:49:26Z

lightning/src/chain/channelmonitor.rs

-/// watchtower or watch our own channels.
-///
-/// Note that you must provide your own key by which to refer to channels.
+/// An implementation of [`chain::Watch`].


Agree that implementations should be separated out from the interface that they are implementing. Would prefer a less verbose naming scheme though. I don't think it's necessary to have interface and impl in module names. Let's punt this to follow-up PRs as needed.

jkczyz · 2020-08-31T21:55:54Z

lightning/src/chain/channelmonitor.rs

 			Ok(_) => Ok(()),
 			Err(_) => Err(ChannelMonitorUpdateErr::PermanentFailure),
 		}
 	}

-	fn update_monitor(&self, funding_txo: OutPoint, update: ChannelMonitorUpdate) -> Result<(), ChannelMonitorUpdateErr> {
-		match self.update_monitor_by_key(funding_txo, update) {
+	fn update_channel(&self, funding_txo: OutPoint, update: ChannelMonitorUpdate) -> Result<(), ChannelMonitorUpdateErr> {


I'm not sure I'm able to parse your comment correctly.

It let think that ChannelMonitor is responsible for driving change of the off-chain channel.

What do you mean here? Might be a word missing or typo that's preventing me from understanding the comment.

TheBlueMatt

Sorry this took so long to get to. Mostly a few high-level questions still but I really love where this ended up for the most part.

TheBlueMatt · 2020-09-04T16:14:17Z

lightning/src/chain/mod.rs

+}
+
+/// An event indicating on-chain activity to watch for pertaining to a channel.
+pub enum WatchEvent {


I find it pretty confusing that this is added midway through and removed by the end. If its not too much trouble, it'd be great to get the removals squashed down so that this doesn't appear.

Likely will be very difficult to do by squashing as it will make the resulting commit not very atomic. Will see if I can move some of the commits around but I have a feeling it will be ugly.

Would it be any better if I moved the trait and enum to channelmonitor.rs?

fuzz/src/router.rs

lightning/src/chain/mod.rs

lightning/src/chain/channelmonitor.rs

lightning/src/chain/mod.rs

The remaining use of ChainWatchInterface was replaced by chain::Access in the previous commit, and thus it is no longer needed.

Rename ManyChannelMonitor to chain::Watch and move to chain/mod.rs, where chain-related interfaces live. Update the documentation for clarity and to conform to rustdoc formatting.

ManyChannelMonitor was renamed chain::Watch in the previous commit. Use a more concise name for an implementation that monitors the chain for channel activity. Future work will parameterize the struct to allow for different varieties of persistence. Thus, users usually will be able to use ChainMonitor directly rather than implementing a chain::Watch that wraps it.

ChainMonitor's template Key parameter was meant to allow supporting both local monitoring, where Key=OutPoint, and watchtowers, where Key= (PublicKey, u32). Use OutPoint directly since the watchtower case will not be supported this way.

BlockNotifier is a convenience for handing blocks to listeners. However, it requires that each listener conforms to the ChainListener interface. Additionally, there are only two listeners, ChannelManager and ChainMonitor, the latter of which may not be used when monitoring channels remotely. Remove BlockNotifier since it doesn't provide much value and constrains each listener to a specific interface.

BlockNotifier was removed in the previous commit, thus ChainListener is no longer needed. Instead, anything needing chain events should be notified directly.

WatchEventProvider served as a means for replacing ChainWatchInterface. However, it requires users to explicitly fetch WatchEvents, even if not interested in them. Replace WatchEventProvider by chain::Filter, which is an optional member of ChainMonitor. If set, interesting transactions and output spends are registered such that blocks containing them can be retrieved from a chain source in an efficient manner. This is useful when the chain source is not a full node. For Electrum, it allows for pre-filtered blocks. For BIP157/158, it serves as a means to match against compact filters.

Arrows should signify "calls" or "generates" unless noted.

The funding TXO was never added to ChannelMonitor's outputs_to_watch in 73dce20. Include it when constructing a ChannelMonitor.

Outputs to watch are tracked by ChannelMonitor as of 73dce20. Instead of determining new outputs to watch independently using ChainWatchedUtil, do so by comparing against outputs already tracked. Thus, ChainWatchedUtil and WatchEvent are no longer needed.

Transaction data from a block may be filtered before it is passed to block_connected functions, which may need the index of each transaction within the block. Rather than define each function in terms of a slice of tuples, define a type alias for the slice where it can be documented.

Given the chain::Watch interface is defined in terms of ChannelMonitor and ChannelMonitorUpdateErr, move channelmonitor.rs from the ln module to the chain module.

… 2020-06-refactor-chain-listener

jkczyz · 2020-10-01T17:08:48Z

Squashed. Also merged a separate branch for splitting channelmonitor.rs as it made it possible to preserve commit history across both files. Let me know if this is a problem.

ariard

Code Review ACK 6cd6816

I've left comments on how we may improve interfaces in the future but for now I think this is okay. I feel we won't agree on the error/async until we start playing with samples.

ariard · 2020-10-01T17:42:15Z

lightning/src/chain/mod.rs

+/// receiving full blocks from a chain source, any further filtering is unnecessary.
+///
+/// After an output has been registered, subsequent block retrievals from the chain source must not
+/// exclude any transactions matching the new criteria nor any in-block descendants of such


I'm quite sure that "any in-block descendants of such transactions" is subclassed under "transactions matching the new criteria" so it sounds redundant to me. And we may no be interested by descendants of a matched transaction when we don't have to react anymore. E.g we don't care about spends of justice transactions.

ariard · 2020-10-01T18:02:16Z

lightning/src/chain/mod.rs

+/// Note that use as part of a [`Watch`] implementation involves reentrancy. Therefore, the `Filter`
+/// should not block on I/O. Implementations should instead queue the newly monitored data to be
+/// processed later. Then, in order to block until the data has been processed, any `Watch`
+/// invocation that has called the `Filter` must return [`TemporaryFailure`].


I think TemporaryFailure convey more an asynchronous event rather than an error. Off-chain logic should be able to move forward the state of other channels than the one being updated onchain. When chain::Filter is okay it should signal back to chain::Watch we should signal readyness of channel to its user.

We should restrain failure to mean the-infrastructure-is-burning, please stop everything. Or we loose network connection with chain source, please wait.

In review of the final doc changes in lightningdevkit#649, I noticed there appeared to be redundant monitored-outpoints function in `ChannelMonitor` - `get_monitored_outpoints()` and `get_outputs_to_watch()`. In 6f08779, get_monitored_outpoints() was added, with its behavior largely the same as today's - only returning the set of remote commitment txn outputs that we've learned about on-chain. This is clearly not sufficient, and in 73dce20, `get_outputs_to_watch` was added which was overly cautious to ensure nothing was missed. Still, the author of 73dce20 (me) seemed entirely unaware of the work in 6f08779 (also me), despite the function being the literal next function in the same file. This is presumably because it was assumed that `get_monitored_outpoints` referred to oupoints for which we should monitor for spends of (which is true), while `get_outputs_to_watch` referred to outpouts which we should monitor for the transaction containing said output (which is not true), or something of that nature. Specifically, it is the expected behavior that the only time we care about `Filter::register_tx` is for the funding transaction (which we aren't aware of the inputs of), but for all other transactions we register interest on the basis of an outpoint in the previous transaction (ie via `Filter::register_output`). Here we drop the broken-on-day-one `get_monitored_outpoints()` version, but assert in testing that the values which it would return are all present in `get_outputs_to_watch()`.

Previously, we had a concept of "rescaning" blocks when we detected a need to monitor for a new set of outputs in future blocks while connecting a block. In such cases, we'd need to possibly learn about these new spends later in the *same block*, requiring clients who filter blocks to get a newly-filtered copy of the same block. While redoing the chain access API, it became increasingly clear this was an overly complicated API feature, and it seems likely most clients will not use it anyway. Further, any client who *does* filter blocks can simply update their filtering algorithm to include any descendants of matched transactions in the filter results, avoiding the need for rescan support entirely. Thus, it was decided that we'd move forward without rescan support in lightningdevkit#649, however to avoid significant further changes in the already-large 649, we decided to fully remove support in a follow-up. Here, we remove the API features that existed for rescan and fix the few tests to not rely on it. After this commit, we now only ever have one possible version of block connection transactions, making it possible to be significantly more confident in our test coverage actually capturing all realistic scenarios.

In review of the final doc changes in lightningdevkit#649, I noticed there appeared to be redundant monitored-outpoints function in `ChannelMonitor` - `get_monitored_outpoints()` and `get_outputs_to_watch()`. In 6f08779, get_monitored_outpoints() was added, with its behavior largely the same as today's - only returning the set of remote commitment txn outputs that we've learned about on-chain. This is clearly not sufficient, and in 73dce20, `get_outputs_to_watch` was added which was overly cautious to ensure nothing was missed. Still, the author of 73dce20 (me) seemed entirely unaware of the work in 6f08779 (also me), despite the function being the literal next function in the same file. This is presumably because it was assumed that `get_monitored_outpoints` referred to oupoints for which we should monitor for spends of (which is true), while `get_outputs_to_watch` referred to outpouts which we should monitor for the transaction containing said output (which is not true), or something of that nature. Specifically, it is the expected behavior that the only time we care about `Filter::register_tx` is for the funding transaction (which we aren't aware of the inputs of), but for all other transactions we register interest on the basis of an outpoint in the previous transaction (ie via `Filter::register_output`). Here we drop the broken-on-day-one `get_monitored_outpoints()` version, but assert in testing that the values which it would return are all present in `get_outputs_to_watch()`.

Bindings Updates for #649

jkczyz force-pushed the 2020-06-refactor-chain-listener branch from bed64c6 to 49b8c58 Compare June 25, 2020 00:29

jkczyz mentioned this pull request Jun 25, 2020

Add subcrate which impls a simple SPV client from Bitcoin Core RPC #614

Closed

jkczyz force-pushed the 2020-06-refactor-chain-listener branch from 49b8c58 to 07779e0 Compare June 26, 2020 18:37

valentinewallace self-requested a review June 26, 2020 23:36

TheBlueMatt reviewed Jun 27, 2020

View reviewed changes

valentinewallace reviewed Jun 29, 2020

View reviewed changes

jkczyz force-pushed the 2020-06-refactor-chain-listener branch from 07779e0 to 2ca65a4 Compare June 29, 2020 23:44

jkczyz force-pushed the 2020-06-refactor-chain-listener branch 2 times, most recently from a9564e1 to 472ecc9 Compare August 4, 2020 02:05

jkczyz force-pushed the 2020-06-refactor-chain-listener branch from 472ecc9 to 478c481 Compare August 5, 2020 00:12

TheBlueMatt reviewed Aug 6, 2020

View reviewed changes

jkczyz force-pushed the 2020-06-refactor-chain-listener branch 2 times, most recently from d50eef3 to 5acef01 Compare August 9, 2020 19:24

ariard reviewed Aug 21, 2020

View reviewed changes

jkczyz force-pushed the 2020-06-refactor-chain-listener branch from 5acef01 to b7c308b Compare September 1, 2020 02:10

jkczyz commented Sep 1, 2020

View reviewed changes

valentinewallace mentioned this pull request Sep 2, 2020

Add ChannelDataPersister trait and point ChainMonitor to it. #681

Merged

5 tasks

TheBlueMatt reviewed Sep 7, 2020

View reviewed changes

jkczyz added 16 commits September 30, 2020 22:39

Remove ChainWatchInterface

bc4f463

The remaining use of ChainWatchInterface was replaced by chain::Access in the previous commit, and thus it is no longer needed.

Replace ManyChannelMonitor with chain::Watch

801b775

Rename ManyChannelMonitor to chain::Watch and move to chain/mod.rs, where chain-related interfaces live. Update the documentation for clarity and to conform to rustdoc formatting.

Remove Key parameter from ChainMonitor

851283d

ChainMonitor's template Key parameter was meant to allow supporting both local monitoring, where Key=OutPoint, and watchtowers, where Key= (PublicKey, u32). Use OutPoint directly since the watchtower case will not be supported this way.

Remove ChainListener

1599a13

BlockNotifier was removed in the previous commit, thus ChainListener is no longer needed. Instead, anything needing chain events should be notified directly.

Fix architecture diagram line length

e83dcda

Fix architecture diagram arrow directions

95b7eee

Arrows should signify "calls" or "generates" unless noted.

Include funding TXO in outputs to watch

9e14256

The funding TXO was never added to ChannelMonitor's outputs_to_watch in 73dce20. Include it when constructing a ChannelMonitor.

Merge ChainMonitor impl blocks

e09767b

Move channelmonitor.rs from ln to chain module

819a865

Given the chain::Watch interface is defined in terms of ChannelMonitor and ChannelMonitorUpdateErr, move channelmonitor.rs from the ln module to the chain module.

Move ln/channelmonitor.rs to chain/chainmonitor.rs

51a5a1a

Merge branch '2020-06-refactor-chain-listener-move-chainmonitor' into…

6cd6816

… 2020-06-refactor-chain-listener

jkczyz force-pushed the 2020-06-refactor-chain-listener branch from ec582dc to 6cd6816 Compare October 1, 2020 17:03

jkczyz changed the title ~~Remove ChainWatchInterface from BlockNotifier~~ Refactor chain monitoring Oct 1, 2020

ariard reviewed Oct 1, 2020

View reviewed changes

TheBlueMatt merged commit 8fb4a3d into lightningdevkit:master Oct 1, 2020

TheBlueMatt mentioned this pull request Oct 2, 2020

Drop last bits of rescan as its too complicated to be worth having #730

Merged

jkczyz added a commit to jkczyz/rust-lightning that referenced this pull request Oct 19, 2020

f - Changes needed for rebase on lightningdevkit#649

d65b12b

jkczyz added a commit to jkczyz/rust-lightning that referenced this pull request Oct 19, 2020

f - Changes needed for rebase on lightningdevkit#649

71b49e3

TheBlueMatt added a commit that referenced this pull request Nov 12, 2020

Merge pull request #721 from TheBlueMatt/2020-09-649-bindings

23a1d7a

Bindings Updates for #649

jkczyz mentioned this pull request Dec 1, 2020

Lightning block sync and sample block sources #763

Closed

Refactor chain monitoring #649

Refactor chain monitoring #649

Conversation

jkczyz commented Jun 23, 2020 • edited Loading

TheBlueMatt commented Jun 23, 2020

jkczyz commented Jun 23, 2020

TheBlueMatt commented Jun 23, 2020 via email

jkczyz commented Jun 25, 2020

TheBlueMatt commented Jun 25, 2020 • edited Loading

jkczyz commented Jun 26, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz commented Jul 21, 2020

jkczyz commented Aug 4, 2020

codecov bot commented Aug 5, 2020 • edited Loading

Codecov Report

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz Aug 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard Aug 21, 2020 • edited by jkczyz Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ariard left a comment

Choose a reason for hiding this comment

ariard commented Aug 31, 2020

jkczyz commented Aug 31, 2020 • edited Loading

jkczyz left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheBlueMatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz commented Oct 1, 2020

ariard left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkczyz commented Jun 23, 2020 •

edited

Loading

TheBlueMatt commented Jun 25, 2020 •

edited

Loading

codecov bot commented Aug 5, 2020 •

edited

Loading

jkczyz Aug 9, 2020 •

edited

Loading

ariard Aug 21, 2020 •

edited by jkczyz

Loading

jkczyz commented Aug 31, 2020 •

edited

Loading

jkczyz left a comment •

edited

Loading