Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Omit previous transaction for taproot inputs #18

Closed

Conversation

t-bast
Copy link

@t-bast t-bast commented Jan 3, 2024

We include the whole previous transaction in tx_add_input to protect against malleability attacks that affect segwit v0. Since lightning messages are limited to 65kB, this has an annoying consequence: if the input you're trying to spend comes from a large transaction, you cannot use it with the interactive-tx protocol.

This is happening in practice with coinjoins and payouts from mining pools. Since segwit v1+ is not affected by this malleability issue, we can relax that requirement and only send the corresponding txout.

If we want to allow this, it probably makes sense to include it in the spec before an official release of dual funding, since this isn't backwards-compatible and would otherwise require an additional feature bit.

We include the whole previous transaction in `tx_add_input` to protect
against malleability attacks that affect segwit v0. Since lightning
messages are limited to 65kB, this has an annoying consequence: if the
input you're trying to spend comes from a large transaction, you cannot
use it with the `interactive-tx` protocol.

This is happening in practice with coinjoins and payouts from mining
pools. Since segwit v1+ is not affected by this malleability issue,
we can relax that requirement and only send the corresponding `txout`.
@@ -199,13 +199,23 @@ This message contains a transaction input.
* [`u32`:`prevtx_vout`]
* [`u32`:`sequence`]

1. `tlv_stream`: `tx_add_input_tlvs`
2. types:
1. type: 0 (`prevtxout`)
Copy link
Author

@t-bast t-bast Jan 3, 2024

Choose a reason for hiding this comment

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

When setting this, we will set prevtx_len to 0 since it is part of the main fields of the message, which feels a bit hackish. An alternative design would be to remove prevtx from the main message fields and make it a TLV, which would make it more similar to prevtxout. I find this cleaner and more future-proof since other types of inputs won't include the whole prevtx (e.g. the previous funding output when using splicing), but it's more backwards-incompatible. But hey, it's now or never that we have the opportunity to make this cleaner, so it's worth studying that option!

@morehouse
Copy link

We include the whole previous transaction in tx_add_input to protect against malleability attacks that affect segwit v0.

What exactly is the malleability attack that this solves? And why is segwit v1 safe?

@t-bast
Copy link
Author

t-bast commented Jan 29, 2024

That's a good question, I remember that it was linked to the fact that segwit v1 included the amount and script of all inputs instead of just the one that is being signed in the transaction hash, but I can't figure out how to exploit it for segwit v0 if we don't transmit the whole previous transaction...I tried to dig into the git history of the dual funding PR but couldn't find it, and couldn't find any write-up explaining that.

@niftynei do you remember the attack? We definitely need to document it, and if it's not necessary for segwit v0, we really should drop the prevtx entirely!

@t-bast
Copy link
Author

t-bast commented Jan 30, 2024

Thanks @morehouse for challenging this, the attack is actually different than what I expected, which means that the change I'm proposing would be unsafe. I've put more details about this issue here: https://delvingbitcoin.org/t/malleability-issues-when-creating-shared-transactions-with-segwit-v0/497

I'm not sure yet how we should introduce the ability to omit the prevtx field. But I think it could be something like:

  • Alice knows that she will include a taproot input, which will protect her from malleability issues
  • Alice thus allows Bob to omit the prevtx from his tx_add_input messages
  • Similarly, if Bob plans to include at least one taproot input, he allows Alice to omit the prevtx from her tx_add_input messages

That kind of negotiation would probably use an odd TLV sent by each participant in open_channel2 and accept_channel2, which could be introduced without any backwards-compatibility issues. So we can probably ignore it for now and add it later.

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