[Policy] Several transaction standardness rules #11423

jl2012 · 2017-09-29T21:44:38Z

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 · 2017-09-29T21:45:32Z

This is part of #8755
@TheBlueMatt

TheBlueMatt · 2017-09-29T21:48:01Z

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

sipa · 2017-09-29T22:18:29Z

Any reason to support OP_CODESEPARATOR inside P2WSH?

TheBlueMatt · 2017-09-29T22:27:57Z

I'd prefer we be consistent. Especially if OP_CODESEPARATOR has no meaning in SegWit transactions we should just make it nonstandard everywhere.

…

On September 29, 2017 6:18:36 PM EDT, Pieter Wuille ***@***.***> wrote: Any reason to support OP_CODESEPARATOR inside P2WSH? -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #11423 (comment)

jl2012 · 2017-09-30T05:33:34Z

@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

NicolasDorier · 2017-10-02T04:40:44Z

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 · 2017-10-02T05:01:00Z

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)

TheBlueMatt · 2017-10-03T17:44:11Z

src/script/interpreter.h

@@ -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


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.

FindAndDelete is not used anywhere in segwit

Oops, indeed, missed the sigversion check.

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.

NicolasDorier · 2017-10-04T03:17:52Z

src/script/interpreter.cpp

@@ -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


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

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

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

jgarzik · 2017-10-04T03:32:35Z

concept ACK

TheBlueMatt

utACK 98cea79fd2161ce0b42bdef27befc6a23047975f

TheBlueMatt · 2017-11-07T21:51:58Z

src/script/interpreter.h

@@ -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


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

Concept ACK

luke-jr · 2017-11-10T14:13:25Z

src/script/interpreter.cpp

@@ -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


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

luke-jr · 2017-11-10T14:13:49Z

src/script/interpreter.cpp

@@ -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


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

TheBlueMatt · 2017-11-14T18:57:26Z

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 · 2017-11-15T07:12:20Z

utACK 0575b1831cd52987c76320d304674a27a140fe1f

IMO requires bitcoin-dev ML post (if merged)
requires release notes part (can be done after merging)

Sjors · 2017-11-16T10:04:15Z

src/script/interpreter.cpp

@@ -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


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"?

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

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

TheBlueMatt · 2017-11-27T16:35:45Z

Relevant ML thread is at https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2017-November/015292.html

NicolasDorier · 2017-11-28T07:02:25Z

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 · 2018-02-06T19:35:45Z

rebased

esotericnonsense · 2018-02-20T14:48:41Z

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

jl2012 · 2018-02-20T18:11:35Z

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

References: - bitcoin/bitcoin#11423 - bitcoin/bitcoin#12600 - bitcoin/bitcoin#12082 Trivial References: - bitcoin/bitcoin#12393 - bitcoin/bitcoin#6539 - bitcoin/bitcoin#10742 - bitcoin/bitcoin@ecb11f5

…closure c4b0c08 Update tx-size-small comment with relevant CVE disclosure (Gregory Sanders) Pull request description: Code first introduced under #11423 with essentially no description and no discussion. ACKs for top commit: MarcoFalke: ACK c4b0c08 fanquake: ACK c4b0c08 Tree-SHA512: 95d5c92998b8b1e944c477dbaee265b62612b6e815099ab31d9ff580b4dff777abaf7f326a284644709f918aa1510412d62310689b1250ef6e64de7b19ca9f71

Summary: Use a variable with a meaningful name to improve readability. See https://reviews.bitcoinabc.org/D8929#204886 This is a partial backport of Core [[bitcoin/bitcoin#11423 | PR11423]] 364bae5 Test Plan: test/functional/test_runner.py p2p_invalid_tx Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D8934

