add TxBuilder trait

[zoedberg]

As discussed on discord, I'm working on a trait to allow custom building of commitment, HTLC and closing TXs. This trait will allow us to drop a lot of code from our fork of rust-lightning needed to support RGB channels in RLN.

This PR is in draft because I would first like to receive a concept ACK and I'm not sure the approach I'm using is the desired one.

Still missing:

• trait method for custom closing TX
• trait method for custom HTLC TX
• code lint and format
• docstrings (currently set to TODO)
• tests fixes

[TheBlueMatt]

Rather than making parts of Channel public, can we instead pass what we need in here, and maybe have a per-channel object similar to the way we do it with KeysInterface? Basically some top-level trait A that exists to give us new instances of some channel-specific trait B, and then we pass A information about the channel which gives us a new instance of B which we then call build_commitment_transaction on.

[zoedberg]

I'm not sure what's the issue in having some parts of Channel public. If the reason is that you're worried a developer can "hurt himself" by accessing parts that should not be touched then I'm not sure the proposed change (which complicates code a lot) is worth it, since IMO a developer using rust-lightning can already hurt himself in many different ways (especially with this trait) and we cannot protect them from an innapropriate library usage.
Anyway, if you are convinced it's important to not make public some parts of Channel I will try the proposed approach. I'm not sure what you are proposing though, I cannot find any KeysInterface, just a TestKeysInterface. @TheBlueMatt are you sure you've referenced the correct trait?

[TheBlueMatt]

Its less about whether someone can "hurt themselves" but rather about providing a public API which is clean, easy to use, hard to misuse (without trying to misuse, that is) and well-documented. None of the stuff in channel.rs is that, and in most cases is none of those :).

Of course if there are parts of channel.rs that meet that bar, there's nothing wrong with reusing them, but in general we assume that developers using LDK have a lot going on in their lives, and they don't have time to learn an inscrutable internal LDK API, so we want to expose the cleanest thing possible so that at least their task takes a bit less of their precious time.

I cannot find any KeysInterface

Oops, sorry, old name for it, its SignerProvider now.

[zoedberg]

I've gave a look at SignerProvider but failed to understand what you're proposing. In your suggestion:

Basically some top-level trait A that exists to give us new instances of some channel-specific trait B, and then we pass A information about the channel which gives us a new instance of B which we then call build_commitment_transaction on.

the TxBuilder trait I've added should be trait B, right? Could you provide a more detailed suggestion please?

in general we assume that developers using LDK have a lot going on in their lives, and they don't have time to learn an inscrutable internal LDK API

I'm convinced that 99% of LDK developers will not need to change commitment/HTLC/closing transactions and those who need to will appreciate to have full access to all channel related data (and will need to partially understand internals anyway to avoid making big mistakes).

Anyway, I'll be happy to implement this the way you prefer if I understand what you're proposing.

To give more context, this is how I'm using this trait in RLN:

pub(crate) struct RgbTxBuilder {
    ldk_data_dir: PathBuf,
}

impl TxBuilder for RgbTxBuilder {
    fn build_commitment_transaction<'a, SP: Deref, L: Deref>(
        &self,
        channel_context: &'a ChannelContext<SP>,
        commitment_number: u64,
        keys: &TxCreationKeys,
        local: bool,
        generated_by_local: bool,
        logger: &L,
    ) -> CommitmentStats<'a>
    where
        SP::Target: SignerProvider,
        L::Target: Logger,
    {
        let mut commitment_stats = channel_context.build_commitment_transaction(
            commitment_number,
            keys,
            local,
            generated_by_local,
            logger,
        );

        let channel_id = &channel_context.channel_id;

        if is_channel_rgb(channel_id, &self.ldk_data_dir) {
            color_commitment(
                channel_id,
                &mut commitment_stats.tx,
                !local,
                &self.ldk_data_dir,
            );
        }
        
        commitment_stats
    }
}

Therefore we get the ChannelContext to:

• call its build_commitment_transaction method
• get its channel_id field

And we return the CommitmentStats object to allow simple replacement of calls to build_commitment_transaction, for example:

- let commitment_stats = self.context.build_commitment_transaction(self.context.holder_commitment_point.transaction_number(), &keys, true, true, logger);
+ let commitment_stats = tx_builder.build_commitment_transaction(&self.context, self.context.holder_commitment_point.transaction_number(), &keys, true, true, logger);

Without introducing another trait I see a possible improvement: we could pass tx_builder to the build_commitment_transaction method, and where we now call CommitmentTransaction::new_with_auxiliary_htlc_data we could call the method from the tx_builder. On the DefaultTxBuilder this new method will call CommitmentTransaction::new_with_auxiliary_htlc_data, while on our implementation we would call that and then the color_commitment method.
This way we would avoid making public ChannelContext and CommitmentStats. @TheBlueMatt What do you think about this?

[EDITED]: because of #3169 (comment)

[zoedberg]

