Scale legacy sigop count in CreateNewBlock #8362

1. 
   sdaftuar commented at 4:42 PM on July 18, 2016: member

   This bugfix should be cherry-picked to 0.13 as well.

2. Scale legacy sigop count in CreateNewBlock 682aa0f289
3. MarcoFalke added the label Mining on Jul 18, 2016
4. MarcoFalke added the label Needs backport on Jul 18, 2016
5. 
   luke-jr commented at 5:05 PM on July 18, 2016: member

   utACK, although I wonder if it would be better to do the scaling in GetLegacySigOpCount

6. MarcoFalke added this to the milestone 0.13.0 on Jul 19, 2016
7. 
   sipa commented at 10:23 AM on July 19, 2016: member

   utACK. I think this is the easiest fix.

8. 
   MarcoFalke commented at 10:25 AM on July 19, 2016: member

   utACK 682aa0f289c550c029733966a2ce3449e4a471df Agree with @sipa

9. 
   laanwj commented at 10:47 AM on July 19, 2016: member

   Sorry for the stupid question here: but what bug does this fix?

10. 
    sipa commented at 11:07 AM on July 19, 2016: member

    I think this is exactly a no-op. The internal result from CreateNewBlock is incorrectly computed (the sigop value for the coinbase transaction is off by a factor 4), however, this value is ignored by all call sites:

    • GBT lets the consumer construct their own coinbase, so does not report anything about the temporary coinbase transaction used inside CreateNewBlock
    • The generate RPC call uses the constructed block directly, ignoring all metadata about it.
11. 
    sdaftuar commented at 1:17 PM on July 19, 2016: member

    @laanwj @sipa You guys are right -- I stumbled across the units error when making the weight/cost change, and assumed it must propagate downstream somehow, but indeed it seems to be ignored.

    Still, unless someone is eager to rewrite the gbt code to be both better tested and have no no-op lines, I think we might as well merge this at some point so that we don't have "incorrect" code lying around (that could inadvertently be exposed or copied), but there's no urgency.

12. 
    laanwj commented at 3:57 PM on July 21, 2016: member

    I think we might as well merge this at some point so that we don't have "incorrect" code lying around (that could inadvertently be exposed or copied), but there's no urgency.

    Thanks for the explanation @sipa @sdaftuar .

    I tend to agree that this is better than leaving it as-is. Though on the other hand, if this is dead code, removing it may be the wisest thing to do. If it's unused even in tests, then for the next change, we'll likely forget to update it again.

13. laanwj merged this on Jul 25, 2016
14. laanwj closed this on Jul 25, 2016

    ---

15. laanwj referenced this in commit 517eee3e8f on Jul 25, 2016
16. laanwj referenced this in commit 86edc20a17 on Jul 25, 2016
17. MarcoFalke removed the label Needs backport on Jul 31, 2016
18. MarcoFalke locked this on Sep 8, 2021

Milestone
0.13.0