Details

Reviewers

Group Reviewers

Restricted Project

Commits

rABC72c060adeae9: [avalanche] Add a way to pass avalanche config values around

Summary

As the avalanche module grows, it is becoming more challenging to pass around
config values without making a mess as these values pass between multiple
levels of dependencies. This patch adds an AvalancheConfig class for passing
these parameters around without needing to modify every intermediate API.

queryTimeoutDuration is used as a proof of concept, but more impact will be
evident in voting and proof verification parameters.

Test Plan

ninja check

Diff Detail

Repository

rABC Bitcoin ABC

Branch

ava-config

Lint

Lint Passed

Unit

No Test Coverage

Build Status

Buildable 19712
Build 39143: Build Diff	lint-circular-dependencies · build-diff · build-debug · build-without-wallet · build-clang · build-clang-tidy
Build 39142: arc lint + arc unit

Event Timeline

sdulfari created this revision.Jul 16 2022, 05:51

Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 16 2022, 05:51

Harbormaster completed remote builds in B19654: Diff 34405.Jul 16 2022, 06:08

Move away from a mutable config design since changing the config during runtime
leads to unnecessary complexity due to race conditions.

sdulfari published this revision for review.Jul 20 2022, 18:52

Harbormaster completed remote builds in B19681: Diff 34440.Jul 20 2022, 19:08

Fabien requested changes to this revision.Jul 21 2022, 10:18

Fabien added a subscriber: Fabien.

Fabien added inline comments.

src/avalanche/avalancheconfig.h
21 ↗	(On Diff #34440)	I think we always put the private attributes first then the public ones (especially since it's a class). Note: if you are using const attributes then a struct with public attributes should be enough, no need for a class nor accessors
src/avalanche/processor.cpp
138 ↗	(On Diff #34440)	Don't copy the structure

This revision now requires changes to proceed.Jul 21 2022, 10:18

feedback

Harbormaster completed remote builds in B19693: Diff 34462.Jul 22 2022, 05:54

Fabien requested changes to this revision.Jul 22 2022, 08:06

Fabien added inline comments.

src/avalanche/avalancheconfig.h
12 ↗	(On Diff #34462)	Config is enough, you already have the avalanche namespace. avalanche::Config is cleaner than AvalancheConfig or avalanche::AvalancheConfig imo. Same goes for the file name.
src/avalanche/processor.cpp
333 ↗	(On Diff #34462)	move it here as well, otherwise it's still a copy

This revision now requires changes to proceed.Jul 22 2022, 08:06

Feedback

I'm not entirely convinced renaming AvalancheConfig -> Config is the best path because of the way
Config and ::Config both appear in avalanche code. But it's not all bad since it is nearly
impossible to pick the wrong one and get past compilation. I'm still posting this change here
so it will get reviewed.

Harbormaster completed remote builds in B19712: Diff 34497.Jul 23 2022, 06:13

Fabien accepted this revision.Jul 25 2022, 09:20

Fabien added inline comments.