Details

Reviewers

Fabien

Group Reviewers

Restricted Project

Commits

rABCc30b383e4bf0: [Chronik] Add minimal `chronik::Start` and `chronik::Stop` to init.cpp

Summary

Current implementation only logs. It gives us a scaffold to add the actual indexer.

Test Plan

ninja
./src/bitcoind -regtest prints "Starting Chronik..." in the logs
Ctrl+C to terminate bitcoind prints "Stopping Chronik..." in the logs

Diff Detail

Repository

rABC Bitcoin ABC

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

tobias_ruck created this revision.Jul 26 2022, 19:41

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 26 2022, 19:41

tobias_ruck requested review of this revision.Jul 26 2022, 19:41

Harbormaster completed remote builds in B19731: Diff 34524.Jul 26 2022, 19:57

Fabien requested changes to this revision.Jul 27 2022, 10:50

Fabien added inline comments.

chronik/CMakeLists.txt
81–85 ↗	(On Diff #34524)	This is enough as the include dirs are transitive properties of the targets (if PUBLIC) so when you use target_link_libraries after that your target inherits the include dirs from the right linked targets
91 ↗	(On Diff #34524)	With the above patch this is not needed
chronik/chronik-cpp/chronik.cpp
6 ↗	(On Diff #34524)	Sort. This is also a hint that the linter doesn't run in this directory, you need to include it in the .arclint file.
9 ↗	(On Diff #34524)	Use `[[maybe_unused]]` attributes around the parameters to avoid warnings
chronik/chronik-cpp/chronik.h
4 ↗	(On Diff #34524)	You're missing the include guards, welcome to C/C++
6 ↗	(On Diff #34524)	You can forward declare Config and NodeContext instead.
9 ↗	(On Diff #34524)	You probably want to use a chronik namespace rather than suffixing. That will likely be useful later anyway.

This revision now requires changes to proceed.Jul 27 2022, 10:50

apply Fabien's C++ wisdom (:

Harbormaster completed remote builds in B19737: Diff 34531.Jul 27 2022, 14:35

Build node with Chronik in CI, fix bad StopChronik

Harbormaster completed remote builds in B19739: Diff 34533.Jul 27 2022, 15:04

tobias_ruck retitled this revision from [Chronik] Add minimal `StartChronik` and `StopChronik` to init.cpp to [Chronik] Add minimal `chronik::Start` and `chronik::Stop` to init.cpp.Jul 27 2022, 15:50

tobias_ruck edited the summary of this revision. (Show Details)

Fabien requested changes to this revision.Jul 27 2022, 16:58

Fabien added inline comments.

.arclint
6 ↗	(On Diff #34533)	There are more useful linters that you can enable
chronik/CMakeLists.txt
81–84 ↗	(On Diff #34533)	Align the PUBLIC or make it a one-liner
chronik/chronik-cpp/chronik.cpp
8 ↗	(On Diff #34533)	#include <chronik-cpp/chronik.h>
12 ↗	(On Diff #34533)
chronik/chronik-cpp/chronik.h
5 ↗	(On Diff #34533)	BITCOIN_CHRONIK_CPP_CHRONIK_H The lint-include-guard linter might need to replace - with _, the only case is currently bitcoin-config.h but it's a generated file so not linted
8 ↗	(On Diff #34533)	The whole point of the forward declaration is that you don't need it
17 ↗	(On Diff #34533)