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

Scale legacy sigop count in CreateNewBlock #8362

Merged
merged 1 commit into from Jul 25, 2016

Conversation

Projects
None yet
5 participants
@sdaftuar
Member

sdaftuar commented Jul 18, 2016

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

@luke-jr

This comment has been minimized.

Show comment
Hide comment
@luke-jr

luke-jr Jul 18, 2016

Member

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

Member

luke-jr commented Jul 18, 2016

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

@MarcoFalke MarcoFalke added this to the 0.13.0 milestone Jul 19, 2016

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 19, 2016

Member

utACK. I think this is the easiest fix.

Member

sipa commented Jul 19, 2016

utACK. I think this is the easiest fix.

@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Jul 19, 2016

Member

utACK 682aa0f Agree with @sipa

Member

MarcoFalke commented Jul 19, 2016

utACK 682aa0f Agree with @sipa

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 19, 2016

Member

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

Member

laanwj commented Jul 19, 2016

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 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.
Member

sipa commented Jul 19, 2016

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.
@sdaftuar

This comment has been minimized.

Show comment
Hide comment
@sdaftuar

sdaftuar Jul 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.

Member

sdaftuar commented Jul 19, 2016

@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.

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Jul 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.

Member

laanwj commented Jul 21, 2016

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.

@laanwj laanwj merged commit 682aa0f into bitcoin:master Jul 25, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 25, 2016

Merge #8362: Scale legacy sigop count in CreateNewBlock
682aa0f Scale legacy sigop count in CreateNewBlock (Suhas Daftuar)

laanwj added a commit that referenced this pull request Jul 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment