Define chain::Confirm trait for use by Electrum clients #889

jkczyz · 2021-04-20T17:39:36Z

Define a separate trait akin to chain::Listen for notifying when transactions have been confirmed on chain or unconfirmed during a chain reorganization. Whereas chain::Listen is used for block-oriented chain sources, chain::Confirm is used for chain sources supplying data for activity related to transactions and outputs registered via chain::Filter.

codecov · 2021-04-20T17:56:49Z

Codecov Report

Merging #889 (99e2283) into main (feeb893) will increase coverage by 0.07%.
The diff coverage is 95.34%.

@@            Coverage Diff             @@
##             main     #889      +/-   ##
==========================================
+ Coverage   90.26%   90.33%   +0.07%     
==========================================
  Files          55       58       +3     
  Lines       29088    29936     +848     
==========================================
+ Hits        26255    27042     +787     
- Misses       2833     2894      +61

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	`100.00% <ø> (ø)`
lightning/src/ln/reorg_tests.rs	`99.62% <ø> (ø)`
lightning/src/ln/functional_test_utils.rs	`95.08% <83.33%> (ø)`
lightning/src/ln/channelmanager.rs	`83.11% <94.73%> (ø)`
lightning/src/chain/chainmonitor.rs	`95.52% <100.00%> (+0.70%)`	⬆️
lightning/src/chain/channelmonitor.rs	`95.45% <100.00%> (ø)`
lightning/src/ln/chanmon_update_fail_tests.rs	`97.63% <100.00%> (ø)`
lightning/src/ln/channel.rs	`87.24% <100.00%> (ø)`
lightning/src/ln/functional_tests.rs	`96.85% <100.00%> (-0.21%)`	⬇️
... and 7 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 feeb893...99e2283. Read the comment docs.

TheBlueMatt

Trait name seems fine to me, but some comments on the docs.

TheBlueMatt · 2021-04-20T19:20:49Z

lightning/src/chain/mod.rs

+/// The `Confirm` trait is used to notify when transactions have been confirmed on chain or
+/// unconfirmed during a chain reorganization.
+///
+/// Clients should use this interface instead of [`Listen`] if they receive transaction data only


I think the distinction as to when to use this API vs Listen is more about if you get chain data in a transaction-oriented API vs a blockchain-oriented API - ie do you receive notifications of each new header with relevant transaction data, or do you receive simply notifications of transaction-level state changes, with header data separated out.

Reworded. Though note that while header transaction is separated out, in practice it still needs to be fetched (either explicitly or cached from a notification) in order to pass it along to transactions_confirmed calls.

Right. I do kinda agree with you that "transaction-oriented" isn't really clear for a user, but I'm not sure what is better, and I do think it captures exactly what we mean. I wonder if adding more words to describe it would help or if that just makes it more confusing. Maybe it can be resolved by just mentioning "Clients which fetch the full header chain" in the docs for Listen?

Good idea -- expanded Listen docs to mention the full header chain. That said, it seems the only difference between the two APIs in this regard is that Listen requires the full header chain whereas Confirm is simply ok with skipping headers . Unless we want to further relax how Confirm docs describe when to call best_block_updated. Currently, it just says "whenever a new chain tip becomes available".

As long as we don't expand the docs around best_block_updated to include the reorg case, the reorg handling is a pretty big difference, too, I think. #889 (comment)

TheBlueMatt · 2021-04-20T19:21:33Z

lightning/src/chain/mod.rs

+/// [`Filter`] by subscribing to activity related to registered transactions and outputs. Upon
+/// notification, it would pass along the relevant transactions using this interface.
+///
+/// The intended use is as follows:


This is a bit confusing as it encompasses both actions thee implementor of this trait is taking, as well as actions the user of it is taking. Maybe the tense can be rejiggered to better describe who is doing what.

Dropped the line about get_relevant_txids since it is covered later and its implementation details are covered in its own docs.

TheBlueMatt · 2021-04-20T19:23:29Z

lightning/src/chain/mod.rs

+	/// Should be called for any transactions registered by [`Filter::register_tx`] or any
+	/// transactions spending an output registered by [`Filter::register_output`]. Such transactions
+	/// appearing in the same block do not need to be included in the same call; instead, multiple
+	/// calls with additional transactions may be made.


However, the calls MUST be in chain order, or at least in dependency order.

Done and linked to pertinent section on ordering in the trait docs.

TheBlueMatt · 2021-04-20T19:26:34Z

lightning/src/chain/mod.rs

+	/// calls with additional transactions may be made.
+	///
+	/// May be called before or after [`best_block_updated`] for the corresponding block. However,
+	/// it must not be called after [`best_block_updated`] if `header` corresponds to a block that


