Page MenuHomePhabricator

[mining] Ensure sigops and fees follow transactions during sorting
ClosedPublic

Authored by schancel on Nov 11 2018, 22:21.

Details

Summary

Ensure that sigOpCount and fees are attached to the proper transaction
when sorting for CTOR.

Test Plan
make check && ./test/functional/test_runner.py --extended

Diff Detail

Repository
rABC Bitcoin ABC
Branch
mining-metadata
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 3884
Build 5841: Bitcoin ABC Buildbot (legacy)
Build 5840: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
jasonbcox requested changes to this revision.Nov 11 2018, 23:11
jasonbcox added a subscriber: jasonbcox.

Needs test(s)

src/miner.cpp
141 ↗(On Diff #5752)

Please retain the comment: // Add dummy coinbase tx as first transaction.

This revision now requires changes to proceed.Nov 11 2018, 23:11
schancel added inline comments.
test/functional/abc-magnetic-anomaly-mining.py
118 ↗(On Diff #5763)

Asser transaciton

jasonbcox added inline comments.
test/functional/abc-magnetic-anomaly-mining.py
118 ↗(On Diff #5763)

This comment still needs fixing. Other than that, everything is good

This revision is now accepted and ready to land.Nov 12 2018, 05:55
deadalnix requested changes to this revision.Nov 12 2018, 12:12
deadalnix added a subscriber: deadalnix.

The timing file needs to be updated.

src/rpc/mining.cpp
636 ↗(On Diff #5764)

PushKV

test/functional/abc-magnetic-anomaly-mining.py
36 ↗(On Diff #5764)

Chose a meaningful variable name.

59 ↗(On Diff #5764)

Why random ? Wouldn't that be preferable if this is deterministic ?

71 ↗(On Diff #5764)

dito

This revision now requires changes to proceed.Nov 12 2018, 12:12
schancel added inline comments.
test/functional/abc-magnetic-anomaly-mining.py
59 ↗(On Diff #5764)

It doesn't matter. It could be index % 5 or something, but I think this is preferable as it guarantees something isn't just completely broken with the deterministic assumptions when checking things match.

Fix comment errors and renamed mnode

deadalnix requested changes to this revision.Nov 12 2018, 18:44
deadalnix added inline comments.
src/miner.cpp
224 ↗(On Diff #5770)

I was wondering how come the block can be filled. This going to be inefficient and at some point someone will forget to keep both in sync and disaster will follow.

Something like https://stackoverflow.com/questions/13840998/sorting-zipped-locked-containers-in-c-using-boost-or-the-stl seems to be a more appropriate way to do it.

This revision now requires changes to proceed.Nov 12 2018, 18:44
schancel added inline comments.
src/miner.cpp
224 ↗(On Diff #5770)

I agree. Except all the methods to zip 3 arrays and sort them depend on external dependencies. They're not part of STL yet, nor boost.

schancel marked an inline comment as done.

Update timing.json

Rebase and add comments to a ticket

schancel changed the visibility from "Restricted Project (Project)" to "Public (No Login Required)".Nov 13 2018, 05:10
schancel added inline comments.
src/miner.cpp
353

Should this be emplace_back?

350 ↗(On Diff #5773)

Should this be emplace?

src/miner.cpp
353

yes

Finish switch from push_back to pushKV in rpc functions

Remove incorrectly amended commit data

This revision is now accepted and ready to land.Nov 13 2018, 23:51
akb added a subscriber: akb.

Looks pretty straightforward to my newbie eyes.

doc/release-notes.md
6 ↗(On Diff #5811)

nit: and -> an

This revision was automatically updated to reflect the committed changes.