Page MenuHomePhabricator

[mining] Rename several CBlockTemplateEntry members for clarity
ClosedPublic

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

Details

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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Owners added a reviewer: Restricted Owners Package.Apr 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 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.Apr 24 2019, 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.Apr 24 2019, 22:57
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.

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.May 3 2019, 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.May 3 2019, 17:09
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.

This revision is now accepted and ready to land.May 7 2019, 23:29

Update with comment per @jasonbcox

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

@jasonbcox how does this sound?

Update for clarity that the comment pertains only to the coinbase

This revision was automatically updated to reflect the committed changes.