Page MenuHomePhabricator

[Chronik] Add `-chronik` and `-chronikbind` command line options
ClosedPublic

Authored by tobias_ruck on Nov 28 2022, 13:58.

Details

Summary

Unless -chronik is specified, Chronik now will not be started.
-chronikbind tells Chronik to bind its HTTP server to the given addresses. Currently, this only logs, in a future diff this will be parsed and listened to.

Test Plan
  1. ninja
  2. ./src/bitcoind -regtest -> No Chronik log
  3. ./src/bitcoind -regtest -chronik -> Starting Chronik bound to [127.0.0.1:18442, [::1]:18442]
  4. ./src/bitcoind -regtest -chronik -chronikbind=0.0.0.0:1234 -> Starting Chronik bound to [0.0.0.0:1234]
  5. ./src/bitcoind -regtest -chronik -chronikbind=1.2.3.4 -chronikbind=\[::5\]:6789 -> Starting Chronik bound to [1.2.3.4:18442, [::0.0.0.5]:6789]
  6. ./src/bitcoind -regtest -chronik -chronikbind=invalid -> Error: Invalid Chronik host address "invalid": invalid IP address syntax

Diff Detail

Repository
rABC Bitcoin ABC
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Fabien requested changes to this revision.Nov 28 2022, 16:09
Fabien added inline comments.
chronik/chronik-cpp/chronik.cpp
20 ↗(On Diff #36749)

Why is the default 127.0.0.1 and not 0.0.0.0 ?
Also there is no input control here, I can set something invalid here and you won't notice. You might as well port the CService class and use the Lookup function to do validation and address/port decoding.

src/init.cpp
629 ↗(On Diff #36749)

If the category is hidden, no need to state it's experimental, and this doc will not need to change when it's ready to deploy

632 ↗(On Diff #36749)
635 ↗(On Diff #36749)

Dito

637 ↗(On Diff #36749)

Please define and mention the default value.
Can it be used several times to bind to multiple hosts ? Like one public IP + localhost ?

638 ↗(On Diff #36749)

I suppose we can run Chronik on testnet as well ? If so you probably don't want the same port, hence make it a network only option (as opposed to a global option)

src/util/system.h
132 ↗(On Diff #36749)

Move the category below HIDDEN for now, it will prevent it from appearing to the user. so we can make it visible only when it's stable enough (see D11912).

src/validation.h
93 ↗(On Diff #36749)

This has nothing to do in validation.h

This revision now requires changes to proceed.Nov 28 2022, 16:09

Refactor setup_chronik, add init_error and return an error if chronikbind is not a valid host address

Allow multiple -chronikbind, parse using SocketAddr/IpAddr

tobias_ruck edited the test plan for this revision. (Show Details)
Fabien requested changes to this revision.Dec 5 2022, 09:39
Fabien added inline comments.
chronik/chronik-lib/src/bridge.rs
55 ↗(On Diff #36966)

I think you're missing the point here. You're basically duplicating (with less features) what already exists in the node and is used for the other -bind options. You should reuse the code that already exists in the node to check the ip:port entries and have consistent error message, then pass the processed content to chronik.

This revision now requires changes to proceed.Dec 5 2022, 09:39

./src/bitcoind -regtest -rpcbind=invalid doesn't return with an error.

My code actually does validation; rpcbind just defaults to localhost if it doesnt parse. Plus, IP parsing is in the stdlib in Rust and the code looks much cleaner to me this way.

Fabien added inline comments.
chronik/chronik-cpp/chronik.h
16–17 ↗(On Diff #36966)
src/validation.h
85 ↗(On Diff #36966)
This revision is now accepted and ready to land.Dec 5 2022, 11:11
tobias_ruck edited the test plan for this revision. (Show Details)

Removed DEFAULT_CHRONIK, which is already in chronik.h

make chronik::DEFAULT_ENABLED constexpr