Change sigops cost to sigops weight #8844

pull jnewbery wants to merge 1 commits into bitcoin:master from jnewbery:sigops_weight changing 15 files +94 −95

jnewbery commented at 9:46 PM on September 29, 2016: member

BIP 141 (https://github.com/bitcoin/bitcoin/pull/8149) introduced a new metric for counting block size. This was initially called 'block cost', but was renamed to 'block weight' in https://github.com/bitcoin/bitcoin/commit/2c06bae39edfaa9c0855d83377ad8fda09e4fa08 (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.
MarcoFalke added the label Docs and Output on Sep 29, 2016
jnewbery commented at 7:11 PM on October 3, 2016: member

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 renamed this:
~~change sigops cost to sigops weight~~
[WIP] change sigops cost to sigops weight
on Oct 3, 2016
jnewbery force-pushed on Oct 3, 2016
jnewbery commented at 7:48 PM on October 3, 2016: member

Ready for review. I've tested with make check
jnewbery renamed this:
~~[WIP] change sigops cost to sigops weight~~
Change sigops cost to sigops weight
on Oct 3, 2016
jnewbery force-pushed on Nov 4, 2016
jnewbery commented at 9:56 AM on November 4, 2016: member

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 commented at 10:16 AM on November 4, 2016: member

Should just go back to simply "sigops". Old sigop counting is dead once segwit activates.
jnewbery commented at 6:08 PM on November 4, 2016: member

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)
change sigops cost to sigops weight f119ec2700
jnewbery force-pushed on Nov 4, 2016
jnewbery commented at 6:21 PM on November 4, 2016: member

bad rebase. Should be fixed now.
laanwj commented at 9:14 AM on November 7, 2016: member

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 commented at 8:36 AM on November 25, 2016: member

This needs a rebase.
jnewbery commented at 9:08 AM on November 26, 2016: member

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 commented at 7:24 AM on November 28, 2016: member

I'm not convinced that we need to do this inside the codebase, but no objection.
jnewbery commented at 9:46 PM on January 9, 2017: member

Closing. There doesn't seem to be much appetite to make this change.
jnewbery closed this on Jan 9, 2017
jnewbery deleted the branch on Jan 9, 2017
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#8149 Segregated witness rebased #30476 scriptpubkeyman fuzz target TopUp is slow