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

Add SCRIPT_VERIFY_CLEANSTACK script verification flag #4311

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
7 participants
@petertodd
Copy link
Contributor

commented Jun 9, 2014

Causes VerifyScript() to fail if scriptPubKey/redeemScript execution leaves more than a single stack item. This is a generic malleability fix that can be used with all scriptPubKeys, as per @sipa's BIP62 thoughts. Of course, the current IsStandard() rules should only allow transactions satisfying this rule; I'm working on relaxing those rules.

Add SCRIPT_VERIFY_CLEANSTACK script verification flag
Causes VerifyScript() to fail if scriptPubKey/redeemScript execution
leaves more than a single stack item. This is a generic malleability fix
that can be used with all scriptPubKeys.
@sipa

This comment has been minimized.

Copy link
Member

commented Jun 10, 2014

Untested ACK.

@sipa

This comment has been minimized.

Copy link
Member

commented Jun 10, 2014

Hmm, it seems pulltester relies on sending non-standard transactions, and seeing them end up in the mempool.

@mikehearn

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2014

Code is:

    Transaction b78tx = new Transaction(params);
    {
        b78tx.addOutput(new TransactionOutput(params, b78tx, ZERO, new byte[]{OP_TRUE}));
        addOnlyInputToTransaction(b78tx, new TransactionOutPointWithValue(
                new TransactionOutPoint(params, 1, b77.getTransactions().get(1).getHash()),
                SATOSHI, b77.getTransactions().get(1).getOutputs().get(1).getScriptPubKey()));
        b78.addTransaction(b78tx);
    }
    b78.solve();
    blocks.add(new BlockAndValidity(blockToHeightMap, b78, true, false, b78.getHash(), chainHeadHeight + 26, "b78"));

    Block b79 = createNextBlock(b78, chainHeadHeight + 27, out26, null);
    Transaction b79tx = new Transaction(params);
    {
        b79tx.addOutput(new TransactionOutput(params, b79tx, ZERO, new byte[]{OP_TRUE}));
        b79tx.addInput(new TransactionInput(params, b79tx, new byte[]{OP_TRUE}, new TransactionOutPoint(params, 0, b78tx.getHash())));
        b79.addTransaction(b79tx);
    }
    b79.solve();
    blocks.add(new BlockAndValidity(blockToHeightMap, b79, true, false, b79.getHash(), chainHeadHeight + 27, "b79"));

It's pretty verbose, seems we should update it to the more modern and convenient tx building APIs. But indeed it's correct that they fail this new check: b79tx has an input script that connects to an output script, both of which are just OP_TRUE, so we end up with a combined script of OP_TRUE OP_TRUE which leaves >1 stack item.

As far as I can tell this isn't actually meant to test such a case, it's just that @TheBlueMatt might have not wanted to write out a full script. Matt, is that the case? If so we can probably make it pass by just changing OP_TRUE in the input script to OP_NOP or something.

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2014

Yes, @mikehearn, youre correct, I'm just lazy :)

@petertodd

This comment has been minimized.

Copy link
Contributor Author

commented Jun 11, 2014

@TheBlueMatt Be less lazy!

And by that I mean be more lazy with your school work... :) (or are you done for the summer?)

@mikehearn

This comment has been minimized.

Copy link
Contributor

commented Jun 18, 2014

I think I fixed the code in bitcoinj git master, but I don't know anything about the pull tester or how to rebuild/upgrade it.

@BitcoinPullTester

This comment has been minimized.

Copy link

commented Jun 23, 2014

Automatic sanity-testing: FAILED BUILD/TEST, see http://jenkins.bluematt.me/pull-tester/p4311_ce00fa1d609e6dcd233cc0859f078c469e40bf7b/ for binaries and test log.

This could happen for one of several reasons:

  1. It chanages paths in makefile.linux-mingw or otherwise changes build scripts in a way that made them incompatible with the automated testing scripts (please tweak those patches in qa/pull-tester)
  2. It adds/modifies tests which test network rules (thanks for doing that), which conflicts with a patch applied at test time
  3. It does not build on either Linux i386 or Win32 (via MinGW cross compile)
  4. The test suite fails on either Linux i386 or Win32
  5. The block test-cases failed (lookup the first bNN identifier which failed in https://github.com/TheBlueMatt/test-scripts/blob/master/FullBlockTestGenerator.java)

If you believe this to be in error, please ping BlueMatt on freenode or TheBlueMatt here.

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.

@jgarzik

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2014

LGTM

@sipa

This comment has been minimized.

Copy link
Member

commented Jul 29, 2014

@mikehearn I can deploy a new pulltester jar (though it'd be nice if I could build it myself...)

@petertodd

This comment has been minimized.

Copy link
Contributor Author

commented Nov 10, 2014

Closing as @sipa has a better version of this code in #5143

@petertodd petertodd closed this Nov 10, 2014

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.