[mining] Rename several CBlockTemplateEntry members for clarity
ClosedPublic
Actions

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

rSTAGINGcebcb34c93c0: [mining] Rename several CBlockTemplateEntry members for clarity
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

Branch

chains-for-real1 (branched from master)

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 5773
Build 9608: Bitcoin ABC Buildbot (legacy)
Build 9607: arc lint + arc unit

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

Herald added a reviewer: deadalnix. · View Herald Transcript

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing started.

schancel added a child revision: D2863: [mining] Add fields for tracking package data..Apr 23 2019, 01:06

Build Bitcoin-ABC / Diffs / Bitcoin-ABC Diff Testing passed.

Harbormaster completed remote builds in B5567: Diff 8224.Apr 23 2019, 01:20

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

schancel added inline comments.Apr 24 2019, 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.Apr 24 2019, 23:42

schancel added inline comments.Apr 24 2019, 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.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

schancel added inline comments.May 5 2019, 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.