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

[Policy] Several transaction standardness rules #11423

Merged
merged 4 commits into from May 12, 2018

Conversation

@jl2012
Copy link
Contributor

commented Sep 29, 2017

This disables OP_CODESEPARATOR in non-segwit scripts (even in an unexecuted branch), and makes a positive FindAndDelete result invalid. This ensures that the scriptCode serialized in SignatureHash is always the same as the script passing to the EvalScript.

@jl2012

This comment has been minimized.

Copy link
Contributor Author

commented Sep 29, 2017

This is part of #8755
@TheBlueMatt

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2017

Strong Concept ACK. We do ML posts for new standardness rules, right? Probably doesn't matter given their lack of use.

@jl2012 jl2012 force-pushed the jl2012:const_scriptcode branch Sep 29, 2017

@sipa

This comment has been minimized.

Copy link
Member

commented Sep 29, 2017

Any reason to support OP_CODESEPARATOR inside P2WSH?

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Sep 29, 2017

@fanquake fanquake added the Validation label Sep 30, 2017

@jl2012

This comment has been minimized.

Copy link
Contributor Author

commented Sep 30, 2017

@sipa I think @NicolasDorier has something that use CODESEPARATOR. I think it could make the size of some contracts smaller.

This is actually part of the attempt to fix the pre-segwit quadratic sighash problem. A variable scriptCode makes sighash caching more difficult. That's why I proposed to do it for pre-segwit only

@bitcoin bitcoin deleted a comment Sep 30, 2017

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Oct 2, 2017

Any reason to support OP_CODESEPARATOR inside P2WSH?

YES, I am using it for tumblebit, it saves 33 bytes per outputs. It allows you to sign a specific branch with one key instead of having separate public key per branch. Please do not do that until we have at least MAST. (only if signing in MAST requires the scriptCode to depends on the executed branch)

OP_CODESEPARATOR is very useful, I am coupling it with SIGHASH_NONE. (The only case where SIGHASH_NONE is not similar to giving your private key)

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Oct 2, 2017

Just to explain my case here:
Imagine the following: Alice, expect Bob to publish a transaction satisfying the PAYMENT condition to get the preimage of <BOB_HASH>.

OP_DEPTH 3 EQUAL
OP_IF
    2 <ALICE_KEY_PAYMENT> <BOB_KEY_PAYMENT> MULTICHECKSIGVERIFY HASH160 <BOB_HASH> EQUAL
OP_ELSE
    <ALICE_KEY_REDEEM> CHECKSIG CLTV DROP
OP_END

If ALICE_KEY_PAYMENT == ALICE_KEY_REDEEM, this would be no good, as BOB could use Alice signature to satisfy the REDEEM condition, and get the money without revealing the pre image.

Now you can save 33 bytes this way. (untested, but it get the idea of what I do for TB)

<ALICE_KEY>
OP_DEPTH 3 EQUAL
OP_IF
    OP_SWAP <BOB_KEY_PAYMENT> CHECKSIGVERIFY HASH160 <BOB_HASH> EQUAL OP_CODESEP
OP_ELSE
     CLTV DROP
OP_END
CHECKSIG

Notice that Alice can just pass a SIGHASH_NONE signature for the first branch to Bob, so she does not have to know how bob will spend the txout, while still being sure she can get the preimage as soon as Bob try to cashout.

Concept ACK for removing on non-segwit scripts, but it should be kept for segwit.
Using this trick, you can save quite a lot of data if there is lots of branches. (and also have more branches, since the scripts are limited to 512 bytes)

src/script/interpreter.h Outdated
@@ -106,6 +106,10 @@ enum
// Public keys in segregated witness scripts must be compressed
//
SCRIPT_VERIFY_WITNESS_PUBKEYTYPE = (1U << 15),

// Making OP_CODESEPARATOR and FindAndDelete non-standard in non-segwit scripts

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 3, 2017

Contributor

For correctness, may wish to note that if FindAndDelete matches in either SegWit or non-SegWit, the script fails, OP_CODESEPARATOR limitation only applies to non-SegWit.

This comment has been minimized.

Copy link
@jl2012

