reconfig with dkg option A stake.move refactoring ver. A #11741
Conversation
⏱️ 1h 7m total CI duration on this PR
🚨 1 job on the last run was significantly faster/slower than expected
|
zjma
force-pushed
the
zjma/reconfig_with_dkg_ver_a_0
branch
from
1028ea8 to
12cc22f
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #11741 +/- ##
===========================================
- Coverage 69.7% 58.0% -11.8%
===========================================
Files 2120 783 -1337
Lines 403595 179338 -224257
===========================================
- Hits 281437 104058 -177379
+ Misses 122158 75280 -46878 ☔ View full report in Codecov by Sentry. |
zjma
changed the title
| force_update_network_and_fullnode_addresses(operator, pool_address, new_network_addresses, new_fullnode_addresses); | ||
| } | ||
|
|
||
| public(friend) fun force_update_network_and_fullnode_addresses( |
There was a problem hiding this comment.
I don't see this function being used elsewhere, why it needs to be a separate function?
| spec { | ||
| assume timestamp::spec_now_seconds() + recurring_lockup_duration_secs <= MAX_U64; | ||
| assume now_secs + recurring_lockup_duration_secs <= MAX_U64; |
There was a problem hiding this comment.
why use now_secs instead of reconfig_start_secs?
There was a problem hiding this comment.
i'm not sure: imagine the DKG took so long that reconfig_start_time + recurring_lockup_duration < reconfig_finish_time, i feel we don't want to set the unlock time to be in the past?
| let cur_pending_inactive = coin::value(&stake_pool.pending_inactive); | ||
|
|
||
| let cur_perf = vector::borrow(&validator_perf.validators, candidate.config.validator_index); | ||
| let cur_reward = if (cur_active > 0) { |
There was a problem hiding this comment.
why we need this condition?
| /// Assuming we are in a middle of a reconfiguration (no matter it is immediate or async), get its start time. | ||
| fun get_reconfig_start_time_secs(): u64 { | ||
| if (reconfiguration_state::is_initialized()) { | ||
| reconfiguration_state::start_time_secs() |
There was a problem hiding this comment.
is it possible for this to be initialized but not used (thus abort)?
There was a problem hiding this comment.
yes, in genesis. Fixed in #11721 at caller side.
| } | ||
|
|
||
| /// Get `ValidatorConsensusInfo.addr`. | ||
| public fun get_addr(vci: &ValidatorConsensusInfo): address { |
There was a problem hiding this comment.
for rust naming convention, we don't use get_ prefix, just fun addr, fun pk_bytes
| /// See `aptos_framework::aptos_governance::reconfigure()` for more details. | ||
| /// | ||
| /// Can only be called by a signer of @std. | ||
| public fun change_feature_flags_for_next_epoch(framework: &signer, enable: vector<u64>, disable: vector<u64>) acquires PendingFeatures, Features { |
There was a problem hiding this comment.
we discussed to let change_feature_flags call this functions and update tests accordingly?
| /// | ||
| /// WARNING: currently only used by tests. In most cases you should use `reconfigure()` instead. | ||
| /// TODO: migrate these tests to be aware of async reconfiguration. | ||
| public fun force_end_epoch(aptos_framework: &signer) { |
There was a problem hiding this comment.
I still think for tests expect this behavior we should just disable the feature instead of adding this function
| @@ -149,6 +153,8 @@ module aptos_framework::reconfiguration { | |||
| epoch: config_ref.epoch, | |||
| }, | |||
| ); | |||
|
|
|||
| reconfiguration_state::on_reconfig_finish(); | |||
There was a problem hiding this comment.
this doesn't call all the on_new_epoch update functions? I thought we want to use the buffer for both cases and just make sure it's applied on_new_epoch
There was a problem hiding this comment.
I feel reconfiguration_state has to be at the very bottom of the dependency graph and can't call them (otherwise circular dependency).
Here's the full graph.
Diff from: #10328
stake::on_new_epoch/update_stake_pooluntouched.next_validator_consensus_infos