Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip redundant OP_CODESEPARATOR scan #14786

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@jl2012
Copy link
Contributor

commented Nov 22, 2018

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.

@IlyasRidhuan

This comment has been minimized.

Copy link

commented Nov 23, 2018

utACK a5660e2

@Empact

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

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

@NicolasDorier

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

Prefer not changing consensus critical code for performance reason without proper benchmark about it.

@laanwj

This comment has been minimized.

Copy link
Member

commented Nov 23, 2018

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.

@Sjors

This comment has been minimized.

Copy link
Member

commented Mar 5, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.