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
|
@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.72% 90.10% +0.38%
==========================================
Files 164 165 +1
Lines 133359 132400 -959
Branches 133359 132400 -959
==========================================
- Hits 119651 119296 -355
+ Misses 11037 10728 -309
+ Partials 2671 2376 -295 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Review comments addressed, some minor (doc) changes applied. |
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.
As a reader of the method though, it seems clearer to keep all the additional weight tracking when we're the initiator in one place.
|
🔔 12th Reminder Hey @dunxen! This PR has been waiting for your review. |
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.
I ended up moving this since doing so is more consistent with how the shared output is included in the weight. That is, shared output weight is not included in our_funding_outputs_weight, so including the shared input in our_funding_inputs_weight is not consistent.
| let shared_funding_output = TxOut { | ||
| value: Amount::from_sat(funding.get_value_satoshis()), | ||
| script_pubkey: funding.get_funding_redeemscript().to_p2wsh(), | ||
| }; | ||
|
|
||
| let interactive_tx_constructor = Some(InteractiveTxConstructor::new( |
There was a problem hiding this comment.
Pre-existing, but it looks like we shouldn't be initializing a constructor now, and should be relying on begin_interactive_funding_tx_construction to do it instead. Let's leave it for a follow-up.
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.
lightning/src/ln/interactivetxs.rs
Outdated
| ); | ||
| let tx_common_fields_fee = | ||
| fee_for_weight(TEST_FEERATE_SATS_PER_KW, TX_COMMON_FIELDS_WEIGHT); | ||
|
|
||
| let amount_adjusted_with_p2wpkh_fee = | ||
| 1_000_000 - p2wpkh_fee - outputs_fee - tx_common_fields_fee; | ||
| 1_000_000 - p2wpkh_fee - outputs_fee - tx_common_fields_fee + 1; |
There was a problem hiding this comment.
It isn't clear to me why we add one here and also whenever we use this variable below.
There was a problem hiding this comment.
Looks like a rounding issue somewhere? Ideally we try to fix it if possible as opposed to hiding it this way.
There was a problem hiding this comment.
Pushed a commit fixing a rounding error after pairing with @wpaulino.
Instead of calculating the fee for the entire weight contributed by the counterparty, each portion of the weight was calculated individually before summed. That caused a rounding error that is avoided by summing the weights first.
| return Err(AbortReason::UnexpectedPrevTx); | ||
| } | ||
| if let Some(shared_funding_input) = &self.shared_funding_input { | ||
| if self.inputs.values().any(|input| matches!(input.input, InputOwned::Shared(_))) { |
There was a problem hiding this comment.
The check here verifies if any shared input already exists, but it doesn't specifically verify that the outpoint matches the current shared funding input. This could potentially allow multiple different shared inputs to be added, which appears to contradict the error handling for DuplicateFundingInput. Consider checking against the specific outpoint of the shared funding input instead:
if self.inputs.values().any(|input|
matches!(input.input, InputOwned::Shared(shared)) &&
shared.input.previous_output == shared_funding_input.input.previous_output
) {
return Err(AbortReason::DuplicateFundingInput);
}| if self.inputs.values().any(|input| matches!(input.input, InputOwned::Shared(_))) { | |
| if self.inputs.values().any(|input| | |
| matches!(input.input, InputOwned::Shared(shared)) && | |
| shared.input.previous_output == shared_funding_input.input.previous_output | |
| ) { |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
There is only ever one shared input, and the check for it immediately follows.
|
Merging with one approval since @jkczyz took over and his is implicit. |
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.