Differential D3474 Diff 10073 src/script/interpreter.cpp

Changeset View

Standalone View

src/script/interpreter.cpp

Show First 20 Lines • Show All 1,005 Lines • ▼ Show 20 Lines	try {
}		}

bool fSuccess = true;		bool fSuccess = true;

// 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);

		if ((flags & SCRIPT_ENABLE_SCHNORR_MULTISIG) &&
		(stacktop(-idummy).size() != 0)) {
		// NEW MULTISIG (SCHNORR / NULL)

		// "Dummy" element is now an integer whose bits
		// represent which pubkeys should be checked. Note
		// that the int type must support numbers as large
		// as 1 << MAX_PUBKEYS_PER_MULTISIG for the getint()
		// overflow clamping to work correctly here.
		int nCheckBits =
		deadalnixAuthorUnsubmitted Not Done Inline Actions You are much better off working with unsigned when you handle bitfields. deadalnix: You are much better off working with unsigned when you handle bitfields.
		CScriptNum(stacktop(-idummy), fRequireMinimal)
		.getint();
		if (nCheckBits < 0) {
		deadalnixAuthorUnsubmitted Not Done Inline Actions This is not the check you want to be doing. You want to check that: All the set bits are in range: `(nCheckBits & ((1 << keyCount) - 1)) == nCheckBits` The number of set bits correspond to the number of expected signatures: `popcount(nCheckBits) == sigCount` deadalnix: This is not the check you want to be doing. You want to check that: - All the set bits are in…
		return set_error(serror,
		SCRIPT_ERR_INVALID_CHECKBITS);
		}

		while (nSigsCount > 0 && nKeysCount > 0) {
		deadalnixAuthorUnsubmitted Not Done Inline Actions Use proper variable instead of mutating other variable that mean something other than being a loop counter. You also probably want to have two loops, one to validate pubkey encoding and one to validate sigs. There is no point starting to validate sigs. More generally, you shoudl try to simplify this loop by shaving stuff out of it. deadalnix: Use proper variable instead of mutating other variable that mean something other than being a…
		markblundebergUnsubmitted Done Inline Actions A separate loop to validate pubkey encoding would likely mean a change in behaviour -- all pubkeys must be correctly encoded. Is that intended? markblundeberg: A separate loop to validate pubkey encoding would likely mean a change in behaviour -- all…
		MengerianUnsubmitted Not Done Inline Actions Having a separate loop here to check pubkey encoding results is strange behavior: It would check all pubkeys for successful Schnorr multisigs, but not for the "0-of-N" or "false" cases. (Or for the legacy case). Two potential alternatives are: Check pubkey encoding only when it has a corresponding signature (ie, when the checkbit is "1"), and don't check the others. I can't think of any reason not to do this. It is similar to the current multisig operation, and seems to have minimal side effects. It would render some previously unspendable coins become spendable (when unused pubkeys encoding was sometimes checked due to how legacy multisig works). Move the loop that checks pubkey encoding to prior to the dummy element "mode selection" check. This would make it apply to both the legacy case (including 0-of-N and "false") as well as the new Schnorr mode. This disadvantage of this is that it would affect already existing coins, rendering them unspendable if they have invalidly encoded pubkeys. I'm not sure if this is common or not, so it's possible it's not a major problem. Mengerian: Having a separate loop here to check pubkey encoding results is strange behavior: It would…
		deadalnixAuthorUnsubmitted Not Done Inline Actions It doesn't check for 0 of n anyways and what is the false case ? If there is a discrepancy in behavior, this can make a good test case. I can't derive one from that comment. deadalnix: It doesn't check for 0 of n anyways and what is the false case ? If there is a discrepancy in…
		markblundebergUnsubmitted Done Inline Actions The appropriate test cases are in place. If you want to `arc patch` this Diff and parents, and try the modifications you suggest, you can see how it affects the test cases. markblundeberg: The appropriate test cases are in place. If you want to `arc patch` this Diff and parents, and…
		if (nCheckBits & 1) {
		deadalnixAuthorUnsubmitted Not Done Inline Actions When you nest most of the logic in an if, you indicate that you probably should bail the opposite branch early. deadalnix: When you nest most of the logic in an if, you indicate that you probably should bail the…
		markblundebergUnsubmitted Done Inline Actions When the predicate here is false, we aren't bailing. There are still statements that need to be executed. markblundeberg: When the predicate here is false, we aren't bailing. There are still statements that need to be…
		deadalnixAuthorUnsubmitted Not Done Inline Actions Execute them before and them continue. deadalnix: Execute them before and them continue.
		markblundebergUnsubmitted Done Inline Actions They can't be executed before since they modify values used in the body of this if-statement. It is possible to rewrite this to use `if(!x) continue;` but it just gets more verbose. markblundeberg: They can't be executed before since they modify values used in the body of this if-statement.
		// Check signature as requested
		valtype &vchPubKey = stacktop(-ikey);
		valtype &vchSig = stacktop(-isig);
		if (!CheckTransactionSchnorrSignatureEncoding(
		vchSig, flags, serror) \|\|
		!CheckPubKeyEncoding(vchPubKey, flags,
		serror)) {
		// serror is set
		return false;
		}
		if (!checker.CheckSig(vchSig, vchPubKey,
		scriptCode, flags)) {
		// It is forbidden to request invalid
		// signature. Make the error message a
		// bit informative though.
		return set_error(
		serror,
		SCRIPT_ERR_INVALID_CHECKBITS_SIGNATURE);
		deadalnixAuthorUnsubmitted Not Done Inline Actions It's not clear what this error stands for. It needs a better name. deadalnix: It's not clear what this error stands for. It needs a better name.
		markblundebergUnsubmitted Done Inline Actions I agree, any suggestions? markblundeberg: I agree, any suggestions?
		deadalnixAuthorUnsubmitted Not Done Inline Actions Thinking about it, it's a nullfail or a nulldummy error, not really a new kind of error. deadalnix: Thinking about it, it's a nullfail or a nulldummy error, not really a new kind of error.
		markblundebergUnsubmitted Done Inline Actions Indeed, that's exactly what I had in a prior version of this Diff. :-D I'm happy to revert if that's desirable. markblundeberg: Indeed, that's exactly what I had in a prior version of this Diff. :-D I'm happy to revert if…
		}
		// A successful checksig is the only way to
		// decrement nSigsCount.
		isig++;
		nSigsCount--;
		}
		nCheckBits >>= 1;
		ikey++;
		nKeysCount--;
		}
		if (nCheckBits != 0) {
		// Ended before consuming all bits, because too
		// many bits were set, or too-high bits were
		// set.
		return set_error(serror,
		SCRIPT_ERR_INVALID_CHECKBITS);
		}
		if (nSigsCount > 0) {
		// Ended before checking all signatures, because
		// too few bits were set. This can return false
		// (without error) if all signatures were null
		// and nCheckBits was 0 to start.
		fSuccess = false;
		}
		} else {
		// LEGACY MULTISIG (ECDSA / NULL)
// A bug causes CHECKMULTISIG to consume one extra		// A bug causes CHECKMULTISIG to consume one extra
// argument whose contents were not checked in any way.		// argument whose contents were not checked in any
		// way.
//		//
// Unfortunately this is a potential source of		// Unfortunately this is a potential source of
// mutability, so optionally verify it is exactly equal		// mutability, so optionally verify it is exactly
// to zero.		// equal to zero.
if ((flags & SCRIPT_VERIFY_NULLDUMMY) &&		if ((flags & SCRIPT_VERIFY_NULLDUMMY) &&
stacktop(-idummy).size()) {		stacktop(-idummy).size()) {
return set_error(serror, SCRIPT_ERR_SIG_NULLDUMMY);		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);
}		}

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
// CHECKMULTISIG NOT if the STRICTENC flag is set.		// by CHECKMULTISIG NOT if the STRICTENC flag is
// See the script_(in)valid tests for details.		// set. See the script_(in)valid tests for
		// details.
