DRY funding_created() and funding_signed() for V1 channels
#3370
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3370 +/- ##
==========================================
- Coverage 89.61% 89.58% -0.04%
==========================================
Files 127 127
Lines 103534 103742 +208
Branches 103534 103742 +208
==========================================
+ Hits 92781 92936 +155
- Misses 8053 8098 +45
- Partials 2700 2708 +8 ☔ View full report in Codecov by Sentry. |
dunxen
force-pushed
the
2024-10-dry-funding-created-signed
branch
from
f1370bd to
a82a65a
Compare
lightning/src/ln/channel.rs
Outdated
| let initial_commitment_tx = match self.check_counterparty_commitment_signature(&counterparty_signature, logger) { | ||
| Ok(res) => res, | ||
| Err(ChannelError::Close(e)) => { | ||
| self.context_mut().channel_transaction_parameters.funding_outpoint = None; |
There was a problem hiding this comment.
I don't think it actually breaks anything (somewhat to my surprise), but ideally let's only do this for inbound (and V2?) channels - outbound ones already have a funding outpoint assigned and it seems weird to remove that because a counterparty gave us a bunk signature.
There was a problem hiding this comment.
I'll have to think about V2 back in #3137, but the rest makes sense. Will update.
lightning/src/ln/channel.rs
Outdated
| obscure_factor, | ||
| holder_commitment_tx, best_block, context.counterparty_node_id, context.channel_id()); | ||
| channel_monitor.provide_initial_counterparty_commitment_tx( | ||
| counterparty_txid, Vec::new(), |
There was a problem hiding this comment.
counterparty_txid needs to match counterparty_initial_bitcoin_tx.transaction.txid() but sadly there's a difference between the numbers on the sending vs receiving end. In funding_signed we calculate the initial counterparty commitment transaction number using self.context.cur_counterparty_commitment_transaction_number but in funding_created we have to add a + 1 because we decrement cur_counterparty_commitment_transaction_number before we call initial_commitment_signed.
Instead, we should make the value common across both paths so that we don't have to pass the counterparty_txid in at all and can just use what we calculate here. Then we could drop counterparty_initial_commitment_tx from funding_created and counterparty_initial_bitcoin_tx from funding_signed.
There was a problem hiding this comment.
Yeah I did notice that difference. Will make them the same.
TheBlueMatt
left a comment
TheBlueMatt
left a comment
There was a problem hiding this comment.
LGTM, feel free to squash.
lightning/src/ln/channel.rs
Outdated
| let counterparty_trusted_tx = counterparty_initial_commitment_tx.trust(); | ||
| let counterparty_initial_bitcoin_tx = counterparty_trusted_tx.built_transaction(); | ||
|
|
||
| log_trace!(logger, "Initial counterparty tx for channel {} is: txid {} tx {}", |
There was a problem hiding this comment.
This is now redundant with the equivalent log in initial_commitment_signed. If we drop it here we can drop the counterparty_trusted_tx entirely here.
There was a problem hiding this comment.
Ah right. Missed this
There was a problem hiding this comment.
I also moved the decrement of cur_counterparty_commitment_transaction_number to the end of initial_commitment_signed.
There is a decent amount of shared code in these two methods so we make an attempt to share that code here by introducing the `InitialRemoteCommitmentReceiver` trait. This trait will also come in handy when we need similar commitment_signed handling behaviour for dual-funded channels.
dunxen
force-pushed
the
2024-10-dry-funding-created-signed
branch
from
e50e94c to
2db1aa2
Compare
|
Landing, we can address any more questions in followup(s)/issue(s). |
4 participants
There is a decent amount of shared code in these two methods so we make
an attempt to share that code here by introducing the
InitialRemoteCommitmentReceivertrait. This trait will also come inhandy when we need similar commitment_signed handling behaviour for
dual-funded channels.