Implement Version Upgrade Mechanism #3096

paberr · 2024-11-18T21:26:59Z

What's in this pull request?

This PR implements the mechanism to carry out breaking changes (version upgrades) after launch.

It mostly follows the specification in the Albatross paper with one exception:
We will not only check for the proportion of stake supporting the upgrade, but also for the proportion of slots supporting the upgrade. This makes sure we don't propose an upgrade in an epoch where less than 2f+1 elected validators support the upgrade.

⚠️ All changes in this PR that affect the consensus pre-version upgrade are backwards compatible in that the current nodes enforce stricter rules. All consensus breaking changes would only ever be performed post-version upgrade (e.g., deactivating validators).

Mechanism Overview

The version upgrade mechanism introduces the following key changes, discussed in a bit more detail below:

Modified block validity (i.e., when upgrades are possible)
Tools for checking supporting stake/slots
Validators producing blocks with new version
New version upgrade inherent (deactivating unsupporting validators)
Moving validator selection in election blocks from before the accounts commit to after the accounts commit

Block validity

The version number is stored in every block. We allow version upgrades only in election blocks.
Instead of checking against a constant version from the Policy, block-level verification of the version is now split into two parts:

Block::verify_header: Making sure that the version is always below or equal to the maximum supported version Policy::max_supported_version(network_id). The maximum supported version depends on the network, so that we can roll out changes in the testnet first.
Block:: verify_immediate_successor and Block::verify_macro_successor: Making sure that the version number can only increase in election blocks and only in steps of one. For all other scenarios, the version needs to match the predecessor's version. We test this in both functions as these are called in different scenarios.

Helper functions

We only perform version upgrades when at least Policy:: UPGRADE_MIN_SUPPORT% of the active stake signals support for the upgrade and when at least Policy::TWO_F_PLUS_ONE slots of the currently elected validators signals support.
We add helper functions to determine the supporting stake and slots.

The get_supporting_stake function is computationally expensive in that it iterates over all validators in the staking contract. This number is bounded by the validator deposit and we only call the function when proposing an election block and not being on the maximum version already.

Currently, the support for a version is signalled by encoding the supported version in the first two bytes of the validator's signal data (in big-endian). This can still be changed later and be implemented version-specific in the Policy::supports_upgrade function.

We also add a function to deactivate unsupportive stake, which is discussed in the section about the new inherent.

Producing an upgrade

Validators who updated their client and have a maximum supported version higher than the current blockchain's version (determined from the head block) will determine support for this upgrade whenever they are the ones to produce a macro proposal. Only when the conditions (supporting_stake >= Policy:: UPGRADE_MIN_SUPPORT * active stake && supporting_slots > Policy::TWO_F_PLUS_ONE) are met, the proposal will contain the increased version number. If the proposal is accepted, the upgrade is successfully active from this block on (including this election block).

There is no explicit check for validators to only vote for a block if they support the upgrade. If they updated their client and have a new maximum supported version, the proposal will be deemed valid and voted for. Validators not supporting the upgrade won't update their clients and will reject the proposal due to an invalid version.

Version upgrade inherent

The mechanism adds a new inherent VersionUpgrade that also has the new version. It has the side effect of deactivating all validators that did not explicitly signal support for the new version with immediate effect. This also means that these unsupporting validators should not be selected for the next epoch (thus preventing possible stalls).

Full nodes will apply this side-effect through trie-diffs. The inherent is not stored in the history and thus does not require any special handling.

Switching validator selection and accounts commit

In the current model, we first select the validators for the next epoch (based on the current staking contract) and then apply the accounts changes of an election block. The only accounts changes possible in an election block right now are: reward distribution and preparing the bitsets in the staking contract for the next epoch.

Since none of these have an effect on the validator selection, we switch the order of these two. This is required so that unsupporting validators are deactivated before the validator selection.

Tests

This PR includes unit tests in the following crates:

validator: Testing that the validator will propose an election block with a new version if the conditions are met.
block: Testing the new validity rules.
staking_contract: Testing correct computation of stake/slot support and correct handling of inherent.
blockchain: Testing that the blockchain correctly emits the new inherent.

Pull request checklist

All tests pass. The project builds and runs.
I have resolved any merge conflicts.
I have resolved all clippy and rustfmt warnings.

hrxi

