Runtime Policy instead of hardcoded constants #68
Conversation
jennijuju
requested review from
elmattic and
raulk
and removed request for
elmattic
There was a problem hiding this comment.
This looks correct, but we should consider doing less. The number of deadlines per proving period, declarations, peer ID maximums, etc. don't really need to be configurable. The main things that need to be configurable are:
- Most things involving epochs. Delays, etc.
- Proof types.
I'm happy to go through the list with you if you want.
Specifically, so we can make this a bit less invasive. Although I'm not sure how much less invasive we'd end up making it. |
| @@ -142,17 +142,19 @@ impl Actor { | |||
| .map(|address| resolve_control_address(rt, address)) | |||
| .collect::<Result<_, _>>()?; | |||
|
|
|||
| let policy = rt.policy(); | |||
There was a problem hiding this comment.
nit: Might as well do this above 133 so everyone can use the local variable to cut down on three function calls
There was a problem hiding this comment.
you would think!!!
thats what i had in the beginning, but then i met the borrow checker and he screamed at me...
| )); | ||
| } | ||
| { | ||
| let policy = rt.policy(); |
There was a problem hiding this comment.
nit: move to directly precede the code block using it
actors should not be allowd to change the policy
arajasek
force-pushed
the
feat/runtime-policy
branch
from
cb5565b
to
a09c18f
Compare
|
Rebased on master |
actors/miner/src/lib.rs
Outdated
| Vec::with_capacity(rt.policy().wpost_period_deadlines as usize); | ||
| for _ in 0..rt.policy().wpost_period_deadlines { | ||
| decls_by_deadline.push(Vec::new()); | ||
| } |
There was a problem hiding this comment.
The right way to write this is vec![Vec::new(); rt.policy().wpost_period_deadlines as usize];.
There was a problem hiding this comment.
Well, that doesn't work because no clone... I could implement clone, but the version I pushed also works.
| .map_err(|e| { | ||
| e.downcast_default( | ||
| ExitCode::ErrSerialization, | ||
| "failed to assign proving period offset", | ||
| ) | ||
| })?; |
| )); | ||
| } | ||
| { | ||
| let policy = rt.policy(); |
There was a problem hiding this comment.
I concur with @Stebalien that this extracted far too much. For next time, it would be better to make a change like this just with a few of the actually troublesome constants, thereby establishing a pattern that we can then move more into after it proves to solve our problems.
But also, the miner actor is not the only one with policy settings. The market and reward actor, to name a couple, have similar settings. So the naming here should be changed to MinerPolicy, or use a nested structure so that there's room for the others too.
| use fvm_shared::clock::ChainEpoch; | ||
| use fvm_shared::clock::EPOCH_DURATION_SECONDS; | ||
|
|
||
| /// Maximum amount of sectors that can be aggregated. |
There was a problem hiding this comment.
These duplicated comments will rot. Please follow-up to delete them.
|
So, it turns out that I lied. We can do this with feature flags as long as the policy lives in a separate crate. In this case, because it lives in the runtime crate, actors can simply enable these extra proof types at compile time with a build flag. |
I offered to reduce it, but @arajasek convinced @Stebalien that all this is fine.
We should move the necessary constants in the policy for the other actors too, there is room to grow. |
This makes the miner policy runtime configurable instead of using hard coded constants, which allows us to write tests that can use different parameters and so on.