CTransactionSignatureSerializer() scans OP_CODESEPARATOR twice. First time to count the number of OP_CODESEPARATOR, second time to remove OP_CODESEPARATOR. If OP_CODESEPARATOR is not found in the first scan, the second scan is redundant.
Since the use of OP_CODESEPARATOR is extremely rare, the repeated scan is wasteful is most cases.
Concept ACK - any bench results to show the effect?
Style-nit: brace-less if should be inline: “If an if only has a single-statement then-clause, it can appear on the same line as the if, without braces.” But you may as well nest the while in the if as well, no?
https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c
Prefer not changing consensus critical code for performance reason without proper benchmark about it.
I have to agree here. Let’s be really careful around this code and only change it if necessary.
I was staring at this particular piece of code today. I found it pretty hard to read, so if we’re changing this, maybe add some more comments. For example it took me a while to realise CTransactionSignatureSerializer is only used for non-SegWit transactions, which finally helped me understand step 4 in BIP-0143.
This change looks harmless, but those are famous last words, so I would also encourage a benchmark.
Both loops start with it at scriptCode.begin() and do while (scriptCode.GetOp(it, opcode). The inside of the second loop does nothing if there’s no OP_CODESEPARATOR in the script.
