Page MenuHomePhabricator

[mining] Rename several CBlockTemplateEntry members for clarity
ClosedPublic

Authored by schancel on Apr 23 2019, 01:05.

Details

Reviewers
deadalnix
jasonbcox
Group Reviewers
Restricted Owners Package(Owns No Changed Paths)
Restricted Project
Commits
rABCcebcb34c93c0: [mining] Rename several CBlockTemplateEntry members for clarity
Summary

Rename variables in order to clarify what these items are associated with for a future change, which will be adding package data. Additionally, add the txSize which will be used in further diffs.

Test Plan
make check

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

schancel created this revision.Apr 23 2019, 01:05
Owners added a reviewer: Restricted Owners Package.Apr 23 2019, 01:05
Herald added a reviewer: Restricted Project. · View Herald TranscriptApr 23 2019, 01:05
jasonbcox requested changes to this revision.Apr 23 2019, 15:56
jasonbcox added a subscriber: jasonbcox.
jasonbcox added inline comments.
src/miner.cpp
206 ↗(On Diff #8224)

Why is the coinbase tx size not set?

src/miner.h
30 ↗(On Diff #8224)

The code seems to indicate that this actually negative total fees. The comment should indicate this.

30 ↗(On Diff #8224)

Why rename these? it will only make backports more difficult with no benefit.

This revision now requires changes to proceed.Apr 23 2019, 15:56
schancel requested review of this revision.Apr 24 2019, 04:40
schancel added inline comments.
src/miner.cpp
206 ↗(On Diff #8224)

I don't know. Apparently it's not important. This change is trying to rename a variable.

src/miner.h
30 ↗(On Diff #8224)

This code is unique to ABC. It was written to deal with CTOR sorts.

30 ↗(On Diff #8224)

They're not negative.

jasonbcox requested changes to this revision.Wed, Apr 24, 22:57
jasonbcox added inline comments.
src/miner.h
30 ↗(On Diff #8224)

The code below has multiple places where txFees is negative and I can't find anywhere they are actually positive values.

30 ↗(On Diff #8224)

ok

This revision now requires changes to proceed.Wed, Apr 24, 22:57
schancel added inline comments.Wed, Apr 24, 23:42
src/miner.h
30 ↗(On Diff #8224)

Which code below? There is no code below that assumes these are negative. If you're talking about:

pblocktemplate->entries.emplace_back(CTransactionRef(), -SATOSHI, 0, -1);

That's because they're supposed to be out of bounds.

schancel requested review of this revision.Wed, Apr 24, 23:42
schancel added inline comments.Wed, Apr 24, 23:47
src/miner.cpp
193 ↗(On Diff #8224)

@jasonbcox You can see right here that these fees are not negative. They are added to the subsidy to generate the coinbase vout.

361 ↗(On Diff #8224)

@jasonbcox And they are added together here. So there's no funny business going on with the sign.

jasonbcox requested changes to this revision.Fri, May 3, 17:09
jasonbcox added inline comments.
src/miner.cpp
206 ↗(On Diff #8224)

Clearly txSize is being added in this diff, so it's absolutely applicable to review here.

This revision now requires changes to proceed.Fri, May 3, 17:09
schancel added inline comments.Sun, May 5, 07:08
src/miner.cpp
206 ↗(On Diff #8224)

These original entries predate anything I've done, and the coinbase transaction is discarded in getblocktemplate with only fees being returned. The mining pool software is expected to fill in the details within the 1000 bytes of reserved space.

When the generate rpc is called on regtest the IncramentExtraNonce function *does* use the coinbase, but these fields are also not used.

jasonbcox accepted this revision.Tue, May 7, 23:29
This revision is now accepted and ready to land.Tue, May 7, 23:29
schancel updated this revision to Diff 8540.Thu, May 9, 01:13

Update with comment per @jasonbcox

src/miner.cpp
207 ↗(On Diff #8540)

@jasonbcox how does this sound?

schancel edited the summary of this revision. (Show Details)Thu, May 9, 01:14
schancel updated this revision to Diff 8541.Thu, May 9, 01:30

Update for clarity that the comment pertains only to the coinbase

jasonbcox accepted this revision.Thu, May 9, 01:31
This revision was automatically updated to reflect the committed changes.