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

Restrictions on Funding Change Script Public Keys #53

Closed
nkohen opened this issue Aug 6, 2020 · 12 comments · Fixed by #137
Closed

Restrictions on Funding Change Script Public Keys #53

nkohen opened this issue Aug 6, 2020 · 12 comments · Fixed by #137
Labels
contract-negotiation question Further information is requested
Milestone

Comments

@nkohen
Copy link
Contributor

nkohen commented Aug 6, 2020

Do we want any restrictions on what change Script Public Keys are allowed to be?

In the current state of the specification (see #51) both parties pay equal fees for the funding transaction so I'm currently leaning towards starting out with the restriction that specifying the length of the SPK requires only one byte (i.e. the var_int for the spk length takes up only one byte of the transaction).

In the future we will likely move to a model where each participant pays the fees for their own inputs and outputs rather than sharing this burden, at which point perhaps this restriction should be lifted.

@nkohen nkohen added this to the v0.1 milestone Aug 6, 2020
@nkohen nkohen added the question Further information is requested label Aug 6, 2020
@ariard
Copy link
Contributor

ariard commented Aug 10, 2020

Is there any reason to have var_int superior to one if we restrain to Segwit outputs only ? DLC is a new sytem, so less concern with being compatible with pre-Segwit software ?

@nkohen
Copy link
Contributor Author

nkohen commented Aug 13, 2020

So currently I see no reason to restrict things only to Segwit (e.g. what if you're using DLC software on top of an older hardware wallet for cold stuff which wants to do P2SH or P2WPKH or something).

@nkohen
Copy link
Contributor Author

nkohen commented Aug 13, 2020

It seems like it comes at very little cost and I only have the var_int restriction right now to make things simpler. Once parties are paying for their own change output fees then I can't think of a good reason to really have any restriction (but I could be wrong).

@benthecarman
Copy link
Contributor

Having a mix of p2wpkh and p2sh(p2wpkh) inputs can reduce privacy. More than likely if there is a mix, one user has all p2wpkh inputs and the other use has all the p2sh(p2wpkh) inputs. However, I think that shouldn't be restricted by the protocol, if a user wants to protection from that they should be able to configure their wallet to do so.

@Christewart
Copy link
Contributor

Christewart commented Aug 16, 2020

My argument for restricting it is for simplifying the number of scripts library developers need to support when building on the DLC protocol. I don't think this is a concern for us per-se in bitcoin-s as we have pretty extensive Script support, but other library developers may not and may not want to spend time on supporting eccentric change output scripts.

An example of real world problem of this the relay policy of bitcoin core nodes, I don't think bitcoin core will relay txs with non standard scripts. 🤷‍♂️

@benthecarman
Copy link
Contributor

An example of real world problem of this the relay policy of bitcoin core nodes, I don't think bitcoin core will relay txs with non standard scripts. 🤷‍♂️

That makes sense, we can say only standard scripts I think that is just
P2PK, Raw Multisig. P2PKH, P2SH, P2WPKH, P2WSH

@Christewart
Copy link
Contributor

I agree with those minus P2PK (pre-taproot any way) and raw multisig as I don't think bitcoin core will relay those? I'm not 100% on that though.

@benthecarman
Copy link
Contributor

@nkohen
Copy link
Contributor Author

nkohen commented Aug 17, 2020

My argument for restricting it is for simplifying the number of scripts library developers need to support when building on the DLC protocol.

I'm quite confused, why on earth would a library need to support an SPK type in order to include it in a change output?

@nkohen
Copy link
Contributor Author

nkohen commented Aug 17, 2020

I think I might understand ... they must only implement or call to something like what Ben is talking about because otherwise relay issues (but I still think calling this "support for SPK types" is a but much).

I'm reading the new dual-funding proposal and it looks like they have this requirement on outputs (checking standard-ness)

@benthecarman
Copy link
Contributor

Yeah, only restriction should be is the output standard. That should cover every functionality a user could want but prevent people from putting a maliciously non-standard script so the tx isn't relayed

@nkohen
Copy link
Contributor Author

nkohen commented Sep 8, 2020

Hm one consideration I hadn't though of before RE standardness is that if we want to use address serializations then we need the restriction proposed by @Christewart here along with P2PKH.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contract-negotiation question Further information is requested
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants