Minimize reads to counterparty_claimable_outpoints #3057

G8XSU · 2024-05-09T16:57:54Z

Minimize current set of non-current/previous_commitment reads to counterparty_claimable_outpoints by re-using existing get calls and refactors

As part of #3049

lightning/src/chain/channelmonitor.rs

codecov-commenter · 2024-05-14T07:48:39Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.76%. Comparing base (1890e80) to head (1690c66).
Report is 45 commits behind head on main.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3057      +/-   ##
==========================================
- Coverage   89.82%   89.76%   -0.07%     
==========================================
  Files         116      116              
  Lines       96466    96466              
  Branches    96466    96466              
==========================================
- Hits        86655    86591      -64     
- Misses       7264     7311      +47     
- Partials     2547     2564      +17

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

G8XSU · 2024-05-15T21:29:26Z

Now this PR should be ready for review.
But even so, there might be further changes as i get into further tasks for #3049
So it is not ready for merging.

G8XSU · 2024-05-15T21:31:27Z

lightning/src/chain/channelmonitor.rs

 					let mut commitment_tx_to_counterparty_output = None;
-					if (tx.input[0].sequence.0 >> 8*3) as u8 == 0x80 && (tx.lock_time.to_consensus_u32() >> 8*3) as u8 == 0x20 {
+					let per_commitment_option = self.counterparty_claimable_outpoints.get(&tx.txid()).map(|x| x.iter()
+						.map(|&(ref a, ref b)| (a.clone(), b.clone())).collect::<Vec<_>>());


Had to clone here because calling self.check_spend_counterparty_transaction needs a mut ref to self and without clone self.counterparty_claimable_outpoints.get will need a immut ref to self.

Its not really a huge deal, but maybe that's an indication we're building the wrong API here - if we're passing a copy of some internal state to a method probably we should....not be doing that :). Instead, maybe we pass an override value (ie "i have this, dont look it up" and check_spend_counterparty_transaction can do a lookup if no override is provided.

removed per_commitment_option from check_spend signature

G8XSU · 2024-05-16T17:30:39Z

This PR is ready for review and merging.
Any further refactor changes, we can do in follow-up.

lightning/src/chain/channelmonitor.rs

TheBlueMatt · 2024-05-18T00:10:28Z

lightning/src/chain/channelmonitor.rs

-			($txid: expr, $commitment_tx: expr) => {
-				if let Some(ref latest_outpoints) = $self.counterparty_claimable_outpoints.get($txid) {
+			($txid: expr, $commitment_tx: expr, $per_commitment_outpoints: expr) => {
+				if let Some(ref latest_outpoints) = $per_commitment_outpoints {


While you're here, mind seeing if you can make this a function instead of a macro?

ok, i tried this.
Both check_htlc_fails and fail_unbroadcast_htlcs need mut refernce to self they push/delete from onchain_events_awaiting_threshold_conf.
and per_commitment_option uses a immut reference. so i don't think we can do it easily unless i use clone everywhere. (otherwise we somehow have to revamp onchain_events_awaiting_threshold_conf into return of fn or something.)

lightning/src/chain/channelmonitor.rs

TheBlueMatt · 2024-05-18T00:12:50Z

lightning/src/chain/channelmonitor.rs

 	/// transaction.
-	fn check_spend_counterparty_transaction<L: Deref>(&mut self, tx: &Transaction, height: u32, block_hash: &BlockHash, logger: &L)
-		-> (Vec<PackageTemplate>, TransactionOutputs, CommitmentTxCounterpartyOutputInfo)
+	fn check_spend_counterparty_transaction<L: Deref>(&mut self, tx: &Transaction, height: u32, block_hash: &BlockHash, per_commitment_option: Option<&Vec<(HTLCOutputInCommitment, Option<Box<HTLCSource>>)>>, logger: &L)


Do you want to go ahead and define a type for this? Doesn't really matter but might leave things cleaner in this PR.

i reverted this change to fn signature, since we are no longer re-using per_commitment_option b/w this and cancel_prev_claims.
We will need some more changes once we implement provide_claim_info, will see if need a new request type here then.

lightning/src/chain/channelmonitor.rs

TheBlueMatt · 2024-05-18T00:21:09Z

lightning/src/chain/channelmonitor.rs

 					let mut commitment_tx_to_counterparty_output = None;
-					if (tx.input[0].sequence.0 >> 8*3) as u8 == 0x80 && (tx.lock_time.to_consensus_u32() >> 8*3) as u8 == 0x20 {
+					let per_commitment_option = self.counterparty_claimable_outpoints.get(&tx.txid()).map(|x| x.iter()
+						.map(|&(ref a, ref b)| (a.clone(), b.clone())).collect::<Vec<_>>());


Its not really a huge deal, but maybe that's an indication we're building the wrong API here - if we're passing a copy of some internal state to a method probably we should....not be doing that :). Instead, maybe we pass an override value (ie "i have this, dont look it up" and check_spend_counterparty_transaction can do a lookup if no override is provided.

TheBlueMatt · 2024-05-18T00:22:04Z

lightning/src/chain/channelmonitor.rs

-				($counterparty_txid: expr, $htlc_output: expr) => {
-					if let Some(txid) = $counterparty_txid {
-						for &(ref pending_htlc, ref pending_source) in self.counterparty_claimable_outpoints.get(&txid).unwrap() {
+				($htlc_output: expr, $per_commitment_data: expr) => {


Same here, can this be an fn instead?

this one should be doable,
but it is getting messy really quick, as args/vars for check_htlc_valid_counterparty/log_claim/sca_commitment are intertwined.

I can do this is separate PR.

its current signature as it is extracted to function looks like

fn check_htlc_valid_counterparty<L: Deref>( &self, htlc_output: &HTLCOutputInCommitment, per_commitment_data: &[(HTLCOutputInCommitment, Option<Box<HTLCSource>>)], htlc_claim: &Option<HTLCClaim>, input: &TxIn, txid: &Txid, logger: &WithChannelMonitor<L>, ) -> Option<(HTLCSource, PaymentHash, u64)> where L::Target: Logger {

Part of #3076

tnull

One question, one nit, otherwise LGTM.

lightning/src/chain/channelmonitor.rs

TheBlueMatt · 2024-05-28T16:00:51Z

lightning/src/chain/channelmonitor.rs

 	/// data in counterparty_claimable_outpoints. Will directly claim any HTLC outputs which expire at a
 	/// height > height + CLTV_SHARED_CLAIM_BUFFER. In any case, will install monitoring for
-	/// HTLC-Success/HTLC-Timeout transactions.
+	/// HTLC-Success/HTLC-Timeout transactions. This function should only be called when claimable HTLC


I'm not really convinced pushing this on the caller makes sense either? Rather, ISTM it would make sense for this function to just be retryable - we call it whenever we think we have a transaction for it, it tries to use the transaction and if it finds it needs some per_commitment data that it doesn't have it just returns and expects to be called again later.

after everything related to 3049 is done,
we will call this function directly when tx is current/prev commitment tx.
otherwise we will call it once ClaimInfo is provided to us by the user.
So this function should always have per_commitment_data. This updated docs doesn't change anything, it was already working with such presumption.
why add the complexity which might never be used?

Then I'm quite confused what we're building to - in your first version of the patch you wanted to pass the the counterparty_claimable_outpoints lookup result to this method which resulted in a double-self-mut-reference, so I suggested that that is probably not the right API. This implies we shouldn't be doing the counterparty_claimable_outpoints lookup at the callsite at all (as it would then be duplicated in check_spend_counterparty_transaction), but rather that we should consider handling the "can I do something or do I need to ask for ClaimInfo" decision inside of check_spend_counterparty_transaction.

in your first version of the patch you wanted to pass the the counterparty_claimable_outpoints lookup result to this method

Yes earlier i was re-using counterparty_claimable_outpoints b/w check_spend and cancel_prev_commitment_claims, but i had to revert that change, i was using it incorrectly.

now, since that re-using is no longer required, check_spend can do a lookup internally. There won't be any duplicate lookup for counterparty_claimable_outpoints.

we will publish ClaimInfoRequest once and after receiving ClaimInfo, we will call check_spend and update_claims_view_from_requests

I can remove this comment if that unblock this PR and we can tackle this when we are actually going to use ClaimInfo.

now, since that re-using is no longer required, check_spend can do a lookup internally. There won't be any duplicate lookup for counterparty_claimable_outpoints. we will publish ClaimInfoRequest once and after receiving ClaimInfo, we will call check_spend and update_claims_view_from_requests

Okay, then I'm very confused why you added this comment here - it seems like we're on the same page that we can call check_spend_counterparty_transaction no matter if we have claim info available or not, and we'll simply let it fail in cases where counterparty_claimable_outpoints doesn't have anything and no ClaimInfo is available?

I can remove this comment if that unblock this PR and we can tackle this when we are actually going to use ClaimInfo.

That's fine, but I'd very much like to make sure we're on the same page in terms of how this is going to work.

but I'd very much like to make sure we're on the same page in terms of how this is going to work.

Yes i will sync with you offline.

I think you are right, i missed the part where we need to check if we should publish ClaimInfoRequest, we will have to access counterparty_claimable_outpoints there.

Removed the comment.

Reads related to current/previous commitment_tx should be explicit and reads related to older commitment_txs should be minimized.

TheBlueMatt · 2024-05-30T17:58:49Z

lightning/src/chain/channelmonitor.rs

 			if counterparty_commitment_txid == confirmed_commitment_txid {
 				continue;
 			}
+			// If we have generated claims for counterparty_commitment_txid earlier, we can rely on always


This is fine but we'll need to make sure we explicitly skip any pruning step when we receive a new revocation secret if we've seen the channel close already.

TheBlueMatt · 2024-06-03T16:24:54Z

The only change from @tnull's previous ACK is the following, so gonna merge:

diff --git a/lightning/src/chain/channelmonitor.rs b/lightning/src/chain/channelmonitor.rs
index 0dda18629..7a6551bab 100644
--- a/lightning/src/chain/channelmonitor.rs
+++ b/lightning/src/chain/channelmonitor.rs
@@ -3232,4 +3232,3 @@ impl<Signer: EcdsaChannelSigner> ChannelMonitorImpl<Signer> {
 	/// height > height + CLTV_SHARED_CLAIM_BUFFER. In any case, will install monitoring for
-	/// HTLC-Success/HTLC-Timeout transactions. This function should only be called when claimable HTLC
-	/// data is available in `counterparty_claimable_outpoints`.
+	/// HTLC-Success/HTLC-Timeout transactions.
 	///

G8XSU changed the title ~~Persist counterparty_claimable_outpoints out of channel_monitor~~ [Draft] Persist counterparty_claimable_outpoints out of channel_monitor May 9, 2024

TheBlueMatt reviewed May 13, 2024

View reviewed changes

lightning/src/chain/channelmonitor.rs Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

G8XSU force-pushed the 2024-05-08-claimable-persist-3049 branch from 0ee0080 to 2081c15 Compare May 14, 2024 07:40

G8XSU force-pushed the 2024-05-08-claimable-persist-3049 branch from 78fd449 to 3aea5ef Compare May 15, 2024 06:30

G8XSU changed the title ~~[Draft] Persist counterparty_claimable_outpoints out of channel_monitor~~ Persist counterparty_claimable_outpoints out of channel_monitor May 15, 2024

G8XSU marked this pull request as ready for review May 15, 2024 21:29

G8XSU commented May 15, 2024

View reviewed changes

G8XSU changed the title ~~Persist counterparty_claimable_outpoints out of channel_monitor~~ Minimize reads to counterparty_claimable_outpoints May 16, 2024

G8XSU mentioned this pull request May 16, 2024

Add PersistClaimInfo and ClaimInfoRequest events #3067

Closed

G8XSU mentioned this pull request May 17, 2024

[Memory] Persist counterparty_claimable_outpoints out of channel_monitor #3049

Open

9 tasks

G8XSU commented May 17, 2024

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

TheBlueMatt reviewed May 18, 2024

View reviewed changes

G8XSU force-pushed the 2024-05-08-claimable-persist-3049 branch 2 times, most recently from 59eb80e to ea5f999 Compare May 21, 2024 01:19

G8XSU mentioned this pull request May 22, 2024

Convert check_htlc_valid_counterparty and log_claim macros to fns #3076

Closed

tnull reviewed May 22, 2024

View reviewed changes

lightning/src/chain/channelmonitor.rs Outdated Show resolved Hide resolved

lightning/src/chain/channelmonitor.rs Show resolved Hide resolved

G8XSU force-pushed the 2024-05-08-claimable-persist-3049 branch from 3435f18 to 39900bb Compare May 22, 2024 20:19

tnull previously approved these changes May 23, 2024

View reviewed changes

TheBlueMatt reviewed May 28, 2024

View reviewed changes

Minimize reads to counterparty_claimable_outpoints

1690c66

Reads related to current/previous commitment_tx should be explicit and reads related to older commitment_txs should be minimized.

G8XSU dismissed tnull’s stale review via 1690c66 May 30, 2024 16:49

G8XSU force-pushed the 2024-05-08-claimable-persist-3049 branch from 39900bb to 1690c66 Compare May 30, 2024 16:49

G8XSU requested review from TheBlueMatt and tnull May 30, 2024 17:10

TheBlueMatt approved these changes May 30, 2024

View reviewed changes

TheBlueMatt merged commit 7326334 into lightningdevkit:main Jun 3, 2024

Minimize reads to counterparty_claimable_outpoints #3057

Minimize reads to counterparty_claimable_outpoints #3057

Uh oh!

Conversation

G8XSU commented May 9, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov-commenter commented May 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

G8XSU commented May 15, 2024

Uh oh!

G8XSU May 15, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G8XSU commented May 16, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G8XSU May 21, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

tnull left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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!

G8XSU May 30, 2024 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jun 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

G8XSU commented May 9, 2024 •

edited

Loading

codecov-commenter commented May 14, 2024 •

edited

Loading

G8XSU May 15, 2024 •

edited

Loading

G8XSU May 21, 2024 •

edited

Loading

G8XSU May 30, 2024 •

edited

Loading