Introduce IsMine logic in ChannelMonitor for SpendableOutputDescriptor detection #552
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
TheBlueMatt
reviewed
Mar 19, 2020
lightning/src/ln/channelmonitor.rs
Outdated
| break; | ||
| } | ||
| } | ||
| let revokeable_script = if let Ok(local_delayedkey) = chan_utils::derive_private_key(&self.secp_ctx, &local_tx.per_commitment_point, delayed_payment_base_key) { |
There was a problem hiding this comment.
Why return this? I think we should be setting it here, make this fn take &mut self and do the local_tx.tx.add_local_sig call in it. This would be a good step towards DRY'ing up calls to local_tx.tx.add_local_sig so that we only ever do it once (which a reasonable ChannelKeys would enforce anyway).
There was a problem hiding this comment.
Okay to delay it in its own dry'up local_tx.tx.add_local_sig PR ?
There was a problem hiding this comment.
Sure, I suppose that'll have to come either way.
ariard
force-pushed
the
2020-03-ismine-spendable
branch
from
eb45bb6 to
15986c8
Compare
|
@TheBlueMatt, addressed most of your comment, let me know if you find new issues/names suits you. |
TheBlueMatt
reviewed
Mar 20, 2020
lightning/src/ln/channelmonitor.rs
Outdated
| } else { | ||
| match self.key_storage { | ||
| Storage::Local { ref shutdown_pubkey, .. } => { | ||
| let our_channel_close_key_hash = Hash160::hash(&shutdown_pubkey.serialize()); |
There was a problem hiding this comment.
This is also an addition of a Hash160 calculation per output in the block.
TheBlueMatt
approved these changes
Mar 20, 2020
|
Sadly needs rebase as well now. |
added 6 commits
March 19, 2020 22:29
Previously, we would generate SpendableOutputDescriptor::StaticOutput in OnchainTxHandler even if our claiming transaction wouldn't confirm onchain, misbehaving user wallet to think it receives more funds than in reality. Fix tests in consequence
is_paying_spendable_output Add ChannelMonitor::broadcasted_local_revokable_script to detect onchain local txn paying back to us. Fix tests in consequence
is_paying_spendable_output Add ChannelMonitor::broadcasted_remote_payment_script to detect onchain remote txn paying back to us.
is_paying_spendable_output Add ChannelMonitor::shutdown_script to detect onchain tx paying back to us
We delay SpendableOutputDescriptor until reaching ANTI_REORG_DELAY to avoid misleading user wallet in case of reorg and alternative settlement on a channel output. Fix tests in consequence.
ariard
force-pushed
the
2020-03-ismine-spendable
branch
2 times, most recently
from
0f9b21e to
5a9d6f4
Compare
TheBlueMatt
reviewed
Mar 20, 2020
lightning/src/ln/functional_tests.rs
Outdated
|
|
||
| let (_, our_payment_hash) = route_payment(&nodes[1], &vec!(&nodes[0])[..], 3_000_000); | ||
|
|
||
| let commitment_tx = nodes[0].node.channel_state.lock().unwrap().by_id.get_mut(&chan_1.2).unwrap().channel_monitor().get_latest_local_commitment_txn(); |
There was a problem hiding this comment.
Please use the new macro here.
TheBlueMatt
approved these changes
Mar 20, 2020
Cover previously missing SpendableOuputDescriptor for timeout tx on non-revoked remote commitment tx. Fix lightningdevkit#338
ariard
force-pushed
the
2020-03-ismine-spendable
branch
from
5a9d6f4 to
1c7b6c8
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously SpendableOutputDescriptor detection was spread in a lot of different places (ChannelMonitor local/remote tx parsing, OnchainTxHandler claim tx generation, closing tx monitoring, ...). This patch gather everything in one new method ChannelMonitor::is_mine. To cover some cases, a bit more of caching is done in ChannelMonitor struct.
Cases covered:
Also introduces MaturingOutput to prevent generating bogus SpendableOuputDescriptor in case of reorg < ANTI_REORG_DELAY.
Takes new commit from #532