Uh oh!
There was an error while loading. Please reload this page.
Drop the redundant/broken ChannelMonitor::get_monitored_outpoints - #722
Conversation
Codecov Report
@@ Coverage Diff @@## main #722 +/- ##
=======================================
Coverage 92.00% 92.00% =======================================
Files 37 37 Lines 20106 20100 -6 =======================================
- Hits 18498 18493 -5 + Misses 1608 1607 -1
Continue to review full report at Codecov.
|
| /// 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>, |
There was a problem hiding this comment.
Can this map be dropped now? The commitment number can be decoded from the commitment TX via:
let commitment_number = (((commitment_tx.input[0].sequenceasu64&0xffffff) << 3*8)
| (commitment_tx.lock_timeasu64&0xffffff))
^ obscure_factor;There was a problem hiding this comment.
oh, I guess this is for sending watchtowers?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thank you for summarizing the discussion, I might give it a try at some point.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
| for (watched, output) in watched_outputs.iter().zip(outputs.iter()) { | ||
| assert_eq!(watched, output); | ||
| } |
There was a problem hiding this comment.
Are the order of these lists guaranteed to match? Guess this is dropped in the next commit, so may not matter.
There was a problem hiding this comment.
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.
77b4288 to
0ffbdbdCompareTheBlueMatt
commented
Oct 2, 2020
Rebased, should be good to go. |
| /// 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>, |
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
| /// 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>, |
There was a problem hiding this comment.
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.
0ffbdbd to
78e98baCompareariard
commented
Oct 5, 2020
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.
78e98ba to
8e99800CompareTheBlueMatt
commented
Oct 5, 2020
Oops. Rebased to pick up upstream CI fixes. |
Based on #649. Offending PRs are #223 and #455.