I wonder if we should somehow support the case of someone not wanting an upgrade. In that case, they should probably not signal support for it — however, this means that they'll get deactivated on a version upgrade. This puts them in a bad spot. Could we maybe add a "I have technical support for this upgrade but I don't like it" flag that allows a validator to vote against an upgrade while not getting deactivated if it does get activated?

hrxi · 2025-02-12T13:48:55Z

primitives/src/policy.rs

+
+            _ => 1,
+        }
+    }


This should probably be a member variable of Policy instead.

validator/src/tendermint.rs

hrxi · 2025-02-13T15:06:39Z

validator/src/tendermint.rs

+
+            // We propose an upgraded block version only if:
+            // - `Policy::UPGRADE_MIN_SUPPORT` of the stake supports the upgrade.
+            // - `Policy::TWO_F_PLUS_ONE` slots support the upgrade.


Maybe also only upgrade when 80% of the slots support the upgrade? Otherwise, a few slots can successfully reject this proposal.

primitives/src/policy.rs

validator/src/tendermint.rs

hrxi · 2025-02-13T15:12:29Z

blockchain/tests/inherents.rs

+        validators: None,
+        next_batch_initial_punished_set,
+        ..Default::default()
+    };


Most of this struct can probably be omitted since you specify ..Default::default() anyway.

blockchain/src/blockchain/inherents.rs

jsdanielh · 2025-02-14T00:32:16Z

blockchain/src/blockchain/slots.rs

+        let txn = txn.or_new(&self.db);
+        let staking_contract = self
+            .get_staking_contract_if_complete(Some(&txn))
+            .expect("We should always have the staking contract.");


Suggested change

.expect("We should always have the staking contract.");

.expect("We should always have the staking contract");

Is this actually guaranteed? Is there no way for next_validators to be called during state sync?

It feels overly restrictive and obfuscated for this function to require the staking contract to be present this way. I would prefer to have the staking contract as a parameter, making this requirement explicit. All call sites of next_validators have already called get_staking_contract_if_complete before calling next_validators anyways.

primitives/Cargo.toml

primitives/block/src/block.rs

nibhar

Left some comments mostly for clarity. Functionally this looks good to me.

blockchain/src/blockchain/history_sync.rs

blockchain/src/blockchain/inherents.rs

blockchain/src/blockchain/history_sync.rs

nibhar · 2025-02-15T17:13:30Z

blockchain/src/blockchain/slots.rs

+        let txn = txn.or_new(&self.db);
+        let staking_contract = self
+            .get_staking_contract_if_complete(Some(&txn))
+            .expect("We should always have the staking contract.");


Is this actually guaranteed? Is there no way for next_validators to be called during state sync?

It feels overly restrictive and obfuscated for this function to require the staking contract to be present this way. I would prefer to have the staking contract as a parameter, making this requirement explicit. All call sites of next_validators have already called get_staking_contract_if_complete before calling next_validators anyways.

nibhar · 2025-02-15T17:23:57Z

blockchain/src/blockchain/verify.rs

