Skip to content

smite-ir: add CreateFundingTransaction IR operation#90

Open
NishantBansal2003 wants to merge 1 commit into
morehouse:masterfrom
NishantBansal2003:funding-tx-ir
Open

smite-ir: add CreateFundingTransaction IR operation#90
NishantBansal2003 wants to merge 1 commit into
morehouse:masterfrom
NishantBansal2003:funding-tx-ir

Conversation

@NishantBansal2003
Copy link
Copy Markdown
Contributor

This commit adds the CreateFundingTransaction IR operation. For this, a new VariableType, FundingTransaction, has been added. This is required because we need the funding transaction after receiving the funding signed message so that we can broadcast the transaction and exchange channel_ready messages

Signed-off-by: Nishant Bansal <nishant.bansal.282003@gmail.com>
Copy link
Copy Markdown
Owner

@morehouse morehouse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good.

What's your vision for fitting CreateFundingTransaction into the funding flow?

I'm guessing we'll implement a BroadcastTx operation next that takes a FundingTransaction as an input? Does it make sense to have separate operations, or should we just combine into a single CreateAndBroadcastFundingTx? Or would it make sense to have a generic Transaction variable type instead of the more specific FundingTransaction?

Comment thread smite-ir/src/operation.rs
Self::DerivePoint => vec![VariableType::PrivateKey],
Self::ExtractAcceptChannel(_) => vec![VariableType::AcceptChannel],
Self::SendMessage => vec![VariableType::Message],
Self::CreateFundingTransaction => vec![
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: the ordering is inconsistent in some places -- in general CreateFundingTransaction comes right after ExtractAcceptChannel, but not here

Comment thread smite-ir/src/operation.rs
/// 1: `acceptor_funding_pubkey` (`Point`)
/// 2: `funding_satoshis` (`Amount`)
/// 3: `feerate_per_kw` (`FeeratePerKw`)
CreateFundingTransaction,
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: should we call it BuildFundingTransaction for consistency with similar operations?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, actually I did not use Build* because we defined Build operations as Build: construct a BOLT message from inputs, so I thought using it here might be confusing. Since this is more of a compute operation, I thought of using Create* instead. But I'm fine with using Build* or maybe some other name if you'd prefer that

Comment thread smite-ir/src/tests.rs
}

#[test]
fn validate_rejects_create_funding_transaction_with_wrong_input_type() {
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this test is hard to follow. I think it would be easier to understand if it were split into 4 tests with a helper function

Comment on lines +1251 to +1280
let instrs = vec![
Instruction {
operation: Operation::LoadPrivateKey([0x11; 32]),
inputs: vec![],
},
Instruction {
operation: Operation::DerivePoint,
inputs: vec![0],
},
Instruction {
operation: Operation::LoadPrivateKey([0x22; 32]),
inputs: vec![],
},
Instruction {
operation: Operation::DerivePoint,
inputs: vec![2],
},
Instruction {
operation: Operation::LoadAmount(10_000_000),
inputs: vec![],
},
Instruction {
operation: Operation::LoadFeeratePerKw(15_000),
inputs: vec![],
},
Instruction {
operation: Operation::CreateFundingTransaction,
inputs: vec![1, 3, 4, 5],
},
];
Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be generated by a helper function.

@NishantBansal2003
Copy link
Copy Markdown
Contributor Author

What's your vision for fitting CreateFundingTransaction into the funding flow?

So I was thinking we would have the following IR operations for the funding flow:

  • CreateFundingTransaction: Creates the funding tx. This is required for funding_created (to reference the funding outpoint) and later for broadcasting after funding_signed is exchanged.
  • BroadcastFundingTransaction: Broadcasts the funding tx after funding_signed.
  • CreateChannelConfig: Along with a ChannelConfig variable type, this would be constructed once after the open_channel <-> accept_channel flow and later used to reference the channel. This can later be extended for normal channel operations with something like LoadChannelConfigFromContext.
  • CreateInitialCommitmentState: Along with CommitmentState variable type, this would construct the initial commitment state. I still need to think more about the design here, but this could later be extended for normal channel operations via something like an AdvanceCommitmentState IR operation. The CommitmentState could also later be loaded through something like LoadInitialCommitmentStateFromContext after initializing normal channel scenario.
  • BuildFundingCreated: Builds the funding_created message using ChannelConfig, CommitmentState.
  • RecvFundingSigned: Uses ChannelConfig, CommitmentState to verify the received signature, and the output_type would be ChannelId , so there is no need for a separate ExtractFundingSigned operation (We could have ExtractFundingSigned operation purely to extract the ChannelId for consistency, WDYT here?)
  • BuildChannelReady
  • RecvChannelReady

I'm guessing we'll implement a BroadcastTx operation next that takes a FundingTransaction as an input? Does it make sense to have separate operations, or should we just combine into a single CreateAndBroadcastFundingTx? Or would it make sense to have a generic Transaction variable type instead of the more specific FundingTransaction?

I mean, we can combine them, and initially I was thinking the same thing, but that would be less aligned with the LN flow, since there the broadcast happens only after funding_signed. Still, I’m fine with it if you want to keep it that way.

I created FundingTransaction because I need both the transaction itself and the vout to be used in funding_created, otherwise, we could simply use Transaction. But I'm not fully sure here -- if in the future we always create the tx output ourselves, then we could maybe just hardcode the vout to 0. WDYT?
With that, we could have something like either of these:

  • CreateAndBroadcastFundingTx: creates the transaction, broadcasts it, and returns the transaction as the output_type (Or we could simply merge this into the CreateChannelConfig case mentioned above)
  • CreateTx and BroadcastTx separately

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants