Skip to content

Conversation

@sipa
Copy link
Member

@sipa sipa commented Nov 8, 2014

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.

@petertodd
Copy link
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
Copy link
Member Author

sipa commented Nov 9, 2014

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
Copy link
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
Copy link
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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, makes sense.

@petertodd
Copy link
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
Copy link
Contributor

ACK

@sipa
Copy link
Member Author

sipa commented Nov 10, 2014

Clarified that STRICTENC is not intended as a consensus rule.

@btcdrak
Copy link
Contributor

btcdrak commented Nov 13, 2014

ACK

@TheBlueMatt
Copy link
Contributor

Concept ACK, but needs rebased.

@sipa
Copy link
Member Author

sipa commented Nov 18, 2014

Rebased.

@TheBlueMatt
Copy link
Contributor

@sipa
Copy link
Member Author

sipa commented Nov 20, 2014

@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 laanwj added this to the 0.10.0 milestone Nov 20, 2014
sipa and others added 2 commits November 20, 2014 15:29
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.
@sipa
Copy link
Member Author

sipa commented Nov 20, 2014

Rebased.

@petertodd
Copy link
Contributor

ACK

@laanwj laanwj merged commit ca81587 into bitcoin:master Nov 21, 2014
laanwj added a commit that referenced this pull request Nov 21, 2014
ca81587 Test the exact order of CHECKMULTISIG sig/pubkey evaluation (Peter Todd)
98b135f Make STRICTENC invalid pubkeys fail the script rather than the opcode. (Pieter Wuille)
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants