Abstract OnchainTxHandler to its own trait

[dunxen]

Requirement for #1621

[dunxen]

Hey @ariard (and anyone with some ideas :)), I've been working on this for a while and it seems a bit tricky abstracting some of the behaviour out into a trait, especially when it comes to the methods with generics such as

rust-lightning/lightning/src/chain/onchaintx.rs

Line 381 in 6772609

fn generate_claim_tx<F: Deref, L: Deref>(&mut self, cur_height: u32, cached_request: &PackageTemplate, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L) -> Option<(Option<u32>, u64, Transaction)>

rust-lightning/lightning/src/chain/onchaintx.rs

Line 419 in 6772609

pub(crate) fn update_claims_view<B: Deref, F: Deref, L: Deref>(&mut self, txn_matched: &[&Transaction], requests: Vec<PackageTemplate>, conf_height: u32, cur_height: u32, broadcaster: &B, fee_estimator: &LowerBoundedFeeEstimator<F>, logger: &L)

and so forth.

We can't make that trait object-safe because of these methods. We need object-safety because the methods of PackageTemplate that would accept object traits. One idea I had was to split into two traits a base handler supertrait, and a subtrait with the non-object-safe methods. This runs into quite a bit of duplication and seems a bit awkward though.

Were there any other ideas for going about this at the time?

[TheBlueMatt]

I'm not sure I understand why OnchainTxHandler needs to be "abstracted" to have dual-funded channels. AFAIU the dual-funding stuff is negotiated all at the pre-funding point, and from there its basically a normal channel with a non-0 initial balance. I'd have imagined the API would basically be changes to the channel logic but that it wouldn't touch the chain package at all.

[dunxen]

I'm not sure I understand why OnchainTxHandler needs to be "abstracted" to have dual-funded channels.

Right, so there have probably been some architectural changes since #606 (comment).

I think the main idea was to have dual-funding under the same umbrella as splicing which is changing onchain stuff over channel operation (see same link above).

Although, definitely looking for the any better alternatives. For dual-funding alone I don't see any need to make it a trait, yeah.

[TheBlueMatt]

Right, so for splicing, I'm still not sure we want to make OnchainTxHandler abstract. We could either update all the ChannelMonitor-and-down things to support multiple funding_txos, or we could just construct a completely parallel ChannelMonitor for the new "funding txo" post-splice. I think the latter would be simpler, and lets us remove all the stale state once we get there. Further, we could reuse the same infrastructure for, eg, anchor-upgrades once we do the upgrade protocol stuff.

[dunxen]

Ok well that would simplify a lot right now :D

[dunxen]

Closing to just get this out of the way of other things.

[ariard]

Right, so for splicing, I'm still not sure we want to make OnchainTxHandler abstract. We could either update all the ChannelMonitor-and-down things to support multiple funding_txos, or we could just construct a completely parallel ChannelMonitor for the new "funding txo" post-splice. I think the latter would be simpler, and lets us remove all the stale state once we get there. Further, we could reuse the same infrastructure for, eg, anchor-upgrades once we do the upgrade protocol stuff.

While the dual-funding specification includes RBF as a fee-bumping strategy, it doesn't preclude to use CPFP as another one. As CPFP might be far more realistic in face of dual-funding with a set of mobile clients, ideally we could register the bumping request as another PackageSolvingData::MultiPartyOutputs, and as such keep an unified pipeline for fee-bumping/rebroadcast/reorg-safety flow.

That said, I don't think we need to achieve such OnchainTxHandler abstraction now for our dual-funding support, we can think about it later once the required changes are landed in the channel logic. And yes I also agree on abstracting the internal of ChannelMonitor to add check_splicing_utxo-kind of methods (or go a step further and abstract OnchainGenericContracts to cover use-cases like peerswap, DLC, splicing flow while relying on the same high-level ChainMonitor/ChannelMonitor infrastructure).