Page MenuHomePhabricator

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

Authored by tobias_ruck on Jul 26 2022, 19:41.

Details

Summary

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

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

Diff Detail

Event Timeline

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

Build node with Chronik in CI, fix bad StopChronik

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)
This revision now requires changes to proceed.Jul 27 2022, 16:58

Fix a handful of style stuff

This revision is now accepted and ready to land.Aug 15 2022, 16:13