Page MenuHomePhabricator
Differential D5019

CreateNewBlock: insert entries into block slightly earlier so that correct size is logged
ClosedPublic
Actions

Authored by markblundeberg on Jan 18 2020, 17:12.

Details

Reviewers
deadalnix
Group Reviewers
Restricted Project
Commits
rSTAGING8217e774678f: CreateNewBlock: insert entries into block slightly earlier so that correct size…
rABC8217e774678f: CreateNewBlock: insert entries into block slightly earlier so that correct size…
Summary

I noticed the log message was always printing "CreateNewBlock(): total size: 81 ...",
which is a bit silly.

This moves the pblock population back to where it belongs, right after the
entries are determined. To make this work, the coinbase tx is inserted
into pblock directly which is also what we want (and this goes back to what
Core does).

Test Plan

ninja check-all

Diff Detail

Repository
rABC Bitcoin ABC
Branch
miner_log_fix
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9062
Build 16086: Default Diff Build & Tests
Build 16085: arc lint + arc unit

Event Timeline

markblundeberg created this revision.Jan 18 2020, 17:12
Herald added a reviewer: Restricted Project. · View Herald TranscriptJan 18 2020, 17:12
Herald added a reviewer: deadalnix. · View Herald Transcript
teamcity added a comment.Jan 18 2020, 17:12

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Jan 18 2020, 17:16

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9056: Diff 15654.Jan 18 2020, 17:17
markblundeberg added inline comments.Jan 18 2020, 17:17
src/miner.cpp
174 ↗(On Diff #15654)

A question for another diff:

What is the performance hit that is suggested by this comment? Is it from the shared_ptr atomic operations being slow? I can't see why they would be slow, not on x86 at least.

I wonder if we could just do a pblock->vtx.push_back(std::move(tx.tx)) here instead, does that prevent the performance hit?

markblundeberg added a comment.Jan 19 2020, 03:35

(For reference, this mechanism was introduced in D2047 which was basically a good fix.)

deadalnix requested changes to this revision.Jan 19 2020, 03:51

Requesting changes, but it's more like questions.

src/miner.cpp
174 ↗(On Diff #15654)

It's O(n) plus memory management, which can be reduced by preallocating. Not sure how big the hit is, but for sure it's not optimal either.

177 ↗(On Diff #15654)

If you are going to do this, at the very least preallocate.

194 ↗(On Diff #15654)

Is this exposed anywhere?

This revision now requires changes to proceed.Jan 19 2020, 03:51
markblundeberg marked an inline comment as done.Jan 19 2020, 04:37
markblundeberg added inline comments.
src/miner.cpp
174 ↗(On Diff #15654)

Hmm yeah the prealloc would make sense.

O(N) seems to be not a big deal in this context. We have O(N log N) sorting just above, and N is even just the number of transactions, where some other operations scan over each input/output as well. There is even TestBlockValidity which does quite a bit. If there are some easy wins we can do it I guess.

177 ↗(On Diff #15654)

I'm going to leave the optimziation for a later Diff, this one I just want to be correct and at least no worse performance than before.

194 ↗(On Diff #15654)

Yes, the full list of entries is returned. However this CBlockTemplateEntry is not a Core thing so we basically have nothing that actually inspect the returned .tx field. Anyway, for this Diff to be safe I'll fill in that entry, we can look at optimizing things later.

markblundeberg updated this revision to Diff 15664.Jan 19 2020, 04:42
markblundeberg marked an inline comment as done.

simplify changes to smallest possible (move-only on the for loop, and an extra update)

teamcity added a comment.Jan 19 2020, 04:42

Build Bitcoin-ABC / Diffs / Diff Testing started.

teamcity added a comment.Jan 19 2020, 04:47

Build Bitcoin-ABC / Diffs / Diff Testing passed.

Harbormaster completed remote builds in B9062: Diff 15664.Jan 19 2020, 04:47
markblundeberg added a child revision: D5022: CreateNewBlock: small tweaks.Jan 19 2020, 05:55
markblundeberg retitled this revision from CreateNewBlock: insert entries into block slightly earlier to CreateNewBlock: insert entries into block slightly earlier so that correct size is logged.Jan 19 2020, 05:57
deadalnix requested changes to this revision.Jan 19 2020, 14:04
deadalnix added inline comments.
src/miner.cpp
177 ↗(On Diff #15654)

Just call reserve. It's one line of code.

This revision now requires changes to proceed.Jan 19 2020, 14:04
markblundeberg requested review of this revision.Jan 20 2020, 04:16
markblundeberg marked 3 inline comments as done.
markblundeberg added inline comments.
src/miner.cpp
177 ↗(On Diff #15654)

See D5022, I promise I'll land it right after this one. :-D

deadalnix accepted this revision.Jan 20 2020, 22:41
This revision is now accepted and ready to land.Jan 20 2020, 22:41
markblundeberg edited the summary of this revision. (Show Details)Jan 21 2020, 08:36
Closed by commit rABC8217e774678f: CreateNewBlock: insert entries into block slightly earlier so that correct size… (authored by Mark Lundeberg <markblundeberg@users.noreply.github.com>). · Explain WhyJan 21 2020, 08:40
This revision was automatically updated to reflect the committed changes.
Mark Lundeberg <markblundeberg@users.noreply.github.com> added a commit: rABC8217e774678f: CreateNewBlock: insert entries into block slightly earlier so that correct size….
Mark Lundeberg <markblundeberg@users.noreply.github.com> added a commit: rSTAGING8217e774678f: CreateNewBlock: insert entries into block slightly earlier so that correct size….Apr 7 2020, 22:35

Revision Contents
Changeset List

PathSize
src/
15 lines

Diff 15664

View Options

src/miner.cpp

Loading...