Page MenuHomePhabricator
Differential D2047

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

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

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 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 updated this revision to Diff 5761.Nov 12 2018, 05:16

Add integration test

schancel updated this revision to Diff 5762.Nov 12 2018, 05:18

Fix a comment

schancel updated this revision to Diff 5763.Nov 12 2018, 05:20

Remove junk

schancel updated this revision to Diff 5764.Nov 12 2018, 05:22

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

schancel updated this revision to Diff 5770.Nov 12 2018, 18:30

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 updated this revision to Diff 5773.Nov 12 2018, 20:07

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.

schancel updated this revision to Diff 5777.Nov 12 2018, 21:52
schancel marked an inline comment as done.

Update timing.json

jasonbcox accepted this revision.Nov 12 2018, 22:18
schancel mentioned this in T479: [mining] Fix sorting of transaction metadata.Nov 13 2018, 04:58
schancel updated this revision to Diff 5794.Nov 13 2018, 05:10

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
teamcity added a subscriber: teamcity.Nov 13 2018, 05:10

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

schancel updated this revision to Diff 5795.Nov 13 2018, 05:13

Add release note line

teamcity added a comment.Nov 13 2018, 05:15

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

teamcity added a comment.Nov 13 2018, 05:18

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

Harbormaster completed remote builds in B3883: Diff 5794.Nov 13 2018, 05:19
teamcity added a comment.Nov 13 2018, 05:24

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

Harbormaster completed remote builds in B3884: Diff 5795.Nov 13 2018, 05:24
schancel marked an inline comment as done.Nov 13 2018, 19:03
schancel added inline comments.
src/miner.cpp
353

Should this be emplace_back?

350 ↗(On Diff #5773)

Should this be emplace?

deadalnix added inline comments.Nov 13 2018, 19:04
src/miner.cpp
353

yes

schancel updated this revision to Diff 5809.Nov 13 2018, 19:05

Use emplace_back

teamcity added a comment.Nov 13 2018, 19:06

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

teamcity added a comment.Nov 13 2018, 19:14

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

Harbormaster completed remote builds in B3893: Diff 5809.Nov 13 2018, 19:14
schancel updated this revision to Diff 5810.Nov 13 2018, 19:15

Finish switch from push_back to pushKV in rpc functions

teamcity added a comment.Nov 13 2018, 19:15

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

schancel updated this revision to Diff 5811.Nov 13 2018, 19:18

Remove incorrectly amended commit data

teamcity added a comment.Nov 13 2018, 19:20

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

teamcity added a comment.Nov 13 2018, 19:23

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

Harbormaster completed remote builds in B3894: Diff 5810.Nov 13 2018, 19:23
teamcity added a comment.Nov 13 2018, 19:29

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

Harbormaster completed remote builds in B3895: Diff 5811.Nov 13 2018, 19:29
schancel removed a reviewer: deadalnix.Nov 13 2018, 23:51
schancel removed a subscriber: deadalnix.
This revision is now accepted and ready to land.Nov 13 2018, 23:51
akb accepted this revision.Nov 14 2018, 00:02
akb added a subscriber: akb.

Looks pretty straightforward to my newbie eyes.

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

nit: and -> an

schancel updated this revision to Diff 5818.Nov 14 2018, 00:04

Fix release-notes nit

teamcity added a comment.Nov 14 2018, 00:06

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

deadalnix accepted this revision.Nov 14 2018, 00:08
teamcity added a comment.Nov 14 2018, 00:14

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

Harbormaster failed remote builds in B3902: Diff 5818!Nov 14 2018, 00:15
teamcity added a comment.Nov 14 2018, 00:17

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

teamcity added a comment.Nov 14 2018, 00:25

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

Harbormaster completed remote builds in B3902: Diff 5818.Nov 14 2018, 00:25
Closed by commit rABCb3abb4892641: [mining] Ensure sigops and fees follow transactions during sorting (authored by schancel). · Explain WhyNov 14 2018, 02:35
This revision was automatically updated to reflect the committed changes.
schancel mentioned this in D2065: Fix warnings about shadowing introduced in D2047.Nov 14 2018, 03:31
schancel mentioned this in rABCaa9ee30af1f3: Fix warnings about shadowing introduced in D2047.Nov 14 2018, 19:32
schancel mentioned this in rSTAGINGaa9ee30af1f3: Fix warnings about shadowing introduced in D2047.Nov 30 2018, 21:31
markblundeberg mentioned this in D5019: CreateNewBlock: insert entries into block slightly earlier so that correct size is logged.Jan 19 2020, 03:35
PiRK mentioned this in D9602: refactor: Remove unused BlockAssembler::pblock member var.May 28 2021, 14:30
PiRK mentioned this in rABC0fb5b7364afe: refactor: Remove unused BlockAssembler::pblock member var.May 31 2021, 19:59

Revision Contents
Changeset List

PathSize
doc/
3 lines
src/
13 lines
41 lines
rpc/
7 lines
test/
functional/
122 lines
160 lines

Diff 5795

View Options

doc/release-notes.md

Loading...
View Options

src/miner.h

Loading...
View Options

src/miner.cpp

Loading...
View Options

src/rpc/mining.cpp

Loading...
View Options

test/functional/abc-magnetic-anomaly-mining.py

Loading...
View Options

test/functional/timing.json

Loading...