Make STRICTENC invalid pubkeys fail the script rather than the opcode.

sipa commented at 10:05 pm on November 8, 2014: member

As suggested on the mailinglist by @petertodd:

This turns STRICTENC turn into a softforking change, and as a result guarantee that using it for mempool validation only results in consensus-valid transactions in the mempool.

sipa force-pushed on Nov 8, 2014

petertodd commented at 6:30 am on November 9, 2014: contributor

While this is an improvement over the existing definition of STRICTENC, I think we need to decide what’s our actual goal here.

Are we trying to simplify the consensus critical codebase? If so we could achieve the same goal by causing all pubkeys that aren’t known to be accepted by OpenSSL to bypass the OpenSSL verify routine and be treated as though the signature failed. This would be a “almost certainly not a fork, unless there’s a weird SSL bug” change. Hybrid keys are the one exception, and my understanding is they’re already supported by your secp256k1 library.

Are we pissed off at embedded consensus systems? Well some of them already implement code to make their data appear as valid pubkeys, so this change does no harm to them, and prevents their users from being able to spend the outputs created and reduce the size of the UTXO set. Equally this change may render some users’ funds unspendable, potentially even funds locked in P2SH outputs.

sipa commented at 9:29 am on November 9, 2014: member

I’m simply trying to clean up the semantics of an existing standardness rule, as you yourself found out that they can lead to accepting invalid transactions in the mempool. A second validation with just consensus rules is definitely possible, but having every flag be a softforking change certainly simplifies reasoning a lot.

This is however not a consensus rule change, and AFAIK does not change anything for tx acceptance except in weird checksig+not cases. I don’t believe it should affect any system in production, as hybrid pubkeys have been non-standard since v0.8.

petertodd commented at 5:47 am on November 10, 2014: contributor

@sipa Well maybe we should just say something about how the STRICTENC rules may not be appropriate as an actual soft-fork and leave it at that? I agree that as an IsStandard() rule they’re fine.

I noticed that many of the hybrid-using addresses I generated haven’t been spent, with the one exception of 33xWKkEzGaABRopJBYtGWgSHGRpHYwJ2Sy, 1 2 CHECKMULTISIG, which I constructed such that existing broken STRICTENC implementation validates the signature against the normal version of the pubkey, and once it’s in a block the actual consensus rules validate against the hybrid version.

petertodd commented at 6:28 am on November 10, 2014: contributor

Whoops, actually I got that wrong: 3DDhLE6ggvuELYTaYsTVC9Ta3cgfbPocZn, 1 normal hybrid 2 CHECKMULTISIG, is the right way to trigger that STRICTENC weirdness as pubkeys are evaluated right to left.

in src/test/data/script_valid.json: in 4501f2df0c outdated

754-    "0x47 0x304402207592427de20e315d644839754f2a5cca5b978b983a15e6da82109ede01722baa022032ceaf78590faa3f7743821e1b47b897ed1a57f6ee1c8a7519d23774d8de3c4401",
755-    "0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 CHECKSIG NOT",
756+    "0 0x47 0x304402203b269b9fbc0936877bf855b5fb41757218d9548b246370d991442a5f5bd1c3440220235268a4eaa8c67e543c6e37da81dd36d3b1be2de6b4fef04113389ca6ddc04501",
757+    "1 0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 0x21 0x038282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f51508 2 CHECKMULTISIG",
758+    "",
759+    "1-of-2 with the second 1 hybrid pubkey and no STRICTENC"

petertodd commented at 6:32 am on November 10, 2014:

What are you trying to check here? The second hybrid pubkey isn’t evaluated, so even with STRICTENC this test will pass. Maybe you should reverse the order of the pubkeys, so the hybrid pubkey is evaluated?

sipa commented at 9:29 am on November 10, 2014:

The test is there to see that we (or others using these tests) don’t accidentally apply the test to non-evaluated public keys.

petertodd commented at 9:38 am on November 10, 2014:

Ah, makes sense.

in src/test/data/script_valid.json: in 4501f2df0c outdated

761+[
762+    "0 0x47 0x3044022044dc17b0887c161bb67ba9635bf758735bdde503e4b0a0987f587f14a4e1143d022009a215772d49a85dae40d8ca03955af26ad3978a0ff965faa12915e9586249a501",
763+    "1 0x41 0x0679be667ef9dcbbac55a06295ce870b07029bfcdb2dce28d959f2815b16f81798483ada7726a3c4655da4fbfc0e1108a8fd17b448a68554199c47d08ffb10d4b8 0x21 0x038282263212c609d9ea2a6e3e172de238d8c39cabd5ac1ca10646e23fd5f51508 2 CHECKMULTISIG",
764     "STRICTENC",
765-    "P2PK NOT with invalid hybrid pubkey"
766+    "1-of-2 with the second 1 hybrid pubkey"

petertodd commented at 6:33 am on November 10, 2014:

Whereas in this case the first compressed pubkey is valid, so the second hybrid isn’t evaluated and doesn’t fail anything.

petertodd commented at 6:35 am on November 10, 2014: contributor

Interestingly it looks like most ‘invalid pubkey’ users do things the wrong way around, with the invalid pubkeys second, so they would be affected by this implementation of STRICTENC when they don’t need to be: http://webbtc.com/scripts/multisig

edit: Of course, the best thing to do is encrypt your data and turn it into an apparently-valid pubkey by brute-forcing a nonce unless you for some reason really need to be able to prove the pubkey isn’t valid in your protocol.

petertodd commented at 2:00 pm on November 10, 2014: contributor

ACK

sipa force-pushed on Nov 10, 2014

sipa commented at 2:07 pm on November 10, 2014: member

Clarified that STRICTENC is not intended as a consensus rule.

btcdrak commented at 4:27 pm on November 13, 2014: contributor

ACK

TheBlueMatt commented at 2:33 am on November 18, 2014: member

Concept ACK, but needs rebased.

sipa force-pushed on Nov 18, 2014

sipa commented at 8:52 am on November 18, 2014: member

Rebased.

TheBlueMatt commented at 2:10 am on November 19, 2014: member

utACK commithash 266898f06b56ae0aeb2102141125a036b6808470: http://bitcoin.ninja/TheBlueMatt-5247.txt

theuni referenced this in commit 1c302e6c60 on Nov 19, 2014

theuni referenced this in commit 62abc8d611 on Nov 19, 2014

sipa commented at 2:21 pm on November 20, 2014: member

@laanwj I’d like to see this in 0.10, as this is not a change to standardness for any currently released version, but it will be if 0.10 is released without.

laanwj added this to the milestone 0.10.0 on Nov 20, 2014

Make STRICTENC invalid pubkeys fail the script rather than the opcode.

This turns STRICTENC turn into a softforking-safe change (even though it
is not intended as a consensus rule), and as a result guarantee that using
it for mempool validation only results in consensus-valid transactions in
the mempool.

98b135f97f

Test the exact order of CHECKMULTISIG sig/pubkey evaluation

Possible with STRICTENC

ca8158719b

sipa force-pushed on Nov 20, 2014

sipa commented at 2:37 pm on November 20, 2014: member

Rebased.

petertodd commented at 7:55 pm on November 20, 2014: contributor

ACK

laanwj merged this on Nov 21, 2014

laanwj closed this on Nov 21, 2014

laanwj referenced this in commit ca6fb4e885 on Nov 21, 2014

MarcoFalke locked this on Sep 8, 2021

Make STRICTENC invalid pubkeys fail the script rather than the opcode. #5247