Skip to content

Conversation

jnewbery
Copy link
Contributor

BIP 141 (#8149) introduced a new metric for counting block size. This was initially called 'block cost', but was renamed to 'block weight' in 2c06bae (and is referred to as 'block weight' in https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki)

BIP 141 also changed the way that sigops are counted in transactions and blocks in exactly the same way - the limit was increased 4x, sigops in the main tx structure are counted 4x and sigops in the witness are counted 1x (essentially giving sigops in the witness a discount). This new metric was initially called SIGOPS_COST in the code. This PR changes the name to SIGOPS_WEIGHT to bring it in line with transaction/block weight.

@maflcko maflcko added the Docs label Sep 29, 2016
@jnewbery
Copy link
Contributor Author

jnewbery commented Oct 3, 2016

I've noticed there are more sigopsCost variables that I haven't renamed. Marking this PR as WIP until I've also fixed those.

@jnewbery jnewbery changed the title change sigops cost to sigops weight [WIP] change sigops cost to sigops weight Oct 3, 2016
@jnewbery
Copy link
Contributor Author

jnewbery commented Oct 3, 2016

Ready for review. I've tested with make check

@jnewbery jnewbery changed the title [WIP] change sigops cost to sigops weight Change sigops cost to sigops weight Oct 3, 2016
@jnewbery
Copy link
Contributor Author

jnewbery commented Nov 4, 2016

rebased.

@laanwj - can I get a concept ACK/NACK for this? Do you think it's worth considering?

I personally think it's a good idea to have consistent terminology between block/sigops weight, but other people may not think it's worth the code churn.

@luke-jr
Copy link
Member

luke-jr commented Nov 4, 2016

Should just go back to simply "sigops". Old sigop counting is dead once segwit activates.

@jnewbery
Copy link
Contributor Author

jnewbery commented Nov 4, 2016

Thanks for the input @luke-jr . I'd be worried that calling SigOpsWeight just "sigops" would be confusing, as people reading the code might think that it refers to the number of signature operations (it doesn't since the the sigops in the scriptSig and scriptPubKey are multiplied by the witness scaling factor, and the sigops in the witness aren't)

@jnewbery
Copy link
Contributor Author

jnewbery commented Nov 4, 2016

bad rebase. Should be fixed now.

@laanwj
Copy link
Member

laanwj commented Nov 7, 2016

I think it is worth having consistent terminology.
But as for code churn, I could see this making it harder to backport things, although this could be backported as it doesn't change any outside interfaces.
Let's ask who is working on segwit whether this is a concern @sipa @jl2012

@fanquake
Copy link
Member

This needs a rebase.

@jnewbery
Copy link
Contributor Author

Thanks @fanquake . @sipa / @jl2012 - can I have a concept ACK/NACK before I rebase this again? If you don't think it's worth changing the sigops terminology to be in line with 'block weight', then I'll just close this PR.

@sipa
Copy link
Member

sipa commented Nov 28, 2016

I'm not convinced that we need to do this inside the codebase, but no objection.

@jnewbery
Copy link
Contributor Author

jnewbery commented Jan 9, 2017

Closing. There doesn't seem to be much appetite to make this change.

@jnewbery jnewbery closed this Jan 9, 2017
@jnewbery jnewbery deleted the sigops_weight branch January 9, 2017 21:46
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants