CBlock.GetSigOpCount counts vtx[0].vin[0] #596

CBlock.GetSigOpCount counts vtx[0].vin[0]scriptSig's OpCount. It shouldn't, because the value in vtx[0].vin[0].scriptSig is a random string between 2 and 100 bytes.

It isn't a random string, the scriptSig is a CScript. Standard txIns have only data-pushing opcodes, which count as zero SigOps, but theoretically you could have a scriptSig that included an OP_CHECKSIG.

AFAIK it is a random string... see IncrementExtraNonce in main.cpp. We are talking about the Coin Base transaction, and the Coin Base's CTxIn.

Got it, sorry for being dense. You're right, the coinbase scriptSig shouldn't be checked for SigOps.

Cheers. :)

Won't changing this make it easy to fork the block chain?

luke-jr: Doing an OpCount on random bytes doesn't make any sense.

dmgores: vtx[0].vin[0].scriptSig is not random bytes-- it can be random bytes encoded as a CScript, but it must deserialize as a CScript for the transaction/block to be valid: pblock->vtx[0].vin[0].scriptSig = CScript() << pblock->nTime << CBigNum(nExtraNonce);

... is the normal way to create it, and CScript::operator<<() Does The Right Thing (create a script that, if it was ever executed, would do two PUSHDATA operations to put the nTime and nExtraNonce on the stack).

luke-jr is correct, if we stop counting OP_CHECKSIG/OP_CHECKMULTISIG operations in vtx[0].vin[0] then somebody could try to split the block chain by sending a transaction that is valid under the new code but invalid under the old.

I don't think the coinbase is ever deserialized.

EDIT: nevermind - the issue is obvious that it is, when checking the opcodes.

I'd argue that forever counting operations on data that is only meant to be random bytes is a waste of CPU time... but if you do not want to at some time break old clients from the chain due to this somewhat minor optimization, I guess I'm ok with that.

At some time. When that happens, I agree this should be fixed. Along with a million other minor bugs.

How do we deal with this, in light of the proposed adding of nHeight to the coinbase?

I don't see how that's relevant to this...

Since sigops in the coinbase are counted, adding non-script-serialized data in front of coinbase may cause an invalid deserialization of what follows, causing sigops to be counted where there aren't any. (yes, I know there aren't ever any real sigops in a coinbase, but as most coinbases are currently constructed through script serialization, the invalid interpretation won't occur).

The proposed height addition is script-serialized.

My mistake, i believed it was different.

This is a three-year old issue now. Does this still need to be solved?

It requires a hard fork to solve it. And there's no reason why it is a problem (rather than inconvenient), as it's easy enough as a miner to never create a sigop in a coinbase.

Closing as it is not actually a problem.