Conversation
liamaharon
force-pushed
the
node-decentralization
branch
5 times, most recently
from
f054964 to
d572164
Compare
liamaharon
force-pushed
the
node-decentralization
branch
from
d572164 to
6129f4c
Compare
shamil-gadelshin
left a comment
shamil-gadelshin
left a comment
There was a problem hiding this comment.
Great work! I tested the runtime upgrade using baedeker tools, and it worked like a charm!
As discussed offline, please, prepare a separate PR for patch->fork migration.
I left a couple of minor comments. I will likely have more review rounds and revisit babe configuration again.
It makes sense to start preparing a test plan using a separate network.
| >; | ||
| type BenchmarkingConfig = runtime_common::elections::BenchmarkConfig; | ||
| type ForceOrigin = EnsureRoot<Self::AccountId>; | ||
| type WeightInfo = (); |
There was a problem hiding this comment.
Please, restore all weights. We'll add other weights later.
| let next_pending_node_validator_emissions = node_validator_block_emission | ||
| .to_num::<u64>() | ||
| .saturating_add(PendingNodeValidatorEmissions::<T>::get().into()); | ||
| PendingNodeValidatorEmissions::<T>::set(next_pending_node_validator_emissions.into()); |
There was a problem hiding this comment.
How to receive these emissions? I don't see this infrastructure part.
There was a problem hiding this comment.
Do you plan to introduce it later in the following PRs?
There was a problem hiding this comment.
Emissions are paid at the end of each era. It is already implemented:
Lines 610 to 620 in a20e696
| total_block_emission.saturating_sub(node_validator_block_emission); | ||
|
|
||
| // Increment pending validator emissions to be paid out at the end of the era. | ||
| let next_pending_node_validator_emissions = node_validator_block_emission |
There was a problem hiding this comment.
Did you consider mutate here?
There was a problem hiding this comment.
I didn't, what is the benefit?
| let keys = SessionKeys { | ||
| babe: babe_id.clone(), | ||
| grandpa: sr25519_to_ed25519(babe_id.clone()) | ||
| .expect("Failed to map Babe ID to Grandpa ID") |
There was a problem hiding this comment.
We shouldn't have panics in the runtime code.
There was a problem hiding this comment.
I think in this situation we actually want to runtime to panic, because we do not want the chain to continue with a misconfigured / accidental set of Babe authorities.
Furthermore, we have try-runtime checks to ensure we have not misconfigured this mapping. If the .expect triggers, it means some very bad undefined behavior is occurring that must be investigated manually.
The alternative would be to brick it without a panic (e.g. not setting any authorities), but I think a panic is the most obvious way to call to our attention somehow the authorities are misconfigured.
Happy to discuss if you have any better ideas how to proceed in this scenario.
| .saturating_add(<T as frame_system::Config>::DbWeight::get().writes(1)) | ||
| .saturating_add(<T as frame_system::Config>::DbWeight::get().reads(0_u64)), | ||
| DispatchClass::Operational, | ||
| Pays::No |
There was a problem hiding this comment.
Why should sudo account pay? Also why have this be Pays::Yes and the rest of the admin utils use Pays::No?
liamaharon
force-pushed
the
node-decentralization
branch
from
e39c3e5 to
b40af33
Compare
3 participants
Phase 3 in #1887
Now that we have a Hybrid Node that supports seamless Aura to Babe upgrades and block imports, we are finally ready to enact the Aura PoA to Babe NPoS runtime upgrade.
This PR includes
run_coinbaseto set aside NPoS validator emissionsI have tested upgrades with fresh and baedeker runtimes.
Steps for this PR
Once confirmed no issues with the above, we can merge.