Drop the redundant/broken `ChannelMonitor::get_monitored_outpoints` #722

TheBlueMatt · 2020-09-27T22:16:08Z

Based on #649. Offending PRs are #223 and #455.

In review of the final doc changes in #649, I noticed there
appeared to be redundant monitored-outpoints function in
`ChannelMonitor` - `get_monitored_outpoints()` and
`get_outputs_to_watch()`.

In 6f08779b0439e7e4367a75f4ee88de093dfb68cb,
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 73dce207dd0ea6c3ac57af3ebb8b87ee03e82c9e,
`get_outputs_to_watch` was added which was overly cautious to
ensure nothing was missed. Still, the author of 73dce207dd0ea6c3ac5
(me) seemed entirely unaware of the work in 6f08779b0439e7e4367a75f
(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()`.

codecov · 2020-09-27T22:59:49Z

Codecov Report

Merging #722 into main will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #722   +/-   ##
=======================================
  Coverage   92.00%   92.00%           
=======================================
  Files          37       37           
  Lines       20106    20100    -6     
=======================================
- Hits        18498    18493    -5     
+ Misses       1608     1607    -1

Impacted Files	Coverage Δ
lightning/src/ln/channelmanager.rs	`85.64% <ø> (ø)`
lightning/src/chain/channelmonitor.rs	`95.43% <100.00%> (+0.06%)`	⬆️
lightning/src/ln/functional_tests.rs	`97.06% <0.00%> (ø)`

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 8fb4a3d...78e98ba. Read the comment docs.

devrandom · 2020-09-28T08:06:31Z

lightning/src/chain/channelmonitor.rs

@@ -782,7 +626,7 @@ pub struct ChannelMonitor<ChanSigner: ChannelKeys> {
 	/// spending. Thus, in order to claim them via revocation key, we track all the counterparty
 	/// commitment transactions which we find on-chain, mapping them to the commitment number which
 	/// can be used to derive the revocation key and claim the transactions.
-	counterparty_commitment_txn_on_chain: HashMap<Txid, (u64, Vec<Script>)>,
+	counterparty_commitment_txn_on_chain: HashMap<Txid, u64>,


Can this map be dropped now? The commitment number can be decoded from the commitment TX via:

let commitment_number = (((commitment_tx.input[0].sequence as u64 & 0xffffff) << 3 * 8) | (commitment_tx.lock_time as u64 & 0xffffff)) ^ obscure_factor;

oh, I guess this is for sending watchtowers?

Its used to figure out the commitment number given an HTLC tx (ie by looking at the previous output spent), which I don't believe is otherwise possible. Any suggestions for updating the comment to make that more clear?

I must be missing something. We cannot see HTLC transactions on-chain before we see the commitment tx on-chain and there can only be one of those. Once we see the commitment tx, we can decode and stash the commitment number in variable in this struct, so we have it for the HTLC tx. So I still don't see the need for a map.

Right, there was some Slack discussion on this, but, yes, ultimately we could track only the latest remote commitment tx we saw on-chain (because to invalidate that we'd need a reorg, invalidating child HTLC txn too). The code to do that would likely be somewhat simpler, but I'd really like more testing in ChannelMonitor, so am ~0/-0 on such a change, at least not gonna jump to implement it myself.

Thank you for summarizing the discussion, I might give it a try at some point.

fyi @devrandom this map was introduced before we had any serious reorg-handling in ChannelMonitor and thus it was simpler at that time to just have a dumb map instead of stashing reorg'ed commitment.

A real improvement would be instead to store a tuple of (commitment_txid, per_commitment_point, per_commitment_key) as it would avoid to fetch the secret holder and re-derive a per-commitment each time we see a revoked child HTLC. It could be done once for all when we see the commitment. I think.

Also I think you can remove the first line of comment "We cannot identify HTLC-Success or HTLC-Timeout transactions by themselves on the chain." In fact we should be able to do so if we watch well commitment outputs. There is a line of further parsing in check_spend_counterparty_htlc to qualify them well as HTLCs.

I'm a little confused by the second comment there? Its true we can't identify them "by themselves", that's why we have the map, no? (and check_spend_counterparty_htlc is only called after looking up in the map).

jkczyz

Nice clean-up!

jkczyz · 2020-09-29T07:33:48Z

lightning/src/chain/channelmonitor.rs

+			for (watched, output) in watched_outputs.iter().zip(outputs.iter()) {
+				assert_eq!(watched, output);
 			}


Are the order of these lists guaranteed to match? Guess this is dropped in the next commit, so may not matter.

They should be, at least modulo #653. I did separate it out so that at least its visible that the current tests don't have things in different order.

TheBlueMatt · 2020-10-02T15:33:19Z

Rebased, should be good to go.

ariard · 2020-10-03T00:33:45Z

lightning/src/chain/channelmonitor.rs

@@ -782,7 +626,7 @@ pub struct ChannelMonitor<ChanSigner: ChannelKeys> {
 	/// spending. Thus, in order to claim them via revocation key, we track all the counterparty
 	/// commitment transactions which we find on-chain, mapping them to the commitment number which
 	/// can be used to derive the revocation key and claim the transactions.
-	counterparty_commitment_txn_on_chain: HashMap<Txid, (u64, Vec<Script>)>,
+	counterparty_commitment_txn_on_chain: HashMap<Txid, u64>,


fyi @devrandom this map was introduced before we had any serious reorg-handling in ChannelMonitor and thus it was simpler at that time to just have a dumb map instead of stashing reorg'ed commitment.

A real improvement would be instead to store a tuple of (commitment_txid, per_commitment_point, per_commitment_key) as it would avoid to fetch the secret holder and re-derive a per-commitment each time we see a revoked child HTLC. It could be done once for all when we see the commitment. I think.

lightning/src/chain/channelmonitor.rs

ariard · 2020-10-03T00:40:50Z

lightning/src/chain/channelmonitor.rs

@@ -782,7 +626,7 @@ pub struct ChannelMonitor<ChanSigner: ChannelKeys> {
 	/// spending. Thus, in order to claim them via revocation key, we track all the counterparty
 	/// commitment transactions which we find on-chain, mapping them to the commitment number which
 	/// can be used to derive the revocation key and claim the transactions.
-	counterparty_commitment_txn_on_chain: HashMap<Txid, (u64, Vec<Script>)>,
+	counterparty_commitment_txn_on_chain: HashMap<Txid, u64>,


Also I think you can remove the first line of comment "We cannot identify HTLC-Success or HTLC-Timeout transactions by themselves on the chain." In fact we should be able to do so if we watch well commitment outputs. There is a line of further parsing in check_spend_counterparty_htlc to qualify them well as HTLCs.

ariard · 2020-10-05T16:14:34Z

Code Review ACK 78e98ba, a new comment to underscore the map consistency has been introduced since last time.

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()`.

This nearly fully reverts 6f08779, removing the extra data storage that it added.

TheBlueMatt · 2020-10-05T16:19:51Z

Oops. Rebased to pick up upstream CI fixes.

devrandom reviewed Sep 28, 2020

View reviewed changes

jkczyz self-requested a review September 29, 2020 00:04

jkczyz approved these changes Sep 29, 2020

View reviewed changes

TheBlueMatt force-pushed the 2020-09-broken-fn branch from 77b4288 to 0ffbdbd Compare October 2, 2020 15:33

TheBlueMatt added this to the 0.0.12 milestone Oct 2, 2020

ariard reviewed Oct 3, 2020

View reviewed changes

TheBlueMatt force-pushed the 2020-09-broken-fn branch from 0ffbdbd to 78e98ba Compare October 5, 2020 15:56

TheBlueMatt added 2 commits October 5, 2020 12:19

Drop now-unused Vec of outpoints in remote-commitment-tx-tracking

8e99800

This nearly fully reverts 6f08779, removing the extra data storage that it added.

TheBlueMatt force-pushed the 2020-09-broken-fn branch from 78e98ba to 8e99800 Compare October 5, 2020 16:19

TheBlueMatt merged commit 8566486 into lightningdevkit:main Oct 5, 2020

Drop the redundant/broken ChannelMonitor::get_monitored_outpoints #722

Drop the redundant/broken ChannelMonitor::get_monitored_outpoints #722

Uh oh!

Conversation

TheBlueMatt commented Sep 27, 2020

Uh oh!

codecov bot commented Sep 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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 Oct 5, 2020 • 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!

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

Uh oh!

TheBlueMatt commented Oct 2, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ariard commented Oct 5, 2020

Uh oh!

TheBlueMatt commented Oct 5, 2020

Uh oh!

Uh oh!

Drop the redundant/broken `ChannelMonitor::get_monitored_outpoints` #722

Drop the redundant/broken `ChannelMonitor::get_monitored_outpoints` #722

codecov bot commented Sep 27, 2020 •

edited

Loading

TheBlueMatt Oct 5, 2020 •

edited

Loading