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

Relax IsStandard rules for pay-to-script-hash transactions #4365

Merged
merged 1 commit into from Jun 27, 2014

Conversation

@gavinandresen
Copy link
Contributor

@gavinandresen gavinandresen commented Jun 18, 2014

Relax the AreInputsStandard() tests for P2SH transactions --
allow any Script in a P2SH transaction to be relayed/mined,
as long as it has 15 or fewer signature operations.

Rationale: https://gist.github.com/gavinandresen/88be40c141bc67acb247

I don't have an easy way to test this, but the code changes are
straightforward and I've updated the AreInputsStandard unit tests.

@ghost
Copy link

@ghost ghost commented Jun 18, 2014

ACK

{
int tmpExpected;
if (whichType2 == TX_SCRIPTHASH) // HASH160 <hash> OP_EQUAL
tmpExpected = 1;

This comment has been minimized.

@laanwj

laanwj Jun 19, 2014
Member

Is this special-case just to make things explicit? I checked and ScriptSigArgsExpected returns 1 for TX_SCRIPTHASH.

@mikehearn
Copy link
Contributor

@mikehearn mikehearn commented Jun 19, 2014

I worry that this will have the same effect as the fee drop - users and merchants will upgrade, miners won't, and the resulting lack of mempool consensus makes double spending easier. When we did the fee drop we didn't worry about this much because we'd done it before and it worked out OK, but this time seems to be different and miners aren't bothering to upgrade, perhaps because 0.9 had nothing for them.

I don't want to make things unnecessarily complicated, but I wonder if the change should be marked in the coinbase. Then IsStandard changes once a sufficient percentage of hashing power has upgraded. Alternatively maybe if @dgenr8 gets his double spend relaying code in, that might also help.

@petertodd
Copy link
Contributor

@petertodd petertodd commented Jun 19, 2014

@mikehearn Are we going to hold up every feature just because it might impact zeroconf transactions? Eligius already has implemented this patch with ~10% of hashing power; if you're worried about double-spends that opportunity already exists.

oneAndTwo << OP_2 << key[3].GetPubKey() << key[4].GetPubKey() << key[5].GetPubKey();
oneAndTwo << OP_3 << OP_CHECKMULTISIG;
oneAndTwo << OP_AND;
keystore.AddCScript(oneAndTwo);

This comment has been minimized.

@petertodd

petertodd Jun 20, 2014
Contributor

Pedantic: This example isn't actually spendable. Right now the first CHECKMULTISIG will leave a '1' on the stack making the second CHECKMULTISIG always fail. Change it to a CHECKMULTISIGVERIFY instead and remove the AND.

@petertodd
Copy link
Contributor

@petertodd petertodd commented Jun 20, 2014

ACK modulo CHECKMULTISIG nit.

I had been working on my own version of this patch and came up with a pretty similar design. Tested this with real-world non-std P2SH transactions on mainnet, no issues found. I also can't see any way that this could be a DoS issue - the 'limit sigops' approach taken is exactly how my version worked as well.

Relax the AreInputsStandard() tests for P2SH transactions --
allow any Script in a P2SH transaction to be relayed/mined,
as long as it has 15 or fewer signature operations.

Rationale: https://gist.github.com/gavinandresen/88be40c141bc67acb247

I don't have an easy way to test this, but the code changes are
straightforward and I've updated the AreInputsStandard unit tests.
@gavinandresen
Copy link
Contributor Author

@gavinandresen gavinandresen commented Jun 23, 2014

Nits picked (removed unneeded test @laanwj found, fixed the sigop-counting unit test so it uses a 'realistic' Script) and rebased for the CMutableTransaction change.

@BitcoinPullTester
Copy link

@BitcoinPullTester BitcoinPullTester commented Jun 23, 2014

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4365_7f3b4e95695d50a4970e6eb91faa956ab276f161/ for binaries and test log.
This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/
Contact BlueMatt on freenode if something looks broken.

@petertodd
Copy link
Contributor

@petertodd petertodd commented Jun 23, 2014

ACK

@mhanne
Copy link
Contributor

@mhanne mhanne commented Jun 24, 2014

FYI, as @petertodd suggested, I added an index of the "inner" P2SH script types to bitcoin-ruby/webbtc, so you can see unusual p2sh scripts that are already spent:
http://webbtc.com/p2sh_scripts/unknown

gavinandresen added a commit that referenced this pull request Jun 27, 2014
Relax IsStandard rules for pay-to-script-hash transactions
@gavinandresen gavinandresen merged commit 6259937 into bitcoin:master Jun 27, 2014
zauguin added a commit to zauguin/bitcoin that referenced this pull request Dec 30, 2015
Since bitcoin#4365 (6259937) P2SH scripts do not have to be IsStandard scripts.
luke-jr added a commit to luke-jr/bitcoin that referenced this pull request Feb 12, 2016
Since bitcoin#4365 (6259937) P2SH scripts do not have to be IsStandard scripts.
Github-Pull: bitcoin#7266
Rebased-From: 6cd198f
lateminer added a commit to lateminer/bitcoin that referenced this pull request Oct 25, 2018
Since bitcoin#4365 (6259937) P2SH scripts do not have to be IsStandard scripts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.