-
Notifications
You must be signed in to change notification settings - Fork 38.7k
Make STRICTENC invalid pubkeys fail the script rather than the opcode. #5247
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
Conversation
|
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. |
|
I'm simply trying to clean up the semantics of an existing standardness This is however not a consensus rule change, and AFAIK does not change |
|
@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. |
|
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, makes sense.
|
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. |
|
ACK |
|
Clarified that STRICTENC is not intended as a consensus rule. |
|
ACK |
|
Concept ACK, but needs rebased. |
|
Rebased. |
|
utACK commithash 266898f06b56ae0aeb2102141125a036b6808470: http://bitcoin.ninja/TheBlueMatt-5247.txt |
|
@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. |
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.
Possible with STRICTENC
|
Rebased. |
|
ACK |
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.