added more clarification by gabbyprecious · Pull Request #1395 · lightningdevkit/rust-lightning

gabbyprecious · 2022-03-30T13:41:05Z

No description provided.

gabbyprecious · 2022-03-30T13:41:35Z

@ConorOkus please is this in line?

codecov-commenter · 2022-03-30T14:12:34Z

Codecov Report

Merging #1395 (a3820f5) into main (7671ae5) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@ Coverage Diff @@## main #1395 +/- ##
==========================================
- Coverage 90.76% 90.76% -0.01% 
==========================================
Files 73 73 Lines 41195 41195 Branches 41195 41195 ==========================================
- Hits 37392 37391 -1 - Misses 3803 3804 +1

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	`61.11% <ø> (ø)`
lightning/src/ln/functional_tests.rs	`97.06% <0.00%> (-0.02%)`	⬇️

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 7671ae5...a3820f5. Read the comment docs.

TheBlueMatt · 2022-03-30T15:00:16Z

 pub trait Filter {
-	/// Registers interest in a transaction with `txid` and having an output with `script_pubkey` as
+	/// Registers interest in funding transactions to inform LDK that a channel
+	/// Funding transaction is transaction with `txid` and having an output with `script_pubkey` as


I don't think we want to narrow the definition of this function to only funding transaction(s). Yes, it's true today, but specifying it in the docs and committing to it I don't think we want to do.

What would be a better description that clarifies and differentiates what it does with register_output

The key difference here is one looks at a given transaction being broadcasted, one looks for spends of a given output being broadcasted.

@TheBlueMatt, should I go with this;
/// Registers interest in transactions to inform LDK that a channel.
/// Transactions with txid and having an output with script_pubkey as
/// a spending condition.

I dont think these functions should have any reference to what kind of transaction it is (channel/close/open/etc).

So I should leave them at what they were?

I mean I do think they need to be clarified, because we've seen a good chunk of confusion on them, but it remains entirely unclear to me exactly how they should be clarified :/

TheBlueMatt · 2022-03-30T15:00:30Z

 	fn register_tx(&self, txid: &Txid, script_pubkey: &Script);

-	/// Registers interest in spends of a transaction output.
+	/// Registers interest in spends of a  force close transaction.


This isn't just about force-closure transactions - we also use it to detect normal closures, etc.

@TheBlueMatt is this a better description; Registers interest in spends of a closing transaction.

Its not "spends of a closing transaction", though, its "spends of a given outpoint", ie any time a given output is spent we care.

TheBlueMatt · 2022-05-24T21:35:20Z

Any interest in continuing to work on this, @gabbyprecious? Sorry it ended up being a bit unclear what the docs should say.

gabbyprecious · 2022-05-30T07:06:03Z

@TheBlueMatt yes, is there any clarity on what it should say. Will also be checking other issues

TheBlueMatt · 2022-06-04T00:18:33Z

Sorry for the late reply, we had the lightning spec summit last week.

I don't have perfect clarity - I've seen a lot of user confusion on this API, so I think we do need to clarify, but its kinda unclear to me how to do so (without changing the definition/API contract here).

Do you find the meaning of register_tx clearer if we separate out the mention of the script from the first sentence? Maybe something like "Registers interest in a given transaction confirming. The txid of the transaction is provided, as well as a script_pubkey of an output in the transaction, useful for those using BIP 157/158 filters"?

TheBlueMatt · 2022-11-30T19:26:06Z

Any desire to keep working on this @gabbyprecious?

gabbyprecious · 2022-11-30T19:32:47Z

Yes, if there's more clarity on it.

TheBlueMatt · 2022-12-07T23:27:12Z

Sorry for the delay in responding. Maybe we just include in the docs a specific contrast between the two methods. Ie in one of them write exactly how it differs from the other, noting that one is about spends and the other is about transactions themselves.

TheBlueMatt · 2024-05-05T19:55:48Z

Closing in favor of #3036

gabbyprecious · 2022-03-30T13:41:05Z

No description provided.

gabbyprecious · 2022-03-30T13:41:35Z

@ConorOkus please is this in line?

codecov-commenter · 2022-03-30T14:12:34Z

Codecov Report

Merging #1395 (a3820f5) into main (7671ae5) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@ Coverage Diff @@## main #1395 +/- ##
==========================================
- Coverage 90.76% 90.76% -0.01% 
==========================================
Files 73 73 Lines 41195 41195 Branches 41195 41195 ==========================================
- Hits 37392 37391 -1 - Misses 3803 3804 +1

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	`61.11% <ø> (ø)`
lightning/src/ln/functional_tests.rs	`97.06% <0.00%> (-0.02%)`	⬇️

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 7671ae5...a3820f5. Read the comment docs.

TheBlueMatt · 2022-03-30T15:00:16Z

 pub trait Filter {
-	/// Registers interest in a transaction with `txid` and having an output with `script_pubkey` as
+	/// Registers interest in funding transactions to inform LDK that a channel
+	/// Funding transaction is transaction with `txid` and having an output with `script_pubkey` as


I don't think we want to narrow the definition of this function to only funding transaction(s). Yes, it's true today, but specifying it in the docs and committing to it I don't think we want to do.

What would be a better description that clarifies and differentiates what it does with register_output

The key difference here is one looks at a given transaction being broadcasted, one looks for spends of a given output being broadcasted.

@TheBlueMatt, should I go with this;
/// Registers interest in transactions to inform LDK that a channel.
/// Transactions with txid and having an output with script_pubkey as
/// a spending condition.

I dont think these functions should have any reference to what kind of transaction it is (channel/close/open/etc).

So I should leave them at what they were?

I mean I do think they need to be clarified, because we've seen a good chunk of confusion on them, but it remains entirely unclear to me exactly how they should be clarified :/

TheBlueMatt · 2022-03-30T15:00:30Z

 	fn register_tx(&self, txid: &Txid, script_pubkey: &Script);

-	/// Registers interest in spends of a transaction output.
+	/// Registers interest in spends of a  force close transaction.


This isn't just about force-closure transactions - we also use it to detect normal closures, etc.

@TheBlueMatt is this a better description; Registers interest in spends of a closing transaction.

Its not "spends of a closing transaction", though, its "spends of a given outpoint", ie any time a given output is spent we care.

TheBlueMatt · 2022-05-24T21:35:20Z

Any interest in continuing to work on this, @gabbyprecious? Sorry it ended up being a bit unclear what the docs should say.

gabbyprecious · 2022-05-30T07:06:03Z

@TheBlueMatt yes, is there any clarity on what it should say. Will also be checking other issues

TheBlueMatt · 2022-06-04T00:18:33Z

Sorry for the late reply, we had the lightning spec summit last week.

I don't have perfect clarity - I've seen a lot of user confusion on this API, so I think we do need to clarify, but its kinda unclear to me how to do so (without changing the definition/API contract here).

Do you find the meaning of register_tx clearer if we separate out the mention of the script from the first sentence? Maybe something like "Registers interest in a given transaction confirming. The txid of the transaction is provided, as well as a script_pubkey of an output in the transaction, useful for those using BIP 157/158 filters"?

TheBlueMatt · 2022-11-30T19:26:06Z

Any desire to keep working on this @gabbyprecious?

gabbyprecious · 2022-11-30T19:32:47Z

Yes, if there's more clarity on it.

TheBlueMatt · 2022-12-07T23:27:12Z

Sorry for the delay in responding. Maybe we just include in the docs a specific contrast between the two methods. Ie in one of them write exactly how it differs from the other, noting that one is about spends and the other is about transactions themselves.

TheBlueMatt · 2024-05-05T19:55:48Z

Closing in favor of #3036

gabbyprecious · 2022-03-30T13:41:05Z

No description provided.

gabbyprecious · 2022-03-30T13:41:35Z

@ConorOkus please is this in line?

codecov-commenter · 2022-03-30T14:12:34Z

Codecov Report

Merging #1395 (a3820f5) into main (7671ae5) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@ Coverage Diff @@## main #1395 +/- ##
==========================================
- Coverage 90.76% 90.76% -0.01% 
==========================================
Files 73 73 Lines 41195 41195 Branches 41195 41195 ==========================================
- Hits 37392 37391 -1 - Misses 3803 3804 +1

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	`61.11% <ø> (ø)`
lightning/src/ln/functional_tests.rs	`97.06% <0.00%> (-0.02%)`	⬇️

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 7671ae5...a3820f5. Read the comment docs.

TheBlueMatt · 2022-03-30T15:00:16Z

 pub trait Filter {
-	/// Registers interest in a transaction with `txid` and having an output with `script_pubkey` as
+	/// Registers interest in funding transactions to inform LDK that a channel
+	/// Funding transaction is transaction with `txid` and having an output with `script_pubkey` as


I don't think we want to narrow the definition of this function to only funding transaction(s). Yes, it's true today, but specifying it in the docs and committing to it I don't think we want to do.

What would be a better description that clarifies and differentiates what it does with register_output

The key difference here is one looks at a given transaction being broadcasted, one looks for spends of a given output being broadcasted.

@TheBlueMatt, should I go with this;
/// Registers interest in transactions to inform LDK that a channel.
/// Transactions with txid and having an output with script_pubkey as
/// a spending condition.

I dont think these functions should have any reference to what kind of transaction it is (channel/close/open/etc).

So I should leave them at what they were?

I mean I do think they need to be clarified, because we've seen a good chunk of confusion on them, but it remains entirely unclear to me exactly how they should be clarified :/

TheBlueMatt · 2022-03-30T15:00:30Z

 	fn register_tx(&self, txid: &Txid, script_pubkey: &Script);

-	/// Registers interest in spends of a transaction output.
+	/// Registers interest in spends of a  force close transaction.


This isn't just about force-closure transactions - we also use it to detect normal closures, etc.

@TheBlueMatt is this a better description; Registers interest in spends of a closing transaction.

Its not "spends of a closing transaction", though, its "spends of a given outpoint", ie any time a given output is spent we care.

TheBlueMatt · 2022-05-24T21:35:20Z

Any interest in continuing to work on this, @gabbyprecious? Sorry it ended up being a bit unclear what the docs should say.

gabbyprecious · 2022-05-30T07:06:03Z

@TheBlueMatt yes, is there any clarity on what it should say. Will also be checking other issues

TheBlueMatt · 2022-06-04T00:18:33Z

Sorry for the late reply, we had the lightning spec summit last week.

I don't have perfect clarity - I've seen a lot of user confusion on this API, so I think we do need to clarify, but its kinda unclear to me how to do so (without changing the definition/API contract here).

Do you find the meaning of register_tx clearer if we separate out the mention of the script from the first sentence? Maybe something like "Registers interest in a given transaction confirming. The txid of the transaction is provided, as well as a script_pubkey of an output in the transaction, useful for those using BIP 157/158 filters"?

TheBlueMatt · 2022-11-30T19:26:06Z

Any desire to keep working on this @gabbyprecious?

gabbyprecious · 2022-11-30T19:32:47Z

Yes, if there's more clarity on it.

TheBlueMatt · 2022-12-07T23:27:12Z

Sorry for the delay in responding. Maybe we just include in the docs a specific contrast between the two methods. Ie in one of them write exactly how it differs from the other, noting that one is about spends and the other is about transactions themselves.

TheBlueMatt · 2024-05-05T19:55:48Z

Closing in favor of #3036

gabbyprecious · 2022-03-30T13:41:05Z

No description provided.

gabbyprecious · 2022-03-30T13:41:35Z

@ConorOkus please is this in line?

codecov-commenter · 2022-03-30T14:12:34Z

Codecov Report

Merging #1395 (a3820f5) into main (7671ae5) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@ Coverage Diff @@## main #1395 +/- ##
==========================================
- Coverage 90.76% 90.76% -0.01% 
==========================================
Files 73 73 Lines 41195 41195 Branches 41195 41195 ==========================================
- Hits 37392 37391 -1 - Misses 3803 3804 +1

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	`61.11% <ø> (ø)`
lightning/src/ln/functional_tests.rs	`97.06% <0.00%> (-0.02%)`	⬇️

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 7671ae5...a3820f5. Read the comment docs.

TheBlueMatt · 2022-03-30T15:00:16Z

 pub trait Filter {
-	/// Registers interest in a transaction with `txid` and having an output with `script_pubkey` as
+	/// Registers interest in funding transactions to inform LDK that a channel
+	/// Funding transaction is transaction with `txid` and having an output with `script_pubkey` as


I don't think we want to narrow the definition of this function to only funding transaction(s). Yes, it's true today, but specifying it in the docs and committing to it I don't think we want to do.

What would be a better description that clarifies and differentiates what it does with register_output

The key difference here is one looks at a given transaction being broadcasted, one looks for spends of a given output being broadcasted.

@TheBlueMatt, should I go with this;
/// Registers interest in transactions to inform LDK that a channel.
/// Transactions with txid and having an output with script_pubkey as
/// a spending condition.

I dont think these functions should have any reference to what kind of transaction it is (channel/close/open/etc).

So I should leave them at what they were?

I mean I do think they need to be clarified, because we've seen a good chunk of confusion on them, but it remains entirely unclear to me exactly how they should be clarified :/

TheBlueMatt · 2022-03-30T15:00:30Z

 	fn register_tx(&self, txid: &Txid, script_pubkey: &Script);

-	/// Registers interest in spends of a transaction output.
+	/// Registers interest in spends of a  force close transaction.


This isn't just about force-closure transactions - we also use it to detect normal closures, etc.

@TheBlueMatt is this a better description; Registers interest in spends of a closing transaction.

Its not "spends of a closing transaction", though, its "spends of a given outpoint", ie any time a given output is spent we care.

TheBlueMatt · 2022-05-24T21:35:20Z

Any interest in continuing to work on this, @gabbyprecious? Sorry it ended up being a bit unclear what the docs should say.

gabbyprecious · 2022-05-30T07:06:03Z

@TheBlueMatt yes, is there any clarity on what it should say. Will also be checking other issues

TheBlueMatt · 2022-06-04T00:18:33Z

Sorry for the late reply, we had the lightning spec summit last week.

I don't have perfect clarity - I've seen a lot of user confusion on this API, so I think we do need to clarify, but its kinda unclear to me how to do so (without changing the definition/API contract here).

Do you find the meaning of register_tx clearer if we separate out the mention of the script from the first sentence? Maybe something like "Registers interest in a given transaction confirming. The txid of the transaction is provided, as well as a script_pubkey of an output in the transaction, useful for those using BIP 157/158 filters"?

TheBlueMatt · 2022-11-30T19:26:06Z

Any desire to keep working on this @gabbyprecious?

gabbyprecious · 2022-11-30T19:32:47Z

Yes, if there's more clarity on it.

TheBlueMatt · 2022-12-07T23:27:12Z

Sorry for the delay in responding. Maybe we just include in the docs a specific contrast between the two methods. Ie in one of them write exactly how it differs from the other, noting that one is about spends and the other is about transactions themselves.

TheBlueMatt · 2024-05-05T19:55:48Z

Closing in favor of #3036

gabbyprecious · 2022-03-30T13:41:05Z

No description provided.

gabbyprecious · 2022-03-30T13:41:35Z

@ConorOkus please is this in line?

codecov-commenter · 2022-03-30T14:12:34Z

Codecov Report

Merging #1395 (a3820f5) into main (7671ae5) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@ Coverage Diff @@## main #1395 +/- ##
==========================================
- Coverage 90.76% 90.76% -0.01% 
==========================================
Files 73 73 Lines 41195 41195 Branches 41195 41195 ==========================================
- Hits 37392 37391 -1 - Misses 3803 3804 +1

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	`61.11% <ø> (ø)`
lightning/src/ln/functional_tests.rs	`97.06% <0.00%> (-0.02%)`	⬇️

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 7671ae5...a3820f5. Read the comment docs.

TheBlueMatt · 2022-03-30T15:00:16Z

 pub trait Filter {
-	/// Registers interest in a transaction with `txid` and having an output with `script_pubkey` as
+	/// Registers interest in funding transactions to inform LDK that a channel
+	/// Funding transaction is transaction with `txid` and having an output with `script_pubkey` as


I don't think we want to narrow the definition of this function to only funding transaction(s). Yes, it's true today, but specifying it in the docs and committing to it I don't think we want to do.

What would be a better description that clarifies and differentiates what it does with register_output

The key difference here is one looks at a given transaction being broadcasted, one looks for spends of a given output being broadcasted.

@TheBlueMatt, should I go with this;
/// Registers interest in transactions to inform LDK that a channel.
/// Transactions with txid and having an output with script_pubkey as
/// a spending condition.

I dont think these functions should have any reference to what kind of transaction it is (channel/close/open/etc).

So I should leave them at what they were?

I mean I do think they need to be clarified, because we've seen a good chunk of confusion on them, but it remains entirely unclear to me exactly how they should be clarified :/

TheBlueMatt · 2022-03-30T15:00:30Z

 	fn register_tx(&self, txid: &Txid, script_pubkey: &Script);

-	/// Registers interest in spends of a transaction output.
+	/// Registers interest in spends of a  force close transaction.


This isn't just about force-closure transactions - we also use it to detect normal closures, etc.

@TheBlueMatt is this a better description; Registers interest in spends of a closing transaction.

Its not "spends of a closing transaction", though, its "spends of a given outpoint", ie any time a given output is spent we care.

TheBlueMatt · 2022-05-24T21:35:20Z

Any interest in continuing to work on this, @gabbyprecious? Sorry it ended up being a bit unclear what the docs should say.

gabbyprecious · 2022-05-30T07:06:03Z

@TheBlueMatt yes, is there any clarity on what it should say. Will also be checking other issues

TheBlueMatt · 2022-06-04T00:18:33Z

Sorry for the late reply, we had the lightning spec summit last week.

I don't have perfect clarity - I've seen a lot of user confusion on this API, so I think we do need to clarify, but its kinda unclear to me how to do so (without changing the definition/API contract here).

Do you find the meaning of register_tx clearer if we separate out the mention of the script from the first sentence? Maybe something like "Registers interest in a given transaction confirming. The txid of the transaction is provided, as well as a script_pubkey of an output in the transaction, useful for those using BIP 157/158 filters"?

TheBlueMatt · 2022-11-30T19:26:06Z

Any desire to keep working on this @gabbyprecious?

gabbyprecious · 2022-11-30T19:32:47Z

Yes, if there's more clarity on it.

TheBlueMatt · 2022-12-07T23:27:12Z

Sorry for the delay in responding. Maybe we just include in the docs a specific contrast between the two methods. Ie in one of them write exactly how it differs from the other, noting that one is about spends and the other is about transactions themselves.

TheBlueMatt · 2024-05-05T19:55:48Z

Closing in favor of #3036

gabbyprecious · 2022-03-30T13:41:05Z

No description provided.

gabbyprecious · 2022-03-30T13:41:35Z

@ConorOkus please is this in line?

codecov-commenter · 2022-03-30T14:12:34Z

Codecov Report

Merging #1395 (a3820f5) into main (7671ae5) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@ Coverage Diff @@## main #1395 +/- ##
==========================================
- Coverage 90.76% 90.76% -0.01% 
==========================================
Files 73 73 Lines 41195 41195 Branches 41195 41195 ==========================================
- Hits 37392 37391 -1 - Misses 3803 3804 +1

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	`61.11% <ø> (ø)`
lightning/src/ln/functional_tests.rs	`97.06% <0.00%> (-0.02%)`	⬇️

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 7671ae5...a3820f5. Read the comment docs.

TheBlueMatt · 2022-03-30T15:00:16Z

 pub trait Filter {
-	/// Registers interest in a transaction with `txid` and having an output with `script_pubkey` as
+	/// Registers interest in funding transactions to inform LDK that a channel
+	/// Funding transaction is transaction with `txid` and having an output with `script_pubkey` as


I don't think we want to narrow the definition of this function to only funding transaction(s). Yes, it's true today, but specifying it in the docs and committing to it I don't think we want to do.

What would be a better description that clarifies and differentiates what it does with register_output

The key difference here is one looks at a given transaction being broadcasted, one looks for spends of a given output being broadcasted.

@TheBlueMatt, should I go with this;
/// Registers interest in transactions to inform LDK that a channel.
/// Transactions with txid and having an output with script_pubkey as
/// a spending condition.

I dont think these functions should have any reference to what kind of transaction it is (channel/close/open/etc).

So I should leave them at what they were?

I mean I do think they need to be clarified, because we've seen a good chunk of confusion on them, but it remains entirely unclear to me exactly how they should be clarified :/

TheBlueMatt · 2022-03-30T15:00:30Z

 	fn register_tx(&self, txid: &Txid, script_pubkey: &Script);

-	/// Registers interest in spends of a transaction output.
+	/// Registers interest in spends of a  force close transaction.


This isn't just about force-closure transactions - we also use it to detect normal closures, etc.

@TheBlueMatt is this a better description; Registers interest in spends of a closing transaction.

Its not "spends of a closing transaction", though, its "spends of a given outpoint", ie any time a given output is spent we care.

TheBlueMatt · 2022-05-24T21:35:20Z

Any interest in continuing to work on this, @gabbyprecious? Sorry it ended up being a bit unclear what the docs should say.

gabbyprecious · 2022-05-30T07:06:03Z

@TheBlueMatt yes, is there any clarity on what it should say. Will also be checking other issues

TheBlueMatt · 2022-06-04T00:18:33Z

Sorry for the late reply, we had the lightning spec summit last week.

I don't have perfect clarity - I've seen a lot of user confusion on this API, so I think we do need to clarify, but its kinda unclear to me how to do so (without changing the definition/API contract here).

Do you find the meaning of register_tx clearer if we separate out the mention of the script from the first sentence? Maybe something like "Registers interest in a given transaction confirming. The txid of the transaction is provided, as well as a script_pubkey of an output in the transaction, useful for those using BIP 157/158 filters"?

TheBlueMatt · 2022-11-30T19:26:06Z

Any desire to keep working on this @gabbyprecious?

gabbyprecious · 2022-11-30T19:32:47Z

Yes, if there's more clarity on it.

TheBlueMatt · 2022-12-07T23:27:12Z

Sorry for the delay in responding. Maybe we just include in the docs a specific contrast between the two methods. Ie in one of them write exactly how it differs from the other, noting that one is about spends and the other is about transactions themselves.

TheBlueMatt · 2024-05-05T19:55:48Z

Closing in favor of #3036

gabbyprecious · 2022-03-30T13:41:05Z

No description provided.

gabbyprecious · 2022-03-30T13:41:35Z

@ConorOkus please is this in line?

codecov-commenter · 2022-03-30T14:12:34Z

Codecov Report

Merging #1395 (a3820f5) into main (7671ae5) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@ Coverage Diff @@## main #1395 +/- ##
==========================================
- Coverage 90.76% 90.76% -0.01% 
==========================================
Files 73 73 Lines 41195 41195 Branches 41195 41195 ==========================================
- Hits 37392 37391 -1 - Misses 3803 3804 +1

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	`61.11% <ø> (ø)`
lightning/src/ln/functional_tests.rs	`97.06% <0.00%> (-0.02%)`	⬇️

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 7671ae5...a3820f5. Read the comment docs.

TheBlueMatt · 2022-03-30T15:00:16Z

 pub trait Filter {
-	/// Registers interest in a transaction with `txid` and having an output with `script_pubkey` as
+	/// Registers interest in funding transactions to inform LDK that a channel
+	/// Funding transaction is transaction with `txid` and having an output with `script_pubkey` as


I don't think we want to narrow the definition of this function to only funding transaction(s). Yes, it's true today, but specifying it in the docs and committing to it I don't think we want to do.

What would be a better description that clarifies and differentiates what it does with register_output

The key difference here is one looks at a given transaction being broadcasted, one looks for spends of a given output being broadcasted.

@TheBlueMatt, should I go with this;
/// Registers interest in transactions to inform LDK that a channel.
/// Transactions with txid and having an output with script_pubkey as
/// a spending condition.

I dont think these functions should have any reference to what kind of transaction it is (channel/close/open/etc).

So I should leave them at what they were?

I mean I do think they need to be clarified, because we've seen a good chunk of confusion on them, but it remains entirely unclear to me exactly how they should be clarified :/

TheBlueMatt · 2022-03-30T15:00:30Z

 	fn register_tx(&self, txid: &Txid, script_pubkey: &Script);

-	/// Registers interest in spends of a transaction output.
+	/// Registers interest in spends of a  force close transaction.


This isn't just about force-closure transactions - we also use it to detect normal closures, etc.

@TheBlueMatt is this a better description; Registers interest in spends of a closing transaction.

Its not "spends of a closing transaction", though, its "spends of a given outpoint", ie any time a given output is spent we care.

TheBlueMatt · 2022-05-24T21:35:20Z

Any interest in continuing to work on this, @gabbyprecious? Sorry it ended up being a bit unclear what the docs should say.

gabbyprecious · 2022-05-30T07:06:03Z

@TheBlueMatt yes, is there any clarity on what it should say. Will also be checking other issues

TheBlueMatt · 2022-06-04T00:18:33Z

Sorry for the late reply, we had the lightning spec summit last week.

I don't have perfect clarity - I've seen a lot of user confusion on this API, so I think we do need to clarify, but its kinda unclear to me how to do so (without changing the definition/API contract here).

Do you find the meaning of register_tx clearer if we separate out the mention of the script from the first sentence? Maybe something like "Registers interest in a given transaction confirming. The txid of the transaction is provided, as well as a script_pubkey of an output in the transaction, useful for those using BIP 157/158 filters"?

TheBlueMatt · 2022-11-30T19:26:06Z

Any desire to keep working on this @gabbyprecious?

gabbyprecious · 2022-11-30T19:32:47Z

Yes, if there's more clarity on it.

TheBlueMatt · 2022-12-07T23:27:12Z

Sorry for the delay in responding. Maybe we just include in the docs a specific contrast between the two methods. Ie in one of them write exactly how it differs from the other, noting that one is about spends and the other is about transactions themselves.

TheBlueMatt · 2024-05-05T19:55:48Z

Closing in favor of #3036

gabbyprecious · 2022-03-30T13:41:05Z

No description provided.

gabbyprecious · 2022-03-30T13:41:35Z

@ConorOkus please is this in line?

codecov-commenter · 2022-03-30T14:12:34Z

Codecov Report

Merging #1395 (a3820f5) into main (7671ae5) will decrease coverage by 0.00%.
The diff coverage is n/a.

@@ Coverage Diff @@## main #1395 +/- ##
==========================================
- Coverage 90.76% 90.76% -0.01% 
==========================================
Files 73 73 Lines 41195 41195 Branches 41195 41195 ==========================================
- Hits 37392 37391 -1 - Misses 3803 3804 +1

Impacted Files	Coverage Δ
lightning/src/chain/mod.rs	`61.11% <ø> (ø)`
lightning/src/ln/functional_tests.rs	`97.06% <0.00%> (-0.02%)`	⬇️

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 7671ae5...a3820f5. Read the comment docs.

TheBlueMatt · 2022-03-30T15:00:16Z

 pub trait Filter {
-	/// Registers interest in a transaction with `txid` and having an output with `script_pubkey` as
+	/// Registers interest in funding transactions to inform LDK that a channel
+	/// Funding transaction is transaction with `txid` and having an output with `script_pubkey` as


I don't think we want to narrow the definition of this function to only funding transaction(s). Yes, it's true today, but specifying it in the docs and committing to it I don't think we want to do.

What would be a better description that clarifies and differentiates what it does with register_output

The key difference here is one looks at a given transaction being broadcasted, one looks for spends of a given output being broadcasted.

@TheBlueMatt, should I go with this;
/// Registers interest in transactions to inform LDK that a channel.
/// Transactions with txid and having an output with script_pubkey as
/// a spending condition.

I dont think these functions should have any reference to what kind of transaction it is (channel/close/open/etc).

So I should leave them at what they were?

I mean I do think they need to be clarified, because we've seen a good chunk of confusion on them, but it remains entirely unclear to me exactly how they should be clarified :/

TheBlueMatt · 2022-03-30T15:00:30Z

 	fn register_tx(&self, txid: &Txid, script_pubkey: &Script);

-	/// Registers interest in spends of a transaction output.
+	/// Registers interest in spends of a  force close transaction.


This isn't just about force-closure transactions - we also use it to detect normal closures, etc.

@TheBlueMatt is this a better description; Registers interest in spends of a closing transaction.

Its not "spends of a closing transaction", though, its "spends of a given outpoint", ie any time a given output is spent we care.

TheBlueMatt · 2022-05-24T21:35:20Z

Any interest in continuing to work on this, @gabbyprecious? Sorry it ended up being a bit unclear what the docs should say.

gabbyprecious · 2022-05-30T07:06:03Z

@TheBlueMatt yes, is there any clarity on what it should say. Will also be checking other issues

TheBlueMatt · 2022-06-04T00:18:33Z

Sorry for the late reply, we had the lightning spec summit last week.

I don't have perfect clarity - I've seen a lot of user confusion on this API, so I think we do need to clarify, but its kinda unclear to me how to do so (without changing the definition/API contract here).

Do you find the meaning of register_tx clearer if we separate out the mention of the script from the first sentence? Maybe something like "Registers interest in a given transaction confirming. The txid of the transaction is provided, as well as a script_pubkey of an output in the transaction, useful for those using BIP 157/158 filters"?

TheBlueMatt · 2022-11-30T19:26:06Z

Any desire to keep working on this @gabbyprecious?

gabbyprecious · 2022-11-30T19:32:47Z

Yes, if there's more clarity on it.

TheBlueMatt · 2022-12-07T23:27:12Z

Sorry for the delay in responding. Maybe we just include in the docs a specific contrast between the two methods. Ie in one of them write exactly how it differs from the other, noting that one is about spends and the other is about transactions themselves.

TheBlueMatt · 2024-05-05T19:55:48Z

Closing in favor of #3036

added more clarification
a3820f5

gabbyprecious marked this pull request as draft March 30, 2022 13:41

TheBlueMatt reviewed Mar 30, 2022
View reviewed changes

tnull mentioned this pull request Dec 9, 2022
Further clarify register_tx/register_output distinction in docs #1069
Closed

tnull linked an issue Dec 9, 2022 that may be closed by this pull request
Further clarify register_tx/register_output distinction in docs #1069
Closed

TheBlueMatt mentioned this pull request May 5, 2024
Provide more color in filter registration methods #3036
Merged

TheBlueMatt closed this May 5, 2024

added more clarification
a3820f5

gabbyprecious marked this pull request as draft March 30, 2022 13:41

TheBlueMatt reviewed Mar 30, 2022
View reviewed changes

tnull mentioned this pull request Dec 9, 2022
Further clarify register_tx/register_output distinction in docs #1069
Closed

tnull linked an issue Dec 9, 2022 that may be closed by this pull request
Further clarify register_tx/register_output distinction in docs #1069
Closed

TheBlueMatt mentioned this pull request May 5, 2024
Provide more color in filter registration methods #3036
Merged

TheBlueMatt closed this May 5, 2024

added more clarification
a3820f5

gabbyprecious marked this pull request as draft March 30, 2022 13:41

TheBlueMatt reviewed Mar 30, 2022
View reviewed changes

tnull mentioned this pull request Dec 9, 2022
Further clarify register_tx/register_output distinction in docs #1069
Closed

tnull linked an issue Dec 9, 2022 that may be closed by this pull request
Further clarify register_tx/register_output distinction in docs #1069
Closed

TheBlueMatt mentioned this pull request May 5, 2024
Provide more color in filter registration methods #3036
Merged

TheBlueMatt closed this May 5, 2024

added more clarification
a3820f5

gabbyprecious marked this pull request as draft March 30, 2022 13:41

TheBlueMatt reviewed Mar 30, 2022
View reviewed changes

tnull mentioned this pull request Dec 9, 2022
Further clarify register_tx/register_output distinction in docs #1069
Closed

tnull linked an issue Dec 9, 2022 that may be closed by this pull request
Further clarify register_tx/register_output distinction in docs #1069
Closed

TheBlueMatt mentioned this pull request May 5, 2024
Provide more color in filter registration methods #3036
Merged

TheBlueMatt closed this May 5, 2024

added more clarification
a3820f5

gabbyprecious marked this pull request as draft March 30, 2022 13:41

TheBlueMatt reviewed Mar 30, 2022
View reviewed changes

tnull mentioned this pull request Dec 9, 2022
Further clarify register_tx/register_output distinction in docs #1069
Closed

tnull linked an issue Dec 9, 2022 that may be closed by this pull request
Further clarify register_tx/register_output distinction in docs #1069
Closed

TheBlueMatt mentioned this pull request May 5, 2024
Provide more color in filter registration methods #3036
Merged

TheBlueMatt closed this May 5, 2024

added more clarification
a3820f5

gabbyprecious marked this pull request as draft March 30, 2022 13:41

TheBlueMatt reviewed Mar 30, 2022
View reviewed changes

tnull mentioned this pull request Dec 9, 2022
Further clarify register_tx/register_output distinction in docs #1069
Closed

tnull linked an issue Dec 9, 2022 that may be closed by this pull request
Further clarify register_tx/register_output distinction in docs #1069
Closed

TheBlueMatt mentioned this pull request May 5, 2024
Provide more color in filter registration methods #3036
Merged

TheBlueMatt closed this May 5, 2024

added more clarification
a3820f5

gabbyprecious marked this pull request as draft March 30, 2022 13:41

TheBlueMatt reviewed Mar 30, 2022
View reviewed changes

tnull mentioned this pull request Dec 9, 2022
Further clarify register_tx/register_output distinction in docs #1069
Closed

tnull linked an issue Dec 9, 2022 that may be closed by this pull request
Further clarify register_tx/register_output distinction in docs #1069
Closed

TheBlueMatt mentioned this pull request May 5, 2024
Provide more color in filter registration methods #3036
Merged

TheBlueMatt closed this May 5, 2024

added more clarification
a3820f5

gabbyprecious marked this pull request as draft March 30, 2022 13:41

TheBlueMatt reviewed Mar 30, 2022
View reviewed changes

tnull mentioned this pull request Dec 9, 2022
Further clarify register_tx/register_output distinction in docs #1069
Closed

tnull linked an issue Dec 9, 2022 that may be closed by this pull request
Further clarify register_tx/register_output distinction in docs #1069
Closed

TheBlueMatt mentioned this pull request May 5, 2024
Provide more color in filter registration methods #3036
Merged

TheBlueMatt closed this May 5, 2024

Uh oh!

Conversation

gabbyprecious commented Mar 30, 2022

Uh oh!

gabbyprecious commented Mar 30, 2022

Uh oh!

codecov-commenter commented Mar 30, 2022

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!

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 commented May 24, 2022

Uh oh!

gabbyprecious commented May 30, 2022

Uh oh!

TheBlueMatt commented Jun 4, 2022

Uh oh!

TheBlueMatt commented Nov 30, 2022

Uh oh!

gabbyprecious commented Nov 30, 2022

Uh oh!

TheBlueMatt commented Dec 7, 2022

Uh oh!

TheBlueMatt commented May 5, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

gabbyprecious commented Mar 30, 2022

Uh oh!

gabbyprecious commented Mar 30, 2022

Uh oh!

codecov-commenter commented Mar 30, 2022

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!

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!