This is confusing - it only needs to be ordered with respect to a best_block_updated call with a new header - ie we cannot be called with a header which is no longer in the main chain if we already called transactions_confirmed or best_block_updated with a later header.

Re-worded. Let me know if that captures the reorg case. I also linked earlier to trait-level documentation on ordering.

TheBlueMatt · 2021-04-20T19:28:33Z

lightning/src/chain/mod.rs

+	/// Should be called when a new header is available but may be skipped for intermediary blocks
+	/// if they become available at the same time.
+	///
+	/// Implementations are not required to handle chain reorganizations unless `height` is not


Hmm, I'm not really sure what the point of this comment is - I can't really figure out what this should mean to me as a user who wishes to call this method on an implementation of this trait?

This paragraph is meant for the implementer not the caller. But I can't exactly recall where we landed on this. Definitely needs to rewritten regardless.

IIRC, this is an implementation detail that may be leaking into the interface. Question is should I just drop this? Or is there something we need to convey about how implementations handle <= heights.

Right, I guess its a question of if we want to support both modes - I think right now you can call either transaction_unconfirmed or you can skip it as long as you call best_block_updated with reorg info. I don't think it really makes sense for clients to do that (if they're fetching the full header chain they should be using Listen, eventually), but technically we could support it. If we don't want to support it, I dunno if its worth mentioning in the docs here, but if you have any other reasoning based on your electrum exploration we can.

Dropped the paragraph.

TheBlueMatt · 2021-04-20T19:29:40Z

lightning/src/chain/mod.rs

+///   has been reorganized out of the chain.
+/// - Call [`best_block_updated`] whenever a new chain tip becomes available.
+///
+/// See individual method documentation for further details.


I feel like we need a general "things have to be called in chain order" call-out. I don't think we can or should reasonably handle things out-of-order, and callers of these interface methods need to take special care that they don't call things out of order.

Added the requirements as discussed. Let me know if they are too strict.

arik-so · 2021-04-20T19:42:26Z

lightning/src/chain/mod.rs

+/// - Call [`transactions_confirmed`] for any on-chain activity of interest.
+/// - Return such transactions from [`get_relevant_txids`] until they have enough confirmations to
+///   no longer be in danger of chain reorganization.
+/// - Call [`transaction_unconfirmed`] for any transaction returned by [`get_relevant_txids`] that


If a transaction was confirmed, but was later reorged into a different/later block height, should transaction_confirmed be called again, or should transaction_unconfirmed be called first?

Here, transaction_unconfirmed would need to be called first. I think this is addressed in the new "Order" section. Let me know if it doesn't clear things up, though, or if you have recommendations on how to better word it.

ariard

Overall, I think it's moving in the good direction

ariard · 2021-04-20T23:45:30Z

lightning/src/chain/mod.rs

+/// Clients should use this interface instead of [`Listen`] if they receive transaction data only
+/// relevant to that identified by [`Filter`]. For instance, an Electrum client may implement
+/// [`Filter`] by subscribing to activity related to registered transactions and outputs. Upon
+/// notification, it would pass along the relevant transactions using this interface.


I think it would be a slight improvement here to dub "Client" as "LDK Client" or "Monitor Client" as a stark difference from Electrum client. In the last sentence, I would replace it by Electrum client.

Both occurrences are meant to refer to the same entity. The unqualified one refers to a client that may be an Electrum client whereas the qualified one is an example of the first. Re-worded to make it clear that it is a client of a chain source.

ariard · 2021-04-21T00:04:42Z

lightning/src/chain/mod.rs

+	/// handled by [`transaction_unconfirmed`].
+	///
+	/// [`transaction_unconfirmed`]: Self::transaction_unconfirmed
+	fn best_block_updated(&self, header: &BlockHeader, height: u32);


Is a user require to call best_block_updated if any transaction has been transaction_confirmed for the associated height and hash ? I believe If it's a MUST just say it.

I don't believe it is necessary as they are allowed to skip intermediary headers even for those where transaction_confirmed is called, IIUC.

If it's not necessary can you say it explicitly in either transaction_confirmed or best_block_updated ? It wasn't obvious to me :)

Added here.

Hmm, its easy to confuse the new wording for "if you passed a header in via transactions_confirmed, don't bother passing it in here, too" which I don't think is what we want. Instead we just mean "if you pass a header in via transactions_confirmed but by the time you go to call best_block_updated the best block is different, just call this with the latest one", which is pretty different.

Alright, I'm just going to drop it to avoid confusion. Let me know if the docs around use of best_block_updated can be clarified in any other way. It's currently mentioned in transactions_confirmed and in the trait-level docs.

ariard · 2021-04-21T00:07:32Z

lightning/src/chain/mod.rs

+	/// Returns transactions that should be monitored for reorganization out of the chain.
+	///
+	/// Should include any transactions passed to [`transactions_confirmed`] that have insufficient
+	/// confirmations to be safe from a chain reorganization. Should not include any transactions


Should it precise that "insufficient confirmations" is a matter of user client's policy ? Though ANTI_REORG_DELAY isn't configurable for now. A user might be willingly to increase is more funds are at stake or lower it if less funds are at stake.

Lowering increase off-chain liquidity velocity as a node can immediately cancel back HTLC, thus freeing a HTLC slot.

Right, not yet configurable. Also, it's general because it's talking about how this may be implemented. For example, ChannelManager always returns the funding transactions no matter how many confirmations.

jkczyz

Please take a look at the "Use" and "Order" sections added to the docs. Any feedback on how to better arrange this would be more than welcome.

jkczyz · 2021-04-21T02:44:36Z

lightning/src/chain/mod.rs

+/// The `Confirm` trait is used to notify when transactions have been confirmed on chain or
+/// unconfirmed during a chain reorganization.
+///
+/// Clients should use this interface instead of [`Listen`] if they receive transaction data only


Reworded. Though note that while header transaction is separated out, in practice it still needs to be fetched (either explicitly or cached from a notification) in order to pass it along to transactions_confirmed calls.

jkczyz · 2021-04-21T02:51:37Z

lightning/src/chain/mod.rs

+/// Clients should use this interface instead of [`Listen`] if they receive transaction data only
+/// relevant to that identified by [`Filter`]. For instance, an Electrum client may implement
+/// [`Filter`] by subscribing to activity related to registered transactions and outputs. Upon
+/// notification, it would pass along the relevant transactions using this interface.


Both occurrences are meant to refer to the same entity. The unqualified one refers to a client that may be an Electrum client whereas the qualified one is an example of the first. Re-worded to make it clear that it is a client of a chain source.

jkczyz · 2021-04-21T02:58:29Z

lightning/src/chain/mod.rs

+/// [`Filter`] by subscribing to activity related to registered transactions and outputs. Upon
+/// notification, it would pass along the relevant transactions using this interface.
+///
+/// The intended use is as follows:


Dropped the line about get_relevant_txids since it is covered later and its implementation details are covered in its own docs.

jkczyz · 2021-04-21T03:16:31Z

lightning/src/chain/mod.rs

+///   has been reorganized out of the chain.
+/// - Call [`best_block_updated`] whenever a new chain tip becomes available.
+///
+/// See individual method documentation for further details.


Added the requirements as discussed. Let me know if they are too strict.

jkczyz · 2021-04-21T03:31:09Z

lightning/src/chain/mod.rs

+	/// Should be called for any transactions registered by [`Filter::register_tx`] or any
+	/// transactions spending an output registered by [`Filter::register_output`]. Such transactions
+	/// appearing in the same block do not need to be included in the same call; instead, multiple
+	/// calls with additional transactions may be made.


Done and linked to pertinent section on ordering in the trait docs.

jkczyz · 2021-04-21T06:49:06Z

lightning/src/chain/mod.rs

+	/// calls with additional transactions may be made.
+	///
+	/// May be called before or after [`best_block_updated`] for the corresponding block. However,
+	/// it must not be called after [`best_block_updated`] if `header` corresponds to a block that


Re-worded. Let me know if that captures the reorg case. I also linked earlier to trait-level documentation on ordering.

jkczyz · 2021-04-21T06:50:17Z

lightning/src/chain/mod.rs

+	/// handled by [`transaction_unconfirmed`].
+	///
+	/// [`transaction_unconfirmed`]: Self::transaction_unconfirmed
+	fn best_block_updated(&self, header: &BlockHeader, height: u32);


I don't believe it is necessary as they are allowed to skip intermediary headers even for those where transaction_confirmed is called, IIUC.

jkczyz · 2021-04-21T06:58:21Z

lightning/src/chain/mod.rs

+	/// Should be called when a new header is available but may be skipped for intermediary blocks
+	/// if they become available at the same time.
+	///
+	/// Implementations are not required to handle chain reorganizations unless `height` is not


This paragraph is meant for the implementer not the caller. But I can't exactly recall where we landed on this. Definitely needs to rewritten regardless.

IIRC, this is an implementation detail that may be leaking into the interface. Question is should I just drop this? Or is there something we need to convey about how implementations handle <= heights.

jkczyz · 2021-04-21T07:00:37Z

lightning/src/chain/mod.rs