if (!CheckTransactionECDSASignatureEncoding(		if (!CheckTransactionECDSASignatureEncoding(
vchSig, flags, serror) \|\|		vchSig, flags, serror) \|\|
!CheckPubKeyEncoding(vchPubKey, flags,		!CheckPubKeyEncoding(vchPubKey, flags,
serror)) {		serror)) {
// serror is set		// serror is set
return false;		return false;
}		}

// Check signature		// Check signature
bool fOk = checker.CheckSig(vchSig, vchPubKey,		bool fOk = checker.CheckSig(vchSig, vchPubKey,
scriptCode, flags);		scriptCode, flags);

if (fOk) {		if (fOk) {
isig++;		isig++;
nSigsCount--;		nSigsCount--;
}		}
ikey++;		ikey++;
nKeysCount--;		nKeysCount--;

// If there are more signatures left than keys left,		// If there are more signatures left than keys
// then too many signatures have failed. Exit early,		// left, then too many signatures have failed.
// without checking any further signatures.		// Exit early, without checking any further
		// signatures.
if (nSigsCount > nKeysCount) {		if (nSigsCount > nKeysCount) {
fSuccess = false;		fSuccess = false;
}		}
}		}
		}

// Clean up stack:		// Clean up stack:
// pop nKeysCount, pubkeys, and nSigsCount;		// pop nKeysCount, pubkeys, and nSigsCount;
for (int i = 0; i < isigcount; i++) {		for (int i = 0; i < isigcount; i++) {
popstack(stack);		popstack(stack);
}		}
// pop signatures. If the operation failed and NULLFAIL		// pop signatures. If the operation failed and NULLFAIL
// flag is set, we require all signatures to be null		// flag is set, we require all signatures to be null
▲ Show 20 Lines • Show All 599 Lines • Show Last 20 Lines