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:
|
1 similar comment
|
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 our_funding_outputs_weight = funding_outputs.iter().fold(0u64, |weight, out| { | ||
| weight.saturating_add(get_output_weight(&out.tx_out().script_pubkey).to_wu()) | ||
| weight.saturating_add(get_output_weight(&out.script_pubkey).to_wu()) |
There was a problem hiding this comment.
It looks like we're not considering the total output amount to see if we can afford the fee. We need to subtract it from total_input_satoshis and that amount must be enough to cover our contribution and fees.
There was a problem hiding this comment.
Updated along with the test.
| 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.
| ); | ||
| (OutPoint { txid, vout }, prev_output.clone(), local_owned) | ||
| } | ||
|
|
There was a problem hiding this comment.
The test case description says "makes fees insufficient for initiator" but the implementation adds 1 to the adjusted amount rather than subtracting 1. To properly make the fees insufficient, consider subtracting 1 instead:
// makes fees insufficient for initiator
shared_output_a: generate_funding_txout(
amount_adjusted_with_p2wpkh_fee - 1,
amount_adjusted_with_p2wpkh_fee - 1,
),This would ensure the test correctly verifies the behavior when fees are insufficient.
| // makes fees insufficient for initiator | |
| shared_output_a: generate_funding_txout( | |
| amount_adjusted_with_p2wpkh_fee - 1, | |
| amount_adjusted_with_p2wpkh_fee - 1, | |
| ), | |
Spotted by Diamond
Is this helpful? React 👍 or 👎 to let us know.
There was a problem hiding this comment.
The code is correct, but the comment in the test is misleading. Will make a fix shortly.
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.
| ); | ||
| 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.
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.