Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Proposed v0 DLC TLV messages and Deterministic Fee Computation #81
Proposed v0 DLC TLV messages and Deterministic Fee Computation #81
Changes from 13 commits
abdca3d
630d3fb
c6850c9
23a3f10
c33fb47
c52d458
1591ba8
18be1e9
5bb66db
6c518c1
367c32e
07b8d66
3b88814
76b7c65
7a7f640
229c597
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even if it's temporary can you assign them in the LN's messages waiting rooms : lightning/bolts#716 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I'll wait a while longer on this until we are sure we will actually be using LN message format long-term (e.g. does #96 work as an approach) before adding to here but I will do so if it becomes clear we won't be using an incompatible variant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A side-note, we should make mandatory in the Oracle spec-side to verify public key ownership to avoid someone impersonating the oracle and thus blocking the state of your contract.
Though I don't know the state of oracle discovery, if we rely on common public key infrastructure.
cc @LLFourn
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#97
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't understand what this meant. How can you impersonate the oracle?
Note I made a comment here: #61 about how I don't think it makes sense to send
oracle_nonce
. If you do you the oracle must sign it along with the event data it is associated with. Perhaps this is what you are getting at.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is quite confusing, in
contract_info_v0
we have a single TLV record for many outcomes. Here we have one TLV record for each input.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because contract information follows a uniform type (all outcomes fall under the same version, and if they don't a future version will have separate TLV records) whereas different inputs are independent (e.g. taproot input could be
funding_input_v1
and not haveprev_tx
at the same time as another input isv0
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in theory not needed, as a parser knows how big is a transaction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I assume this is to allow clients which don't have transaction parser to make external calls to one after having parsed the transaction. Regardless of why I have made this record as close to this as possible: https://github.com/niftynei/lightning-rfc/pull/1/files#diff-3369c5aa1774fef2ff1e246979f223eaR169
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing the
script
field.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure what you are referring to? If you're talking about p2sh that is the redeem script below and if not then you definitely must not include a witness script when giving your funding input as funding signatures must be delayed until after CET signatures are received.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In that case, I don't understand the purpose of
max_witness_len
.I can see what it is, but why is it needed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For deterministic fee computation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a deal breaker for me.
In BTCPay Server, we don't have the prevtx, because if the user restored his wallet, it is through scantxoutsetinfo and we work on pruned node, so no way to get back this data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this requirement can be dropped, because the signatures already cover the input value (it's a segwit feature).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well it was not needed in the non-TLV version of the spec, why has it been added?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a receiver you have to verify that announced inputs aren't malleable ones, how do you combine this with pruning mode ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@devrandom it covers your own input value but not the others https://github.com/bitcoin/bips/blob/master/bip-0143.mediawiki#specification
If you don't include the full prev tx they can give you a valid txid pointing to a non-segwit address and convince you it is segwit (because you're trusting them by not requiring the full prevtx) once you sign the funding tx, they can include a script signature you weren't expecting, your signatures are still valid but the txid changes and all CETs and refund tx become invalid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh and I think I misunderstood, if I now understand correctly, with this new versioned approach you should be able to be trustless if your counter-party accepts txid+vout instead of prevtx (if they are a full node for example) as the issue for you is getting your own prevtx, not validating the counter party's if you are given a prevtx right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nkohen so I searched a bit:
Correct.
Stepping back a bit, I think the best reason for allowing witness_utxo in the protocol, is not about making it easier for wallets with the full utxo set. But to prepare taproot. Once we have taproot, the non_witness_utxo won't be needed in any case. So I think it makes sense to allow it in the protocol for this reason.
On my side, my implementation of this protocol I would allow witness_utxo even for segwit v0 as we have the utxo set, but still try to add non_witness_utxo, if possible, to offer/accept.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great news :) Lmk if there's anything I can do to help. And also to clarify, you're cool with moving forward with prevtx and adding a non-prevtx version in the near future (potentially the subsequent PR to this one)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, seems good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#98
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Christewart Can we tag as a V0.2 to reject malleable witness scripts ? I don't think Miniscript compilers have the interface yet but that something to consider in the future.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I'm not sure this is a feasible constraint seeing as one party never sees the other's witnesses before they see them on-chain. Is there any reason witness malleability is an issue in this context as it doesn't invalidate future txs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes witness malleability is a pinning vector and as such you can prevent confirmation of the funding transaction and escape the contract in case of non-favorable outcome.
In theory you can parse a
witnessScript
and decide if a solving witness is malleable. But as pointed IIRC Miniscript isn't supporting this.