@TheBlueMatt I've noticed an inaccuracy in my previous message, we don't need to take the channel_transaction_parameters.funding_outpoint field from the ChannelContext since we should be able to retrive that from the "vanilla" transaction input.

[TheBlueMatt]

the TxBuilder trait I've added should be trait B, right? Could you provide a more detailed suggestion please?

Basically something like this:

pub trait TxBuilderProvider {
  type Builder: TxBuilder;
  pub fn get_builder(&self, params: ChannelTransactionParameters, more_stuff_we_have...) -> Builder;
}
pub trait Builder {
  pub fn build_commitment_tx(channel_state: ChannelState) -> Result<...>
}

[zoedberg]

@TheBlueMatt Sorry but I'm still having a hard time understanding your proposal.

1. What should be the role of ChannelState in build_commitment_tx?
2. Why get_builder should get the ChannelTransactionParameters?
3. Where should the ChannelContext be used? Should we implement the TxBuilderProvider for the ChannelContext?
4. Since in RLN we need to change the CommitmentStats, which is the output of build_commitment_transaction and is private, how can the proposed solution work without making CommitmentStats public?

[TheBlueMatt]

What should be the role of ChannelState in build_commitment_tx?

A struct describing the HTLCs to include in the commitment transaction, the balances of different parties, the feerate, and anything else we need.

Why get_builder should get the ChannelTransactionParameters?

Because it should be aware of the type of channel, the public keys/funding which parameterize the channel.

Where should the ChannelContext be used? Should we implement the TxBuilderProvider for the ChannelContext?

I don't believe ChannelContext should be in the public API

Since in RLN we need to change the CommitmentStats, which is the output of build_commitment_transaction and is private, how can the proposed solution work without making CommitmentStats public?

I believe CommitmentStats should be made public.

[zoedberg]

What should be the role of ChannelState in build_commitment_tx?

A struct describing the HTLCs to include in the commitment transaction, the balances of different parties, the feerate, and anything else we need.

I assume you are not referring to the already existing ChannelState enum but to a new struct I should add containing the info needed to construct the commitment TX. Am I correct?

Why get_builder should get the ChannelTransactionParameters?

Because it should be aware of the type of channel, the public keys/funding which parameterize the channel.

The ChannelContext::build_commitment_transaction is not getting the ChannelTransactionParameters, therefore with this suggestion I'm assuming that in the trait's default implementation we should not just call ChannelContext::build_commitment_transaction but also retrieve the keys (e.g. currently retrieved by ChannelContext::build_remote_transaction_keys() or ChannelContext::build_holder_transaction_keys(), that internally use ChannelTransactionParameters). Could you please tell me if this assumption is correct?

Where should the ChannelContext be used? Should we implement the TxBuilderProvider for the ChannelContext?

I don't believe ChannelContext should be in the public API

Without making the ChannelContext public I'm failing to understand how in RLN we can retrieve a vanilla commitment TX (on which we need to add an output to move RGB assets). Should I move the method from ChannelContext::build_commitment_transaction to an utility method external to the ChannelContext?

Since in RLN we need to change the CommitmentStats, which is the output of build_commitment_transaction and is private, how can the proposed solution work without making CommitmentStats public?

I believe CommitmentStats should be made public.

Great, this means that I just need to understand how to avoid making the ChannelContext public and how to use the suggested traits (TxBuilderProvider and Builder).

P.S. Sorry for asking many questions but, since this may require a lot of changes to the code, I would prefer to be sure that I'm going to implement the desired solution.

[TheBlueMatt]

I assume you are not referring to the already existing ChannelState enum but to a new struct I should add containing the info needed to construct the commitment TX. Am I correct?

Ah, apologies, yes, I am referring to some new struct.

The ChannelContext::build_commitment_transaction is not getting the ChannelTransactionParameters, therefore with this suggestion I'm assuming that in the trait's default implementation we should not just call ChannelContext::build_commitment_transaction but also retrieve the keys (e.g. currently retrieved by ChannelContext::build_remote_transaction_keys() or ChannelContext::build_holder_transaction_keys(), that internally use ChannelTransactionParameters). Could you please tell me if this assumption is correct?

That would make sense to me, yea!

Without making the ChannelContext public I'm failing to understand how in RLN we can retrieve a vanilla commitment TX (on which we need to add an output to move RGB assets). Should I move the method from ChannelContext::build_commitment_transaction to an utility method external to the ChannelContext?

Yes, making build_commitment_transaction a public externally-visible utility makes sense to me. In general, however, we'll want to have a public default implementation of the new building trait anyway, which could be used by y'all to get the "default" commitment tx and then build on it.

P.S. Sorry for asking many questions but, since this may require a lot of changes to the code, I would prefer to be sure that I'm going to implement the desired solution.

No, please, better to ask questions and make sure we're on the same page before you go write a ton of stuff and we end up concluding it wasn't the right direction.

[TheBlueMatt]

Closing as abandoned by later work from @tankyleo, eg #4026.