Remove unnecessary todo #746
Conversation
There was a problem hiding this comment.
Technically, I think this is only really resolved by #611, but we should be using issues, not TODO's anyway as we move forward.
Oh, I guess in the sense that any TODO relating to claiming backwards HTLCs is only truly solved by 611... (unless I misunderstand) |
|
Heh, right, I read it that way, but the fact that it can be read multiple ways is maybe even a better reason to remove it :) |
Codecov Report
@@ Coverage Diff @@
## main #746 +/- ##
==========================================
- Coverage 91.38% 91.36% -0.02%
==========================================
Files 37 37
Lines 22067 22065 -2
==========================================
- Hits 20165 20159 -6
- Misses 1902 1906 +4
Continue to review full report at Codecov.
|
|
I think commit message is confusing. IIRC it's the ChannelMonitor responsibility to scan commitment and pass off-chain the preimage. It's done in
I agree this TODO is already covered, as it mentions outgoing-onchain-to-incoming-offchain preimage processing. #611 is about outgoing-offchain-to-incoming-onchain-if-already-closed. |
The ChannelMonitor already monitors the chain for counterparties revealing preimages, and will give the HTLCSources back to the ChannelManager for claiming. Thus it's unnecessary for the ChannelManager to monitor these HTLCs itself. See is_resolving_htlc_output: - if the counterparty broadcasted and then claimed one of the HTLCs we offered them, line 2015 is where the ChannelMonitor gives the ChannelManager the HTLC source - if we broadcasted and they claimed an HTLC we offered them, line 2025 is where the ChannelMonitor gives the ChannelManager the HTLC source
valentinewallace
force-pushed
the
remove-todo
branch
from
e382832 to
8e7b291
Compare
Hmm, I think the counterparty commitment case is L2015? Lmk if I'm off on that. Good point on the ChannelMonitor being the one passing the ChannelManager HTLC sources, updated commit msg! |
EDIT: You're right an accepted HTLC is an incoming one on the counterparty commitment, which make sense for them to claim it with a preimage. This code should be better documented I guess :) LGTM. |
The OnchainTxHandler already monitors the chain for counterparties
revealing preimages, etc, and will give the HTLCSources back to the
ChannelManager for claiming. Thus it's unnecessary for the ChannelManager
to monitor these HTLCs itself.
I had the start of a "solution" for this todo in the course of a #611 rabbit hole, then realized it's unnecessary. So figured it might be worth removing.