Details

Reviewers

jasonbcox
akb
deadalnix

Group Reviewers

Restricted Project

Commits

rSTAGINGb3abb4892641: [mining] Ensure sigops and fees follow transactions during sorting
rABCb3abb4892641: [mining] Ensure sigops and fees follow transactions during sorting

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 3883
Build 5839: Bitcoin ABC Buildbot (legacy)
Build 5838: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

Add integration test

Fix a comment

Remove junk

Fix the test description

schancel marked an inline comment as done.Nov 12 2018, 05:23

schancel added inline comments.

test/functional/abc-magnetic-anomaly-mining.py
118 ↗	(On Diff #5763)	Asser transaciton

jasonbcox accepted this revision.Nov 12 2018, 05:55

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

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 marked an inline comment as done.Nov 12 2018, 18:29

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

Rebase on master

schancel marked an inline comment as done.Nov 12 2018, 20:13

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.