+	/// Returns transactions that should be monitored for reorganization out of the chain.
+	///
+	/// Should include any transactions passed to [`transactions_confirmed`] that have insufficient
+	/// confirmations to be safe from a chain reorganization. Should not include any transactions


Right, not yet configurable. Also, it's general because it's talking about how this may be implemented. For example, ChannelManager always returns the funding transactions no matter how many confirmations.

jkczyz · 2021-04-21T07:04:01Z

lightning/src/chain/mod.rs

+/// - Call [`transactions_confirmed`] for any on-chain activity of interest.
+/// - Return such transactions from [`get_relevant_txids`] until they have enough confirmations to
+///   no longer be in danger of chain reorganization.
+/// - Call [`transaction_unconfirmed`] for any transaction returned by [`get_relevant_txids`] that


Here, transaction_unconfirmed would need to be called first. I think this is addressed in the new "Order" section. Let me know if it doesn't clear things up, though, or if you have recommendations on how to better word it.

lightning/src/ln/channelmanager.rs

TheBlueMatt

Looks good mod one question.

lightning/src/chain/chainmonitor.rs

TheBlueMatt · 2021-04-22T03:19:57Z

lightning/src/chain/chainmonitor.rs

 	///
 	/// [`block_connected`]: Self::block_connected
-	/// [`update_best_block`]: Self::update_best_block
+	/// [`best_block_updated`]: Self::best_block_updated


Should we convert these into an impl chain::Confirm for ChainMonitor?

Yeah, this likely would be needed when implementing a sync utility. Added a separate block with Send and Sync restrictions akin to Listen. Let me know if I'm misunderstanding when this is needed.

Could also move the implementations directly into the added definitions if you'd like. It's unclear to me why they are split for Listen.

Added a separate block with Send and Sync restrictions akin to Listen. Let me know if I'm misunderstanding when this is needed.

Hmm, frankly not sure why those are there, I assume we used to need them for something, but now don't? Either way, it seems to compile without them, so no reason to have extra bounds on it - same goes for the chain::Listen impl as well now.

Also, I think if the traits are implemented we should drop the public functions which are duplicative. block_connected is only there because its slightly different from the chain::Listen block_connected, IIRC.

Yeah, sounds good. Note that this requires having full_stack uses chain::Confirm for ChainMonitor otherwise it would be a much larger change. FWIW, it is already doing so for ChannelManager.

arik-so

Looks good to me, too!

Define a separate trait akin to chain::Listen for notifying when transactions have been confirmed on chain or unconfirmed during a chain reorganization. Whereas chain::Listen is used for block-oriented chain sources, chain::Confirm is used for chain sources supplying data for activity related to transactions and outputs registered via chain::Filter.

jkczyz · 2021-04-22T21:19:42Z

Squashed fixup commits

TheBlueMatt reviewed Apr 20, 2021

View reviewed changes

arik-so reviewed Apr 20, 2021

View reviewed changes

ariard reviewed Apr 21, 2021

View reviewed changes

jkczyz commented Apr 21, 2021

View reviewed changes

TheBlueMatt reviewed Apr 21, 2021

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

jkczyz force-pushed the 2021-04-electrum-trait branch from c0fc420 to 81f093f Compare April 21, 2021 18:06

TheBlueMatt reviewed Apr 21, 2021

View reviewed changes

lightning/src/ln/channelmanager.rs Outdated Show resolved Hide resolved

TheBlueMatt added this to the 0.0.14 milestone Apr 21, 2021

TheBlueMatt approved these changes Apr 22, 2021

View reviewed changes

jkczyz force-pushed the 2021-04-electrum-trait branch 2 times, most recently from 3f1d727 to 7f35903 Compare April 22, 2021 17:30

arik-so approved these changes Apr 22, 2021

View reviewed changes

jkczyz added 4 commits April 22, 2021 14:17

Implement chain::Confirm for relevant structs

23c4c8b

Implement chain::Confirm for ChainMonitor

93d20ff

Drop pub functions for ChainMonitor's Listen impl

99e2283

jkczyz force-pushed the 2021-04-electrum-trait branch from 7f35903 to 99e2283 Compare April 22, 2021 21:19

TheBlueMatt approved these changes Apr 23, 2021

View reviewed changes

TheBlueMatt merged commit 0d75a63 into lightningdevkit:main Apr 23, 2021

Define chain::Confirm trait for use by Electrum clients #889

Define chain::Confirm trait for use by Electrum clients #889

Uh oh!

Conversation

jkczyz commented Apr 20, 2021

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

TheBlueMatt left a comment

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!

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!

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!

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!

ariard left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

jkczyz left a comment

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

codecov bot commented Apr 20, 2021 •

edited

Loading