jl2012 Oct 3, 2017

Author Contributor

FindAndDelete is not used anywhere in segwit

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Oct 3, 2017

Contributor

Oops, indeed, missed the sigversion check.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Nov 7, 2017

Contributor

nit: comments on script flags shouldnt reference standardness - this flag "Makes OP_CODESEPARATOR and FindAndDelete fail any non-segwit scripts", not just make them nonstandard.

src/script/interpreter.cpp Outdated
@@ -301,6 +301,10 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
opcode == OP_RSHIFT)
return set_error(serror, SCRIPT_ERR_DISABLED_OPCODE); // Disabled opcodes.

// OP_CODESEPARATOR in non-segwit transaction is invalid even in an unexecuted branch

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Oct 4, 2017

Member

nit: would have pushed that in the switch case for OP_CODESEPARATOR.

This comment has been minimized.

Copy link
@luke-jr

luke-jr Nov 10, 2017

Member

The switch case wouldn't get reached if it's in an unexecuted branch...?

This comment has been minimized.

Copy link
@NicolasDorier

NicolasDorier Nov 11, 2017

Member

Yep, disregard what I said, it would not have worked.

@jgarzik

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2017

concept ACK

@TheBlueMatt
Copy link
Contributor

left a comment

utACK 98cea79fd2161ce0b42bdef27befc6a23047975f

src/script/interpreter.h Outdated
@@ -106,6 +106,10 @@ enum
// Public keys in segregated witness scripts must be compressed
//
SCRIPT_VERIFY_WITNESS_PUBKEYTYPE = (1U << 15),

// Making OP_CODESEPARATOR and FindAndDelete non-standard in non-segwit scripts

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Nov 7, 2017

Contributor

nit: comments on script flags shouldnt reference standardness - this flag "Makes OP_CODESEPARATOR and FindAndDelete fail any non-segwit scripts", not just make them nonstandard.

@luke-jr
Copy link
Member

left a comment

Concept ACK

src/script/interpreter.cpp Outdated
@@ -301,6 +301,10 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
opcode == OP_RSHIFT)
return set_error(serror, SCRIPT_ERR_DISABLED_OPCODE); // Disabled opcodes.

// OP_CODESEPARATOR in non-segwit transaction is invalid even in an unexecuted branch

This comment has been minimized.

Copy link
@luke-jr

luke-jr Nov 10, 2017

Member

The switch case wouldn't get reached if it's in an unexecuted branch...?

src/script/interpreter.cpp Outdated
@@ -301,6 +301,10 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
opcode == OP_RSHIFT)
return set_error(serror, SCRIPT_ERR_DISABLED_OPCODE); // Disabled opcodes.

// OP_CODESEPARATOR in non-segwit transaction is invalid even in an unexecuted branch

This comment has been minimized.

Copy link
@luke-jr

luke-jr Nov 10, 2017

Member

"Invalid" is the wrong term here, since it's just a policy rule (for now).

@jl2012 jl2012 force-pushed the jl2012:const_scriptcode branch 2 times, most recently Nov 14, 2017

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2017

utACK effb6f9568a92ad6fe0ebf9da308cb0237df327b. One interesting test-case you may consider adding is checking that a FindAndDelete does not match (and thus the script is valid, even with CONST_SCRIPTCODE) a push with a non-minimal push encoding.

@jonasschnelli

This comment has been minimized.

Copy link
Member

commented Nov 15, 2017

utACK 0575b1831cd52987c76320d304674a27a140fe1f

  • IMO requires bitcoin-dev ML post (if merged)
  • requires release notes part (can be done after merging)
src/script/interpreter.cpp Outdated
@@ -301,6 +301,10 @@ bool EvalScript(std::vector<std::vector<unsigned char> >& stack, const CScript&
opcode == OP_RSHIFT)
return set_error(serror, SCRIPT_ERR_DISABLED_OPCODE); // Disabled opcodes.

// With SCRIPT_VERIFY_CONST_SCRIPTCODE, OP_CODESEPARATOR in non-segwit script is invalid even in an unexecuted branch

This comment has been minimized.

Copy link
@Sjors

