Insert channel funding outputs into Wallet #726
Conversation
|
👋 Thanks for assigning @jkczyz as a reviewer! |
benthecarman
force-pushed
the
add-chan-to-wallet
branch
2 times, most recently
from
091e391 to
d7f8bc9
Compare
There was a problem hiding this comment.
Makes sense to me. Do we think this is worth a backport for 0.7 or are we fine waiting for 0.8 with shipping this?
The different cases in the event handler could use a comment or two to explain when we'd land on which side of the if/else clauses there. Also, we really need to be careful with just replaying events in cases where we don't expect immediate resolution of the error on retry, as we'd otherwise just spin and block any other event processing. This could also use better test coverage to show why we need this change in the first place.
Besides that, also pinging @jkczyz as a second reviewer on this.
src/event.rs
Outdated
| self.logger, | ||
| "Failed to find channel info for pending channel {channel_id} with counterparty {counterparty_node_id}" | ||
| ); | ||
| return Err(ReplayEvent()); |
There was a problem hiding this comment.
This would induce an infinite loop in event handling, no? Rather than doing this, we probably need to just log the error for now, and abort the flow when we can with 0.3? Also, would it make sense to add a debug_assert here, given that we expect this always to be avaialble? (same below)
There was a problem hiding this comment.
The error below can only happen on a persistence failure, elsewhere in the event handling we replay on those errors so I think it makes sense to keep as is.
For this one, it should always be available so I will just change to a debug_assert
| }, | ||
| } | ||
| } else { | ||
| log_info!( |
There was a problem hiding this comment.
In what case would we end up in this else branch, and would we need to do something related to the redeemscript, too?
There was a problem hiding this comment.
According to the docs, the funding_txo "Will be None if the channel's funding transaction reached an acceptable depth prior to version 0.2". Since this was also added in 0.2, the redeem script would also be None so there shouldn't be anything to add. Either way, we need the funding txo to be able to insert so we can't add it.
| node_b.splice_in(&user_channel_id_b, node_a.node_id(), 4_000_000).unwrap(); | ||
|
|
||
| expect_splice_pending_event!(node_a, node_b.node_id()); | ||
| let txo = expect_splice_pending_event!(node_a, node_b.node_id()); |
There was a problem hiding this comment.
These test changes pass on main. Can you add coverage for the cases where the previous approach would lead to inaccurate fee estimations?
There was a problem hiding this comment.
The bug/issue only exists with bitcoind syncing so to do so, we'd have to change the test to sync with bitcoind rpc. Is that wanted?
| } | ||
| } | ||
|
|
||
| impl UniffiCustomTypeConverter for ScriptBuf { |
There was a problem hiding this comment.
nit: While this should be fine for now, we generally try to get away from the UniffiCustomTypeConverter approach, and instead convert more and more objects to proper interfaces.
When doing a splice, to properly calculate fees we need the channel's funding utxo in our wallet, otherwise our wallet won't know the channel's original size. This adds the channel funding txo on ChannelReady events and modifies the splicing test to make sure we can calculate fees on splice-ins.
Exposes the funding_redeem_script that LDK already exposes
benthecarman
force-pushed
the
add-chan-to-wallet
branch
from
d7f8bc9 to
b2c3793
Compare
|
🔔 1st Reminder Hey @jkczyz! This PR has been waiting for your review. |
|
🔔 2nd Reminder Hey @jkczyz! This PR has been waiting for your review. |
3 participants
When doing a splice, to properly calculate fees we need the channel's funding utxo in our wallet, otherwise our wallet won't know the channel's original size. This adds the channel funding txo on ChannelReady events and modifies the splicing test to make sure we can calculate fees on splice-ins.