Page MenuHomePhabricator

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

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

Details

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
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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?

(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 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 marked an inline comment as done.

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

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

This revision is now accepted and ready to land.Jan 20 2020, 22:41