Sjors Nov 16, 2017

Member

Perhaps out of scope, but the word "invalid" is confusing. It's non-standard because this check is only part of STANDARD_SCRIPT_VERIFY_FLAGS. A future soft fork could make it actually invalid depending on e.g. block height. Perhaps we should use a different word like "reject"?

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Nov 16, 2017

Contributor

I mean it clearly indicates that "With SCRIPT_VERIFY_CONST_SCRIPTCODE", and in that case it is "invalid", same as any other invalid action.c

This comment has been minimized.

Copy link
@esotericnonsense

esotericnonsense Feb 20, 2018

Contributor

Not particularly concerned but I agree that 'rejected' would be slightly clearer here.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Nov 27, 2017

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Nov 28, 2017

Just to clarify, OP_CODESEP could be removed completely from segwit if there is another way to explicitely sign a script path. (MAST could allow that)

@jl2012

This comment has been minimized.

Copy link
Contributor Author

commented Feb 6, 2018

rebased

@esotericnonsense

This comment has been minimized.

Copy link
Contributor

commented Feb 20, 2018

utACK 850c41c2a33efd73eff0bbdefc6ba2762901b60e
checked the diff of tx_valid and tx_invalid.json

@jl2012 jl2012 force-pushed the jl2012:const_scriptcode branch Feb 20, 2018

@jl2012

This comment has been minimized.

Copy link
Contributor Author

commented Feb 20, 2018

Rebase with comments fixed (s/invalid/rejected/)

@fanquake fanquake added this to For backport in High-priority for review May 24, 2018

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request May 24, 2018

Add constant scriptCode policy in non-segwit scripts
This disables OP_CODESEPARATOR in non-segwit scripts (even in an unexecuted branch), and makes a positive FindAndDelete result invalid. This ensures that the scriptCode serialized in SignatureHash() is always the same as the script passing to the EvalScript.

Github-Pull: bitcoin#11423
Rebased-From: 9dabfe4

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request May 24, 2018

Add transaction tests for constant scriptCode
Tests showing that CONST_SCRIPTCODE is applied only to non-segwit transactions

Github-Pull: bitcoin#11423
Rebased-From: 0f8719b

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request May 24, 2018

Policy to reject extremely small transactions
A transaction with 1 segwit input and 1 P2WPHK output has non-witness size of 82 bytes. Anything smaller than this have unnecessary malloc overhead and are not relayed/mined.

Github-Pull: bitcoin#11423
Rebased-From: 7485488

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request May 24, 2018

@fanquake

This comment has been minimized.

Copy link
Member

commented May 28, 2018

Backported in #13317.

@fanquake fanquake removed this from For backport in High-priority for review May 28, 2018

azuchi added a commit to chaintope/bitcoinrb that referenced this pull request May 31, 2018

HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Jun 29, 2018

Add constant scriptCode policy in non-segwit scripts
This disables OP_CODESEPARATOR in non-segwit scripts (even in an unexecuted branch), and makes a positive FindAndDelete result invalid. This ensures that the scriptCode serialized in SignatureHash() is always the same as the script passing to the EvalScript.

Github-Pull: bitcoin#11423
Rebased-From: 9dabfe4

HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Jun 29, 2018

Add transaction tests for constant scriptCode
Tests showing that CONST_SCRIPTCODE is applied only to non-segwit transactions

Github-Pull: bitcoin#11423
Rebased-From: 0f8719b

HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Jun 29, 2018

Policy to reject extremely small transactions
A transaction with 1 segwit input and 1 P2WPHK output has non-witness size of 82 bytes. Anything smaller than this have unnecessary malloc overhead and are not relayed/mined.

Github-Pull: bitcoin#11423
Rebased-From: 7485488

HashUnlimited pushed a commit to chaincoin/chaincoin that referenced this pull request Jun 29, 2018

braydonf added a commit to braydonf/bcoin that referenced this pull request Feb 2, 2019

braydonf added a commit to braydonf/bcoin that referenced this pull request Feb 2, 2019

tuxcanfly added a commit to tuxcanfly/bcoin that referenced this pull request Apr 19, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.