feat(congestion_control) - Make the parameters part of the runtime config #11307
Conversation
|
@jakmeier Can you have a quick look please? Finishing it will be time consuming so I want to avoid that if you have some better suggestions. The idea is as follows:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #11307 +/- ##
==========================================
+ Coverage 71.00% 71.08% +0.08%
==========================================
Files 782 783 +1
Lines 156476 156831 +355
Branches 156476 156831 +355
==========================================
+ Hits 111099 111487 +388
+ Misses 40553 40516 -37
- Partials 4824 4828 +4
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
|
Yeah this looks very reasonable to me and makes testing things much nicer. Plus, of course, future changes to the parameter values. And yes, it's going to take some time to get everything in place with the new parameters and such. But it should be manageable. If you get stuck anywhere, don't hesitate to ping me on Zulip, as I'm quite familiar with the runtime parameter setup. |
There was a problem hiding this comment.
Generally looks good to me so far to make congestion control configurable.
But the part about making it a runtime parameter is still incomplete. Personally, I would rather not do it in separate PRs in this case, because reviewing it much harder if we first merge it in "the wrong way" and are supposed to fix it later. It's just to easy to use the wrong config in some places.
There was a problem hiding this comment.
Looking good now.
To pass CI, you will also need to update all the old protocol version runtime config snapshot JSON files. In case you don't know already, running the test with cargo insta test --accept allows you to update all the files once.
I think the same test also updates parameters.snap, which should already show the new parameters with the dummy values.
| # 0.25 | ||
| reject_tx_congestion_threshold: { | ||
| old : { numerator: 1, denominator: 1 }, | ||
| new : { numerator: 25, denominator: 100 } |
There was a problem hiding this comment.
Maybe reduce it?
| new : { numerator: 25, denominator: 100 } | |
| new : { numerator: 1, denominator: 4 } |
But I don't know, maybe percentage is better.
There was a problem hiding this comment.
Personally I like % a bit more
This is part one of making the congestion control configurable. Here I'm introducing the Config and Control classes for congestion control and some refactoring. In the next part I'll actually make it properly configurable based on protocol versions. I decided to split it into smaller PRs because it's large enough as is and I'm a bit tired for today ;)