@@ -143,6 +143,22 @@ impl Blockchain {
        if let Block::Macro(macro_block) = block {
            // If we don't have the staking contract, there is nothing we can check.
            if let Some(staking_contract) = self.get_staking_contract_if_complete(Some(txn)) {
+                // Verify validators.
+                let validators = match macro_block.is_election() {
+                    true => Some(self.next_validators(&macro_block.header.seed, Some(txn))),


See other comment about the staking contract requirement.

blockchain/tests/push.rs

light-blockchain/tests/push.rs

nibhar · 2025-02-15T18:02:03Z

primitives/account/src/account/staking_contract/mod.rs

+        StakingContractStoreRead::new(data_store)
+            .iter_validators()
+            .filter_map(|validator| {
+                if validator.is_active() && support_check(validator.signal_data) {
+                    Some(validator.total_stake)
+                } else {
+                    None
+                }
+            })
+            .sum()


Alternatively:

Suggested change

StakingContractStoreRead::new(data_store)

.iter_validators()

.filter_map(|validator| {

if validator.is_active() && support_check(validator.signal_data) {

Some(validator.total_stake)

} else {

None

}

})

.sum()

let staking_Contract_read = StakingContractStoreRead::new(data_store);

self.active_validators

.iter()

.filter_map(|(validator_address, stake)| {

let validator_signal_data = staking_contract_read

.get_validator(address)

.expect("Active Validators must be present in the Staking Contract.")

.signal_data;

support_check(validator_signal_data).then_some(*stake)

})

.sum()

Should also work. It shifts from going over all validator into going over only active validators and querying their signal data individually. May even be worse, just left it here for consideration.

nibhar · 2025-03-10T15:54:55Z

rpc-interface/src/types.rs

@@ -340,7 +340,6 @@ pub struct PolicyConstants {
    pub coinbase_address: String,
    pub transaction_validity_window: u32,
    pub max_size_micro_body: usize,
-    pub version: u16,


This seems useful, why was it removed? Maybe rename it to max_supported_version?

nibhar · 2025-03-10T16:01:53Z

validator/src/tendermint.rs

+            // - `Policy::TWO_F_PLUS_ONE` slots support the upgrade.
+            if supporting_slots >= Policy::TWO_F_PLUS_ONE
+                && u64::from(supporting_stake) * 100 / u64::from(total_active_stake)
+                    >= Policy::UPGRADE_MIN_SUPPORT as u64


Maybe that constant should be a u64 to begin with.

Add function for supporting slots

Produce upgraded blocks after gathering enough support

Implement upgrade test Add block tests Add accounts tree tests and fix block number Test creation of inherent

paberr added the enhancement New feature or request label Nov 18, 2024

paberr self-assigned this Nov 18, 2024

paberr requested a review from hrxi November 18, 2024 21:42

paberr force-pushed the pb/upgrade branch 3 times, most recently from cad6edb to 42076ca Compare November 19, 2024 01:59

paberr requested review from styppo and nibhar November 19, 2024 02:10

hrxi reviewed Feb 13, 2025

View reviewed changes

jsdanielh reviewed Feb 14, 2025

View reviewed changes

nibhar reviewed Mar 10, 2025

View reviewed changes

paberr and others added 6 commits March 24, 2025 10:52

Add helper functions for upgrades

847b280

Add function for supporting slots

Allow version upgrades in election blocks

b1ecc3e

Make validator selection dependent on accounts

5e72a92

Remove unsupporting validators after upgrade

c6c6baa

Produce upgraded blocks after gathering enough support

Make max supported version dependent on network

ff7a8e8

Implement upgrade test Add block tests Add accounts tree tests and fix block number Test creation of inherent

Implement some of the feedback

4f06efc

paberr force-pushed the pb/upgrade branch from 42076ca to 4f06efc Compare March 24, 2025 13:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Version Upgrade Mechanism #3096

Implement Version Upgrade Mechanism #3096

paberr commented Nov 18, 2024

hrxi left a comment

hrxi Feb 12, 2025

hrxi Feb 13, 2025

hrxi Feb 13, 2025

jsdanielh Feb 14, 2025

nibhar Feb 15, 2025

nibhar left a comment

nibhar Feb 15, 2025

nibhar Feb 15, 2025

nibhar Feb 15, 2025

nibhar Mar 10, 2025

nibhar Mar 10, 2025

	.expect("We should always have the staking contract.");
	.expect("We should always have the staking contract");

-        StakingContractStoreRead::new(data_store)
-            .iter_validators()
-            .filter_map(|validator| {
-                if validator.is_active() && support_check(validator.signal_data) {
-                    Some(validator.total_stake)
-                } else {
-                    None
-                }
-            })
-            .sum()
+        let staking_Contract_read = StakingContractStoreRead::new(data_store);
+        self.active_validators
+            .iter()
+            .filter_map(|(validator_address, stake)| {
+                let validator_signal_data = staking_contract_read
+                    .get_validator(address)
+                    .expect("Active Validators must be present in the Staking Contract.")
+                    .signal_data;
+                support_check(validator_signal_data).then_some(*stake)
+            })
+            .sum()

Implement Version Upgrade Mechanism #3096

Are you sure you want to change the base?

Implement Version Upgrade Mechanism #3096

Conversation

paberr commented Nov 18, 2024

What's in this pull request?

Mechanism Overview

Block validity

Helper functions

Producing an upgrade

Version upgrade inherent

Switching validator selection and accounts commit

Tests

Pull request checklist

hrxi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nibhar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment