Page MenuHomePhabricator

[avalanche] Add a way to pass avalanche config values around
ClosedPublic

Authored by sdulfari on Jul 16 2022, 05:51.

Details

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

Event Timeline

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
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
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.

Fabien added inline comments.
src/avalanche/test/processor_tests.cpp
89

Unrelated, but keeping a ref here is a bit overkill as this is always just an alias to GetConfig(), and it's used at is a single place

This revision is now accepted and ready to land.Jul 25 2022, 09:20