-
Notifications
You must be signed in to change notification settings - Fork 22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
chore(consensus): add network config #1808
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. Join @dan-starkware and the rest of your teammates on Graphite |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1808 +/- ##
===========================================
- Coverage 40.10% 11.74% -28.36%
===========================================
Files 26 286 +260
Lines 1895 33348 +31453
Branches 1895 33348 +31453
===========================================
+ Hits 760 3918 +3158
- Misses 1100 29167 +28067
- Partials 35 263 +228 ☔ View full report in Codecov by Sentry. |
3705f80
to
f0db78b
Compare
bd4dcc9
to
8f5b1d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dan-starkware, @Itay-Tsabary-Starkware, and @matan-starkware)
crates/sequencing/papyrus_consensus/src/config.rs
line 84 at r1 (raw file):
impl Default for ConsensusConfig { fn default() -> Self { let mut network_config = NetworkConfig::default();
consider instead
let network_config = NetworkConfig {
tcp_port: 10100,
quic_port: 10101,
..Default::default()
};
Previously, ShahakShama wrote…
I don't think it can work since secret key is private |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-starkware, @matan-starkware, and @ShahakShama)
crates/sequencing/papyrus_consensus/src/config.rs
line 87 at r1 (raw file):
// TODO(Dan/Shahak): consider something nicer, maybe a builder? network_config.tcp_port = 10100; network_config.quic_port = 10101;
Can these be made the default values of NetworkConfig
?
Code quote:
network_config.tcp_port = 10100;
network_config.quic_port = 10101;
Previously, Itay-Tsabary-Starkware wrote…
WDYM? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dan-starkware, @matan-starkware, and @ShahakShama)
crates/sequencing/papyrus_consensus/src/config.rs
line 87 at r1 (raw file):
Previously, dan-starkware wrote…
WDYM?
They are set this way in
config/papyrus/default_config.json
Can NetworkConfig::default()
return a struct whose values are
tcp_port = 10100;
quic_port = 10101;
?
Previously, Itay-Tsabary-Starkware wrote…
IMO, since we have more than one component that's using it, each component should have different default values. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aa
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dan-starkware and @ShahakShama)
f0db78b
to
c27ce71
Compare
8f5b1d4
to
a0cc4ca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @Itay-Tsabary-Starkware, @matan-starkware, and @ShahakShama)
crates/sequencing/papyrus_consensus/src/config.rs
line 37 at r1 (raw file):
Previously, matan-starkware wrote…
If it's just adding a macro annotation, why not do that now?
I'm not sure what is the right validation (and whether one is needed) for each field.
I suggest to do it for all of the fields in a designated PR
crates/sequencing/papyrus_consensus/src/config.rs
line 85 at r1 (raw file):
Previously, matan-starkware wrote…
How did you choose these ports?
We have a utility for selecting a free portfind_free_port
.
If you prefer the constants, should we move these to the top of the file instead of inline magic numbers?
Agree. There should be a constants source somewhere, for now just moving to local consts
c27ce71
to
135c179
Compare
a0cc4ca
to
133dc43
Compare
d3b9ad8
to
86230ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed all commit messages.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @Itay-Tsabary-Starkware and @ShahakShama)
86230ee
to
8e6331c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewable status: 0 of 6 files reviewed, all discussions resolved (waiting on @Itay-Tsabary-Starkware, @matan-starkware, and @ShahakShama)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 1 of 1 files at r2, 5 of 5 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @dan-starkware)
No description provided.