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
Branch
miner_log_fix
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 9056
Build 16074: Default Diff Build & Tests
Build 16073: arc lint + arc unit

Event Timeline

src/miner.cpp
174

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

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

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

194

Is this exposed anywhere?

This revision now requires changes to proceed.Jan 19 2020, 03:51
markblundeberg added inline comments.
src/miner.cpp
174

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

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

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

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

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