Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABC05e860392659: [refactor] move ReadBlockFromDisk+ from validation.cpp

Summary

Upcoming Core PR16443 makes validation.h #include <txdb.h>, which has
the unwelcome effect of making zmq transitively #include <leveldb/db.h>
which cmake helpfully doesn't make available to it.

turns out zmqpublishnotifier.cpp (the only translation unit requiring
validation.h) needs it only for ReadBlockFrom disk.

This looks like a good place to start to cut things out of
the validation TU which is basically responsible for everything right
now.

This diff moves a few functions related to handling block files into the
(tentatively named) readblockfromdisk.cpp translation unit so that
zmqpublishnotifier.cpp no longer pulls leveldb transitively.

Test Plan

ninja check check-functional

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

majcosta created this revision.Sep 27 2020, 20:07

Herald added a reviewer: Restricted Project. · View Herald TranscriptSep 27 2020, 20:07

majcosta requested review of this revision.Sep 27 2020, 20:07

[Bot Message]
One or more PR numbers were detected in the summary.
Links to those PRs have been inserted into the summary for reference.

majcosta added a child revision: D7580: [backport#16443 1/3] refactor: pcoinsTip -> CChainState::CoinsTip().Sep 27 2020, 20:14

Harbormaster completed remote builds in B12939: Diff 23829.Sep 27 2020, 20:15

the #include <validation.h> in rpcwallet.cpp creates the same problem later on, removing it here

Harbormaster completed remote builds in B12941: Diff 23831.Sep 27 2020, 20:37

This is not a bug, this a feature. A pice of code shoudn't transitively import implementation details such as the DB used in it's client.

src/readblockfromdisk.h
17 ↗	(On Diff #23831)	You don't need this in the header.
20 ↗	(On Diff #23831)	You can make this constexpr while you are at it. It is also somewhat concerning that this is in the header. Is really used anywhere else?
32 ↗	(On Diff #23831)	Because you only manipulate block by ref, you can forward declare.

This revision now requires changes to proceed.Sep 27 2020, 22:04

moved declaration of the global cs_main muted to cpp file and made BLOCKFILE_CHUNK_SIZE constexpr. it is still being used by static void FindFilesToPrune in validation, itself a good candidate for refactoring, given that it is currently doing two things: finding which block files it can prune, and then pruning them.

Harbormaster completed remote builds in B12988: Diff 23910.Sep 28 2020, 19:02

In D7579#180439, @majcosta wrote:

it is still being used by static void FindFilesToPrune in validation, itself a good candidate for refactoring, given that it is currently doing two things: finding which block files it can prune, and then pruning them.

Looks like this pruning thing belong in the new module your are creating. That also means that the name of this module is not accurate. The code is just telling you what needs to be done if you care to look at it.

This revision now requires changes to proceed.Sep 28 2020, 22:10

majcosta mentioned this in T1084: Refactor pruning related code from validation.cpp.Oct 1 2020, 14:53

named the new translation unit to something more appropriate going forward

Harbormaster completed remote builds in B13428: Diff 24673.Oct 15 2020, 18:28

deadalnix requested changes to this revision.Oct 15 2020, 21:53

deadalnix added inline comments.

src/CMakeLists.txt
340 ↗	(On Diff #24673)	It's not pruning. Maybe blockdb.h/cpp or something.

This revision now requires changes to proceed.Oct 15 2020, 21:53

renamed new TU

deadalnix requested changes to this revision.Oct 15 2020, 22:28

deadalnix added inline comments.

src/blockdb.h
8 ↗	(On Diff #24690)	This is not needed.
10 ↗	(On Diff #24690)	This is also not needed.
src/validation.cpp
4487 ↗	(On Diff #24690)	Sounds like this is the exact same code ith the same structure, so you need to move it too, or we'll end up not only with repetition, but it will now be invisible.