BIP 143: Transaction signature verification for version 0 and version 1 witness program #270

pull jl2012 wants to merge 1 commits into bitcoin:master from jl2012:segwit-checksig changing 0 files +0 −0

jl2012 commented at 5:26 AM on January 5, 2016: contributor

This proposal defines a new transaction digest algorithm for signature verification in version 0 and version 1 witness program, in order to minimize redundant data hashing in verification, and to cover the input value by the signature.
jl2012 force-pushed on Jan 5, 2016
in bip-segwit-checksig.mediawiki:None in fd55dd92e6 outdated
```
  97 | +  ss << hashSequence;
  98 | +  // The input being signed (replacing the scriptSig with scriptCode + amount)
  99 | +  // The prevout may already be contained in hashPrevout, and the nSequence
 100 | +  // may already be contain in hashSequence.
 101 | +  ss << txTo.vin[nIn].prevout;
 102 | +  ss << static_cast<const CScriptBase&>(scriptCode);
```
NicolasDorier commented at 9:21 AM on January 6, 2016:

render as

ss << static_cast<const></const>(scriptCode);
jl2012 commented at 6:10 AM on January 7, 2016:

It seems a bug of github. Any idea?

btcdrak commented at 6:11 AM on January 7, 2016:

yeah looks like output escaping.

MarcoFalke commented at 12:43 PM on January 7, 2016:

Not a bug. There is a syntax error in your mediawiki code: https://github.com/jl2012/bips/pull/5

Also you can activate code highlighting on the cpp code.
NicolasDorier commented at 9:37 AM on January 6, 2016: contributor

So, this basically redefines the 4 ECDSA OPs in the case of segwit scripts.

The only downside I see is outweighted by the upside.

On the good side, signature checkers already know the transaction being spent. From that they can parse the input being signed, and see if it is segwit. If it is, then change the hash rule, this is easy change.

However, I am a bit concerned about needing the previous input values. Actually transaction checkers don't have this information. Requiring it will make lots more code to change to support segwit, because the signature checker need now more data than before. It might not really be a valid complain though considering the upside.

(btw, people using NBitcoin with TransactionBuilder won't have any breaking code because of that, the TransactionBuilder already requires the full spent txout)
sipa commented at 2:58 PM on January 6, 2016: member

On Jan 6, 2016 01:37, "Nicolas Dorier" notifications@github.com wrote:

However, I am a bit concerned about needing the previous input values. Actually transaction checkers don't have this information. Requiring it will make lots more code to change to support segwit, because the signature checker need now more data than before. It might not really be a valid complain though considering the upside.

I am a bit confused what signature checker you are talking about. Either you are a full node that maintains a UTXO set, and you have the amounts, or you can't usefully verify anything at all. If you're talking about the signing code: that should have access to amounts or it couldn't construct a correct transaction.

However, I don't consider signing amounts or not a deal breaker. We had to propose changing the sighashing algorithm to fix the O(n^2) behaviour, and if we're doing that, it would seem stupid to not also make the probably most-often requested related feature (amount signing). If there is substantial controversy about it, I think we can remove that, and postpone it to be an optional feature of a future softfork.
NicolasDorier commented at 6:43 PM on January 6, 2016: contributor
I don't think this will be controversial. My point is that the method (in all frameworks and language)
```
EvalScript(Script scriptPubKey, Script scriptSig, Transaction, Input)
```
Will become
```
EvalScript(TxOut previous, Script scriptSig, Transaction, Input)
```
And same thing for the "Sign" method. In practice, this will be easy to fix, people who can fetch the scriptPubKey before will most likely be able to fetch the TxOut easily as you said. But this will not be automatic.

Frankly, I don't think that matter, wallet providers, hardware wallet provider, and service providers like myself want so much the benefit of having the input value in the hash that nobody will likely complain.
luke-jr added the label New BIP on Jan 6, 2016
luke-jr added the label Needs number assignment on Jan 6, 2016
rubensayshi cross-referenced this on Jan 7, 2016 from issue BIP 141: Segregated Witness (Consensus layer) by CodeShark
Merge pull request #5 from MarcoFalke/patch-1
Fix syntax error </ref>
7cecfacdf5
FelixWeis commented at 2:31 PM on January 7, 2016: none

This update seems like the perfect opportunity to get rid of that CHECKMULTISIG bug where one item too many is popped off the stack.
jl2012 commented at 5:25 AM on January 8, 2016: contributor

@FelixWeis I'm not sure if it's worth extra testing and reviewing. The improvement is just 1 byte per CHECKMULTISIG. And CHECKMULTISIG will become obsolete when we introduce new signature scheme like Schnorr after the segwit softfork.
FelixWeis commented at 5:51 AM on January 8, 2016: none

If schnorr is replacing these completly not fixing makes sense, thanks!

On Fri, 8 Jan 2016 14:25 Johnson Lau notifications@github.com wrote:

@FelixWeis https://github.com/FelixWeis I'm not sure if it's worth extra testing and reviewing. The improvement is just 1 byte per CHECKMULTISIG. And CHECKMULTISIG will become obsolete when we introduce new signature scheme like Schnorr after the segwit softfork.

— Reply to this email directly or view it on GitHub #270 (comment).
luke-jr assigned luke-jr on Jan 8, 2016
luke-jr renamed this:
~~New BIP: Transaction signature verification for version 0 and version 1 witness program~~
BIP 143: Transaction signature verification for version 0 and version 1 witness program
on Jan 8, 2016
luke-jr removed the label Needs number assignment on Jan 8, 2016
luke-jr merged this on Jan 8, 2016
luke-jr closed this on Jan 8, 2016

Contributors

Labels

Linked (view graph)

#265 BIP 141: Segregated Witness (Consensus layer)