Add policy: null signature for failed CHECK(MULTI)SIG#8634
Add policy: null signature for failed CHECK(MULTI)SIG#8634laanwj merged 1 commit intobitcoin:masterfrom
Conversation
|
Please tag 0.13.1 |
|
Concept ACK |
|
This is an implementation of NULLFAIL of BIP146, except the activation logic: https://github.com/bitcoin/bips/blob/master/bip-0146.mediawiki#NULLFAIL |
src/test/data/script_tests.json
Outdated
There was a problem hiding this comment.
Comments for the following few lines should be read as "BIP66-compliant but not NULLFAIL-compliant". Will fix
|
CodeReview ACK bb50e7683d97ea6cc69fa5837a51fba54643f2cc |
|
concept ACK |
|
ACK d071bfd |
|
utACK |
|
concept ACK, utACK, will review more. |
afk11
left a comment
afk11
left a comment
There was a problem hiding this comment.
tACK & code review bb50e76
|
utACK bb50e7683d97ea6cc69fa5837a51fba54643f2cc |
src/script/interpreter.cpp
Outdated
There was a problem hiding this comment.
Small nit: moving this line below the next will avoid performing the addition if the range check on nKeysCount fails. Also, since this is only used for cleanup, I would suggest giving the variable a more descriptive name like nNonSigCleanupItems or adding a comment describing what it's for.
There was a problem hiding this comment.
Addressed with 5d23cfe
jl2012
force-pushed
the
nullfail
branch
2 times, most recently
from
be29b35 to
1912bf1
Compare
|
ut ACK 1912bf1 |
instagibbs
left a comment
instagibbs
left a comment
There was a problem hiding this comment.
utACK 1912bf12d06011f989c0db4ae7cbf433cfff18da
I'd like to see the commits squashed though.
src/script/interpreter.cpp
Outdated
There was a problem hiding this comment.
nit of nits: just check vchSig.size() instead, move before popstack
There was a problem hiding this comment.
Addressed with cd102b5
src/script/interpreter.cpp
Outdated
There was a problem hiding this comment.
This code section is a bit confusing already with all these different counters, why not set
int ikey2 = i;
right after the following line:
i += nKeysCount;
below? From there the two values diverge.
There was a problem hiding this comment.
I prefer the current way as it's clearly correct. The way that i is calculated looks a bit obscure to me
There was a problem hiding this comment.
Oh, I misread. Yes, the i is calculated quite oddly, but it's the value we're calculating yet again here.
|
utACK cd102b57a4c148fb93428e656e7282732c65033c |
|
needs rebase |
|
rebased |
e41bd44 Add policy: null signature for failed CHECK(MULTI)SIG (Johnson Lau)
Github-Pull: bitcoin#8634 Rebased-From: e41bd44
…)SIG e41bd44 Add policy: null signature for failed CHECK(MULTI)SIG (Johnson Lau)
…)SIG e41bd44 Add policy: null signature for failed CHECK(MULTI)SIG (Johnson Lau)
14 participants
Intended for 0.13.1
This PR adds a new policy to require that signature(s) must be empty vector for failed CHECK(MULTI)SIG operations.
This fixes malleability for some forms of scripts, for example:
"key 1" CHECKSIG IF "key 2" CHECKSIG ELSE "key 3" CHECKSIGVERIFY "locktime" CLTV ENDIF
which is spendable with both sig 1 and sig 2 anytime, or sig 3 only after locktime
Without this new policy, a relay node may inject arbitrary BIP66-compliant data to the scriptSig/witness when the ELSE branch is executed.
This may become a softfork in the future. I will prepare for a BIP later