364bae5 qa: Pad scriptPubKeys to get minimum sized txs (MarcoFalke) 7485488 Policy to reject extremely small transactions (Johnson Lau) 0f8719b Add transaction tests for constant scriptCode (Johnson Lau) 9dabfe4 Add constant scriptCode policy in non-segwit scripts (Johnson Lau) Pull request description: 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`. Tree-SHA512: a0552cb920294d130251c48053fa2ff1fbdd26332e62b52147d918837852750f0ce35ce2cd1cbdb86588943312f8154ccb4925e850dbb7c2254bc353070cd5f8

…CVE disclosure c4b0c08 Update tx-size-small comment with relevant CVE disclosure (Gregory Sanders) Pull request description: Code first introduced under bitcoin#11423 with essentially no description and no discussion. ACKs for top commit: MarcoFalke: ACK c4b0c08 fanquake: ACK c4b0c08 Tree-SHA512: 95d5c92998b8b1e944c477dbaee265b62612b6e815099ab31d9ff580b4dff777abaf7f326a284644709f918aa1510412d62310689b1250ef6e64de7b19ca9f71

jl2012 force-pushed the const_scriptcode branch from b8b8d48 to 98cea79 Compare September 29, 2017 21:48

fanquake added the Validation label Sep 30, 2017

bitcoin deleted a comment Sep 30, 2017

TheBlueMatt reviewed Oct 3, 2017

View reviewed changes

NicolasDorier reviewed Oct 4, 2017

View reviewed changes

TheBlueMatt reviewed Nov 7, 2017

View reviewed changes

luke-jr reviewed Nov 10, 2017

View reviewed changes

jl2012 force-pushed the const_scriptcode branch 2 times, most recently from 0eab289 to effb6f9 Compare November 14, 2017 18:35

Sjors reviewed Nov 16, 2017

View reviewed changes

NicolasDorier mentioned this pull request Jan 5, 2018

[feature] Allow signing raw transaction in trezorctl, eg BIP-174 PSBT trezor/python-trezor#183

Closed

jl2012 force-pushed the const_scriptcode branch from effb6f9 to 6337539 Compare February 6, 2018 17:39

jl2012 force-pushed the const_scriptcode branch from 6337539 to 441f75c Compare February 20, 2018 18:10

jl2012 mentioned this pull request Feb 26, 2018

[WIP] Reuse sighash computations across evaluation #8654

Closed

jl2012 mentioned this pull request Mar 6, 2018

Implement excessive sighashing protection policy with tight sighash estimation #8755

Closed

braydonf mentioned this pull request Feb 2, 2019

Update transaction and script tests bcoin-org/bcoin#684

Merged

Sjors mentioned this pull request Mar 8, 2019

Implement BIPXXX's new softfork rules (The Great Consensus Cleanup) #15482

Closed

instagibbs mentioned this pull request Sep 16, 2019

doc: Update tx-size-small comment with relevant CVE disclosure #16885

Merged

jonatack mentioned this pull request Jan 16, 2020

Topics: updates for December and four new topics bitcoinops/bitcoinops.github.io#318

Merged

adamjonas mentioned this pull request Feb 12, 2021

Update "82 bytes" Q and A chaincodelabs/seminars#53

Merged

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

[Policy] Several transaction standardness rules #11423

[Policy] Several transaction standardness rules #11423

Uh oh!

Conversation

jl2012 commented Sep 29, 2017

Uh oh!

jl2012 commented Sep 29, 2017

Uh oh!

TheBlueMatt commented Sep 29, 2017

Uh oh!

sipa commented Sep 29, 2017

Uh oh!

TheBlueMatt commented Sep 29, 2017 via email

Uh oh!

jl2012 commented Sep 30, 2017

Uh oh!

NicolasDorier commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasDorier commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NicolasDorier Oct 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgarzik commented Oct 4, 2017

Uh oh!

TheBlueMatt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Nov 14, 2017

Uh oh!

jonasschnelli commented Nov 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Nov 27, 2017

Uh oh!

NicolasDorier commented Nov 28, 2017

Uh oh!

jl2012 commented Feb 6, 2018

Uh oh!

esotericnonsense commented Feb 20, 2018

Uh oh!

jl2012 commented Feb 20, 2018

Uh oh!

Uh oh!

NicolasDorier commented Oct 2, 2017 •

edited

Loading

NicolasDorier commented Oct 2, 2017 •

edited

Loading

NicolasDorier Oct 4, 2017 •

edited

Loading