Page MenuHomePhabricator
Differential D11761

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

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

Details

Reviewers
Fabien
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

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
sdulfari updated this revision to Diff 34440.Jul 20 2022, 18:51

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

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

Don't copy the structure

This revision now requires changes to proceed.Jul 21 2022, 10:18
sdulfari updated this revision to Diff 34462.Jul 22 2022, 05:28

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
sdulfari updated this revision to Diff 34497.Jul 23 2022, 05:56

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.
src/avalanche/test/processor_tests.cpp
89 ↗(On Diff #34497)

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
Closed by commit rABC72c060adeae9: [avalanche] Add a way to pass avalanche config values around (authored by sdulfari). · Explain WhyJul 25 2022, 21:38
This revision was automatically updated to reflect the committed changes.
sdulfari added a commit: rABC72c060adeae9: [avalanche] Add a way to pass avalanche config values around.
sdulfari added a child revision: D11750: [avalanche] Make proof dust threshold configurable.Jul 28 2022, 05:05

Revision Contents
Changeset List

PathSize
src/
avalanche/
25 lines
9 lines
17 lines
test/
12 lines

Diff 34440

View Options

src/avalanche/avalancheconfig.h

Loading...
View Options

src/avalanche/processor.h

Loading...
View Options

src/avalanche/processor.cpp

Loading...
View Options

src/avalanche/test/processor_tests.cpp

Loading...