Page MenuHomePhabricator
Differential D3473 Diff 10070 src/script/interpreter.cpp

Changeset View

Standalone View

View Options

src/script/interpreter.cpp

Show First 20 Lines • Show All 955 Lines • ▼ Show 20 Lines try {
return set_error(serror, return set_error(serror,
SCRIPT_ERR_CHECKDATASIGVERIFY); SCRIPT_ERR_CHECKDATASIGVERIFY);
} }
} }
} break; } break;
case OP_CHECKMULTISIG: case OP_CHECKMULTISIG:
case OP_CHECKMULTISIGVERIFY: { case OP_CHECKMULTISIGVERIFY: {
// ([sig ...] num_of_signatures [pubkey ...] // ([dummy] [sig ...] num_of_signatures [pubkey ...]
// num_of_pubkeys -- bool) // num_of_pubkeys -- bool)
if (stack.size() < 1) {
int i = 1;
if ((int)stack.size() < i) {
return set_error( return set_error(
serror, SCRIPT_ERR_INVALID_STACK_OPERATION); serror, SCRIPT_ERR_INVALID_STACK_OPERATION);
} }
int nKeysCount = int nKeysCount =
CScriptNum(stacktop(-i), fRequireMinimal).getint(); CScriptNum(stacktop(-1), fRequireMinimal).getint();
deadalnixUnsubmitted
Not Done
Inline Actions

No need for magic constants. Just make the initial value for i const and give it a meaningful name.

deadalnix: No need for magic constants. Just make the initial value for i const and give it a meaningful…
if (nKeysCount < 0 || if (nKeysCount < 0 ||
nKeysCount > MAX_PUBKEYS_PER_MULTISIG) { nKeysCount > MAX_PUBKEYS_PER_MULTISIG) {
return set_error(serror, SCRIPT_ERR_PUBKEY_COUNT); return set_error(serror, SCRIPT_ERR_PUBKEY_COUNT);
} }
nOpCount += nKeysCount; nOpCount += nKeysCount;
if (nOpCount > MAX_OPS_PER_SCRIPT) { if (nOpCount > MAX_OPS_PER_SCRIPT) {
return set_error(serror, SCRIPT_ERR_OP_COUNT); return set_error(serror, SCRIPT_ERR_OP_COUNT);
} }
int ikey = ++i; // ikey is the position of the last pubkey, if any.
MengerianUnsubmitted
Not Done
Inline Actions

The term "last pubkey" seems confusing to me.

I think of the topmost pubkey on the stack as the "first" one... And isn't the topmost item also first to be checked when looping through the signatures?

In any case, maybe changing the comment to something like "topmost pubkey" would be less ambiguous. Same for the other "last" items in comments below.

Mengerian: The term "last pubkey" seems confusing to me. I think of the topmost pubkey on the stack as…
markblundebergAuthorUnsubmitted
Done
Inline Actions

👍

markblundeberg: 👍
deadalnixUnsubmitted
Not Done
Inline Actions

Also, the fact you need a comment indicate the name is not good. Remove the comment and fix the name. Also make it const.

deadalnix: Also, the fact you need a comment indicate the name is not good. Remove the comment and fix the…
markblundebergAuthorUnsubmitted
Done
Inline Actions

This variable can't be const since it gets modified later on. Likewise for nPubKeys.

markblundeberg: This variable can't be const since it gets modified later on. Likewise for nPubKeys.
// ikey2 is the position of last non-signature item in int ikey = 2;
// the stack. Top stack item = 1. With
// SCRIPT_VERIFY_NULLFAIL, this is used for cleanup if // isigcount the position of nSigsCount, the stack
// operation fails. // element that divides the signatures and pubkeys list.
int ikey2 = nKeysCount + 2; // This is used for cleanup.
i += nKeysCount; const int isigcount = nKeysCount + 2;
if ((int)stack.size() < i) { if (stack.size() < size_t(isigcount)) {
deadalnixUnsubmitted
Not Done
Inline Actions

Stacktop is deifned as

#define stacktop(i) (stack.at(stack.size() + (i)))

So it also expect a size_t. No reasons not to use size_t for indices. Negating an unsigned integer is not a big deal, computer math are not real math :)

deadalnix: Stacktop is deifned as #define stacktop(i) (stack.at(stack.size() + (i))) So it also expect…
markblundebergAuthorUnsubmitted
Done
Inline Actions

This feels pretty weird to me. Is that a change you'd like to see across the entire interpreter.cpp?

markblundeberg: This feels pretty weird to me. Is that a change you'd like to see across the entire interpreter.
return set_error( return set_error(
serror, SCRIPT_ERR_INVALID_STACK_OPERATION); serror, SCRIPT_ERR_INVALID_STACK_OPERATION);
} }
int nSigsCount = int nSigsCount =
CScriptNum(stacktop(-i), fRequireMinimal).getint(); CScriptNum(stacktop(-isigcount), fRequireMinimal)
.getint();
if (nSigsCount < 0 || nSigsCount > nKeysCount) { if (nSigsCount < 0 || nSigsCount > nKeysCount) {
return set_error(serror, SCRIPT_ERR_SIG_COUNT); return set_error(serror, SCRIPT_ERR_SIG_COUNT);
} }
int isig = ++i; // isig is the position of the last signature, if any.
i += nSigsCount; int isig = nKeysCount + 3;
deadalnixUnsubmitted
Not Done
Inline Actions

No magic constants.

deadalnix: No magic constants.
if ((int)stack.size() < i) {
// idummy is the position of the dummy element.
const int idummy = nKeysCount + nSigsCount + 3;
deadalnixUnsubmitted
Not Done
Inline Actions

dito

deadalnix: dito
if (stack.size() < size_t(idummy)) {
return set_error( return set_error(
serror, SCRIPT_ERR_INVALID_STACK_OPERATION); serror, SCRIPT_ERR_INVALID_STACK_OPERATION);
} }
bool fSuccess = true;
deadalnixUnsubmitted
Not Done
Inline Actions

You don't need success to be moved here. What comes next isn't using it.

deadalnix: You don't need success to be moved here. What comes next isn't using it.
// Subset of script starting at the most recent // Subset of script starting at the most recent
// codeseparator // codeseparator
CScript scriptCode(pbegincodehash, pend); CScript scriptCode(pbegincodehash, pend);
// A bug causes CHECKMULTISIG to consume one extra
// argument whose contents were not checked in any way.
//
// Unfortunately this is a potential source of
// mutability, so optionally verify it is exactly equal
// to zero.
if ((flags & SCRIPT_VERIFY_NULLDUMMY) &&
stacktop(-idummy).size()) {
return set_error(serror, SCRIPT_ERR_SIG_NULLDUMMY);
}
// Remove signature for pre-fork scripts // Remove signature for pre-fork scripts
for (int k = 0; k < nSigsCount; k++) { for (int k = 0; k < nSigsCount; k++) {
valtype &vchSig = stacktop(-isig - k); valtype &vchSig = stacktop(-isig - k);
CleanupScriptCode(scriptCode, vchSig, flags); CleanupScriptCode(scriptCode, vchSig, flags);
} }
bool fSuccess = true;
while (fSuccess && nSigsCount > 0) { while (fSuccess && nSigsCount > 0) {
valtype &vchSig = stacktop(-isig); valtype &vchSig = stacktop(-isig);
valtype &vchPubKey = stacktop(-ikey); valtype &vchPubKey = stacktop(-ikey);
// Note how this makes the exact order of // Note how this makes the exact order of
// pubkey/signature evaluation distinguishable by // pubkey/signature evaluation distinguishable by
// CHECKMULTISIG NOT if the STRICTENC flag is set. // CHECKMULTISIG NOT if the STRICTENC flag is set.
// See the script_(in)valid tests for details. // See the script_(in)valid tests for details.
Show All 19 Lines try {
// If there are more signatures left than keys left, // If there are more signatures left than keys left,
// then too many signatures have failed. Exit early, // then too many signatures have failed. Exit early,
// without checking any further signatures. // without checking any further signatures.
if (nSigsCount > nKeysCount) { if (nSigsCount > nKeysCount) {
fSuccess = false; fSuccess = false;
} }
} }
// Clean up stack of actual arguments // Clean up stack:
while (i-- > 1) { // pop nKeysCount, pubkeys, and nSigsCount;
// If the operation failed, we require that all for (int i = 0; i < isigcount; i++) {
// signatures must be empty vector popstack(stack);
}
// pop signatures. If the operation failed and NULLFAIL
// flag is set, we require all signatures to be null
// (empty vectors);
for (int i = isigcount + 1; i < idummy; i++) {
if (!fSuccess && (flags & SCRIPT_VERIFY_NULLFAIL) && if (!fSuccess && (flags & SCRIPT_VERIFY_NULLFAIL) &&
!ikey2 && stacktop(-1).size()) { stacktop(-1).size()) {
return set_error(serror, return set_error(serror,
SCRIPT_ERR_SIG_NULLFAIL); SCRIPT_ERR_SIG_NULLFAIL);
} }
if (ikey2 > 0) {
ikey2--;
}
popstack(stack); popstack(stack);
} }
deadalnixUnsubmitted
Not Done
Inline Actions

You'd make this much clearer if you'd have something like

if (!fSuccess) {
  // Checks for NULLFAIL.
}

for (int i = 0; i < whatever; i++) {
  popstack(stack);
}

You could even consider moving the NULLFAIL check ahead of signature check in a subsequent diff to avoid checking a bunch of signature for nothing when you expect a failure anyways.

deadalnix: You'd make this much clearer if you'd have something like if (!fSuccess) { // Checks…
markblundebergAuthorUnsubmitted
Done
Inline Actions

OK 👍

As for moving the nullfail check to the front, that is rather tricky since even when you know ahead of time that fSuccess=false, the rest of the code must execute anyway, including at least one iteration of the main loop (due to the pubkey encoding rule).

Also, it's not clear what kind of actual optimization this offers given that TransactionSignatureChecker::CheckSig should return immediately when the signature is null. At best we would be optimizing for a very particular case of a transaction where the signature list contains both null and non-null signatures, which anyway causes the originating peer to be banned.

markblundeberg: OK 👍 As for moving the nullfail check to the front, that is rather tricky since even when you…
markblundebergAuthorUnsubmitted
Done
Inline Actions

(regarding executing anyway, looks like this wasn't covered, so I put up D3613)

markblundeberg: (regarding executing anyway, looks like this wasn't covered, so I put up D3613)
// pop dummy element.
// A bug causes CHECKMULTISIG to consume one extra
// argument whose contents were not checked in any way.
//
// Unfortunately this is a potential source of
// mutability, so optionally verify it is exactly equal
// to zero prior to removing it from the stack.
if (stack.size() < 1) {
return set_error(
serror, SCRIPT_ERR_INVALID_STACK_OPERATION);
}
if ((flags & SCRIPT_VERIFY_NULLDUMMY) &&
stacktop(-1).size()) {
return set_error(serror, SCRIPT_ERR_SIG_NULLDUMMY);
}
popstack(stack); popstack(stack);
stack.push_back(fSuccess ? vchTrue : vchFalse); stack.push_back(fSuccess ? vchTrue : vchFalse);
if (opcode == OP_CHECKMULTISIGVERIFY) { if (opcode == OP_CHECKMULTISIGVERIFY) {
if (fSuccess) { if (fSuccess) {
popstack(stack); popstack(stack);
} else { } else {
return set_error( return set_error(
serror, SCRIPT_ERR_CHECKMULTISIGVERIFY); serror, SCRIPT_ERR_CHECKMULTISIGVERIFY);
} }
} }
▲ Show 20 Lines • Show All 578 Lines • Show Last 20 Lines