Add Shared Input support in interactive TX construction #3842
Conversation
|
👋 Thanks for assigning @wpaulino as a reviewer! |
1 similar comment
1 similar comment
I managed to do it: first I refactored the shared output support as discussed, then added shared input support in a similar style. Although this PR does not include spicing, I also checked the changes with splicing (shared input is used there). |
optout21
force-pushed
the
shared-input
branch
5 times, most recently
from
166e364 to
b2e0da8
Compare
| // If there is a shared input, account for it, | ||
| // and for the initiator also consider the fee | ||
| if let Some(shared_input) = shared_input { | ||
| total_input_satoshis = total_input_satoshis.saturating_add(shared_input); |
There was a problem hiding this comment.
Is this supposed to be the full channel value of the shared input, or just the locally owned portion of it?
There was a problem hiding this comment.
The locally owned portion, as this calculation is for ensuring that the contribution is enough to cover the (locally owned) outputs and fees.
There was a problem hiding this comment.
I have some doubts about the possible rounding errors due to the msat->sat conversion.
There was a problem hiding this comment.
The msat->sat conversion shouldn't be an issue, we always round down to the nearest satoshi. We should document the shared_input argument in the method docs though.
|
@wpaulino thanks for the comments. I will process them (I've re-requested review accidentally, please ignore that). |
1 similar comment
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3842 +/- ##
==========================================
- Coverage 89.70% 89.68% -0.03%
==========================================
Files 164 164
Lines 133359 133563 +204
Branches 133359 133563 +204
==========================================
+ Hits 119632 119787 +155
- Misses 11052 11086 +34
- Partials 2675 2690 +15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
optout21
force-pushed
the
shared-input
branch
3 times, most recently
from
21e5e36 to
bb322a8
Compare
|
Review comments addressed. Keeping the fixup commits separately for now, but in the end I will just squash into a single commit. |
1 similar comment
This simplifies tracking separately the expected and actual shared output. In the initiator case, we can just provide the shared output separately, instead of including it within other outputs, and marking which one is the output. We can use the same field for the intended shared output in the initiator case, and the expected one in the acceptor case.
In interactivetxs, add support for shared inputs, similar to shared outputs. A shared input is optional, and is used in case of splicing to add the current funding as an input.
|
Squashed, rebased, new comment addressed with a fix. Ready for review. |
1 similar comment
| // - the peer's paid feerate does not meet or exceed the agreed feerate (based on the minimum fee). | ||
| self.check_counterparty_fees(remote_inputs_value.saturating_sub(remote_outputs_value))?; | ||
|
|
||
| let shared_funding_output = self.shared_funding_output.clone(); | ||
| let constructed_tx = ConstructedTransaction::new(self); |
There was a problem hiding this comment.
Can we have new return an error and check the conditions below as part of it?
| // - the `prevtx` and `prevtx_vout` are identical to a previously added | ||
| // (and not removed) input's | ||
| return Err(AbortReason::PrevTxOutInvalid); | ||
| if let Some(shared_funding_input) = &self.shared_funding_input { |
There was a problem hiding this comment.
We need to abort if prevtx is also set here, and below in the else case if shared_funding_input is set
| } else { | ||
| // The receiving node: | ||
| // - MUST fail the negotiation if: | ||
| // - `prevtx_vout` is greater or equal to the number of outputs on `prevtx` | ||
| return Err(AbortReason::PrevTxOutInvalid); | ||
| } | ||
| if let Some(prevtx) = &msg.prevtx { |
There was a problem hiding this comment.
We can save one indent by collapsing these into a single line
| added_by: AddingRole::Remote, | ||
| input, | ||
| }); | ||
| if !self.prevtx_outpoints.insert(prev_outpoint) { |
There was a problem hiding this comment.
Don't think it matters here, but best to only insert the entry above after we know it's not a duplicate
| } else { | ||
| if let Some(prevtx) = &msg.prevtx { |
There was a problem hiding this comment.
Same here, can save one indent
| input: TxIn, | ||
| prev_tx: Option<TransactionU16LenLimited>, |
There was a problem hiding this comment.
Why is this optional? This is always required for a non-shared input.
| pub(crate) struct InteractiveTxInput { | ||
| serial_id: SerialId, | ||
| added_by: AddingRole, | ||
| input: InputOwned, |
There was a problem hiding this comment.
We really need to use a different type here so we're not tracking prevtx for Single inputs
lightning/src/ln/interactivetxs.rs
Outdated
| if is_initiator { | ||
| our_funding_inputs_weight = | ||
| our_funding_inputs_weight.saturating_add(estimate_input_weight(output).to_wu()); | ||
| } else { | ||
| return Err(AbortReason::PrevTxOutInvalid); | ||
| our_funding_inputs_weight.saturating_add(P2WSH_INPUT_WEIGHT_LOWER_BOUND); |
There was a problem hiding this comment.
This should probably go in the is_initiator check below instead
| let txid = prev_funding_tx.compute_txid(); | ||
| let value = prev_funding_tx.output.get(vout as usize).unwrap().value.to_sat(); | ||
| if local_owned > value { | ||
| println!("Warning: local owned > value for shared input, {} {}", local_owned, value); |
There was a problem hiding this comment.
We shouldn't print in tests, just make this an assertion instead?
| // If there is a shared input, account for it, | ||
| // and for the initiator also consider the fee | ||
| if let Some(shared_input) = shared_input { | ||
| total_input_satoshis = total_input_satoshis.saturating_add(shared_input); |
There was a problem hiding this comment.
The msat->sat conversion shouldn't be an issue, we always round down to the nearest satoshi. We should document the shared_input argument in the method docs though.
In interactive TX construction, add support for shared input:
Additionally, the
prevtxfield of theTxAddInputmessage is changed to Optional, as it should not be set for the shared input (it cannot, as the full funding transaction is not stored on the acceptor side) (spec discussion: lightning/bolts#1160 (comment))To be used by splicing, see #3736 .
Note: this PR does not include splicing negotiation.