feat: basic global contract support #12155

bowenwang1996 · 2024-09-26T16:40:37Z

A first step towards near/NEPs#556. This PR implements the following:

Global contract shard that stores account id -> global contract mapping. It uses the existing MPT state implementation with state root included in block headers.
DeployPermanentContractAction that allows an account to deploy a global contract. The contract is immediately compiled upon deployment and only gets deployed if it can be compiled successfully.
The logic of propagating the deployment information from runtime to chain to update global contract root and back from chain to runtime to charge properly for new transactions.

A few important things that are not implemented in this PR:

DeployExistingContractAction which references a global contract
The logic of actually propagating deployed global contract code across the network to each node.

This PR is light on testing because DeployExistingContractAction is not yet implemented and it is hard to test on the interface level as a result. Most of the testing focuses on whether global contract root is properly updated.

wacban

That is a big PR ;)
Looks good but I only did a high level review.

random thoughts:

gc, cold storage support are not really needed as the contracts are forever
on distribution side we'll need to cover two cases
- continuous distribution of the changes
- something similar to state sync for a fresh node to get the entire cache in one go
It would be nice to have the global shard a bit more generic than just contracts, there is a good chance we may want to reuse it later.
nit: I prefer global over permanent it's shorter and it's the most common name for this feature

wacban · 2024-09-27T09:57:18Z

chain/chain/src/chain_update.rs

+                    let trie_key = TrieKey::Account { account_id: account_id.clone() };
+                    trie_update.set(trie_key, contract_hash.into());


This feels a bit hacky. The account key in regular trie stores values in the Account format. Here you use a different format but using the same trie key.

Also I don't quite see why do you need the account id in the contract shard at all. I thought the whole point is to allow any account to use the contract. In that case it would make sense to use the contract hash as the key.

I don't think the type byte is needed at all now but it may be a good idea to introduce it just in case we want to add other type of keys in the future. Still I think the trie_keys for regular shards and the global contract shard should be separate.

+1 for not using the existing key with different type of value.

Could we use a new TrieKey for this, or TrieKey::ContractCode {account_id} for this? (though for the latter the value is the code itself, not the hash of it).

wacban · 2024-09-27T10:29:49Z

chain/chain/src/chain_update.rs

+    fn update_global_shard(
+        &mut self,
+        block: &Block,
+        protocol_version: ProtocolVersion,


nit: I think it would be a bit cleaner to move the protocol version check outside of this method. Currently just by looking at the method signature it's not clear if None is a valid return value that means "no changes" or if it's only valid when this feature is disabled.

wacban · 2024-09-27T10:36:15Z

chain/chain/src/store/mod.rs

+        // first read from DB to get the slice. Then try to parse it as `BlockExtra`.
+        // If that parsing fails, try again to parse it as `VersionedBlockExtra`.
+        // This avoids the migrate the column `DBCol::BlockExtra`.


Why not migration?

Is it certain that a VersionedBlockExtra cannot be incorrectly parsed as BlockExtra? Is it an invariant that we can ensure holds in the future versions of VersionedBlockExtra?

Another thought: Currently BlockExtra only contains ChallengesResult. Is that ever used? If not, could we simply clean out existing data and start writing VersionedBlockExtra only?

wacban · 2024-09-27T10:40:19Z

chain/chain/src/types.rs

+    /// New permanent contracts metadata.
+    pub permanent_contracts_metadata: Vec<(AccountId, CryptoHash)>,


nit: Would be nice to add changes somewhere in the name though it's quite long already.

wacban · 2024-09-27T10:42:28Z

chain/client/src/client.rs

@@ -926,6 +930,7 @@ impl Client {
            outgoing_receipts_root,
            tx_root,
            congestion_info,
+            chunk_extra.permanent_contracts_metadata().to_vec(),


nit: Why is to_vec() needed? It is already a vector.

wacban · 2024-09-27T11:20:25Z

runtime/runtime/src/actions.rs

@@ -674,6 +676,38 @@ pub(crate) fn action_deploy_contract(
    Ok(())
 }

+/// Compiles the permanent contract and store the compiled module in the cache.


Is it a common practice to compile contracts on deployment? Is it meant to filter out invalid contracts?

wacban · 2024-09-27T11:22:13Z

runtime/runtime/src/config.rs

+                fees.fee(ActionCosts::deploy_contract_base).send_fee(sender_is_receiver)
+                    + fees.fee(ActionCosts::deploy_contract_byte).send_fee(sender_is_receiver)


I think should you be using the costs for the global contracts here.

wacban · 2024-09-27T11:22:20Z

runtime/runtime/src/config.rs

+            fees.fee(ActionCosts::deploy_contract_base).exec_fee()
+                + fees.fee(ActionCosts::deploy_contract_byte).exec_fee() * num_bytes


wacban · 2024-09-27T11:26:14Z

core/parameters/res/runtime_configs/145.yaml

@@ -0,0 +1 @@
+storage_contract_code_burnt_amount_per_byte: { old: 0, new: 100_000_000_000_000_000_000}


Since the cost is quite high, does it implicitly impose a smaller limit on the contract size?

wacban · 2024-09-27T11:27:05Z

runtime/runtime/src/config.rs

@@ -150,6 +157,13 @@ pub fn total_send_fees(
                        &delegate_action.receiver_id,
                    )?
            }
+            #[cfg(feature = "protocol_feature_global_contracts")]
+            DeployPermanentContract(DeployPermanentContractAction { code }) => {


What about the other new action? deploy existing contract?

tayfunelmas

Some comments. In general looks good. One thing needs discussion is the part about deploying from existing contract using contract hash and ContractIdentifier part.

tayfunelmas · 2024-09-30T09:02:31Z

chain/chain/src/chain_update.rs

@@ -208,6 +214,8 @@ impl<'a> ChainUpdate<'a> {
                        balance_burnt,
                        // TODO(congestion_control) - integration with resharding
                        None,
+                        // TODO: integrate with resharding


can we add some issue ID to these TODOs to locate/filter later?
eg. // TODO(#1234): ...

tayfunelmas · 2024-09-30T09:03:58Z

chain/chain/src/chain_update.rs

+        &mut self,
+        block: &Block,
+        protocol_version: ProtocolVersion,
+    ) -> Result<Option<CryptoHash>, Error> {


nit. Use StateRoot type instead of CryptoHash (same type but cleaner).

tayfunelmas · 2024-09-30T09:04:05Z

chain/chain/src/chain_update.rs

+    fn update_global_shard(
+        &mut self,
+        block: &Block,
+        protocol_version: ProtocolVersion,


tayfunelmas · 2024-09-30T09:13:25Z

chain/chain/src/chain_update.rs

+                    let trie_key = TrieKey::Account { account_id: account_id.clone() };
+                    trie_update.set(trie_key, contract_hash.into());


+1 for not using the existing key with different type of value.

Could we use a new TrieKey for this, or TrieKey::ContractCode {account_id} for this? (though for the latter the value is the code itself, not the hash of it).

tayfunelmas · 2024-09-30T09:17:26Z

chain/chain/src/chain_update.rs

+        let protocol_version =
+            self.epoch_manager.get_epoch_protocol_version(block.header().epoch_id())?;
+
+        if let Some(state_root) = self.update_global_shard(block, protocol_version)? {


I would prefer this method returning (StateRoot, WrappedTrieChanges) pair and self.chain_store_update.save_trie_changes be called here (together with save_block_extra), to match the rest of the code here and be clear what is saved in chain_store.

tayfunelmas · 2024-09-30T09:46:45Z

core/primitives/src/action/mod.rs

+/// If account id is specified, then it means that the contract deployed by a specific account will always be used
+/// In this case the contract might change since the deployer account can decide to update the contract.
+/// If contract hash is used, then it means that the contract specified by this hash will be used. In this case the
+/// contract deployed cannot be changed by an external account.


In the latter case (using contract hash), I would expect that the deployment would turn out to be local deployment of that code (ie. like issuing a DeployContractAction with the given code but not changing the global shard but updating the shard of that account). Then the only contract identifier would be AccountId. In that case, an account A can deploy a permanent contract and another account B can deploy that existing contract in global shard. Whenever A changes the contract, B will start using the new code. If B wants its contract not changed, it will deploy that code to its own shard like another other DeployContractAction. In this case, all contracts deployed in the global shard are shared, if the deployer account updates it, everyone gets the update.

tayfunelmas · 2024-09-30T09:50:34Z

core/primitives/src/sharding.rs

+                tx_root,
+                prev_validator_proposals,
+                congestion_info,
+                new_permanent_contracts: permanent_contracts_metadata,


maybe we could make it shorter permanent_contracts but use a specific type for the (accountid, hash) pairs like PermanentContractsMetadata. see my other comment about representing the list of these pairs with a specific struct type.

tayfunelmas · 2024-09-30T09:52:12Z

core/primitives/src/types.rs

 #[derive(Debug, PartialEq, BorshSerialize, BorshDeserialize, Clone, Eq, ProtocolSchema)]
 pub struct BlockExtra {
    pub challenges_result: ChallengesResult,
 }

+#[derive(Debug, PartialEq, BorshSerialize, BorshDeserialize, Clone, Eq, ProtocolSchema)]
+pub struct BlockExtraV1 {
+    pub global_shard_state_root: CryptoHash,


see my other comment about ChallengesResult, if it is not used before, maybe we can cleanup the existing BlockExtra and only use the new type (I expect it will not impact the protocol and hashing of other types).

tayfunelmas · 2024-09-30T09:56:03Z

core/store/src/trie/global.rs

+    }
+}
+
+pub enum GlobalTrieKey {


Why not extend existing TrieKey enum for this (with new variant names).

tayfunelmas · 2024-09-30T09:58:39Z

core/primitives/src/errors.rs

@@ -586,6 +586,8 @@ pub enum ActionErrorKind {
    DelegateActionNonceTooLarge { delegate_nonce: Nonce, upper_bound: Nonce },
    /// Non-refundable storage transfer to an existing account is not allowed according to NEP-491.
    NonRefundableTransferToExistingAccount { account_id: AccountId },
+    /// An error occurs during the deployment of a contract. Currently only used for permanent contract deployment errors due to legacy reasons.
+    CompilationError { account_id: AccountId, err_msg: String },


also add the hash of the code

tayfunelmas · 2024-10-14T07:36:52Z

core/primitives/src/sharding.rs

@@ -439,12 +460,23 @@ impl ShardChunkHeader {
        }
    }

+    #[inline]
+    pub fn permanent_contracts_metadata(&self) -> &[(AccountId, CryptoHash)] {


to be consistent in naming, can be rename permanent_contracts_metadata to global_contracts_metadata?

tayfunelmas · 2024-10-14T07:37:24Z

core/primitives/src/sharding.rs

+                tx_root,
+                prev_validator_proposals,
+                congestion_info,
+                new_permanent_contracts: permanent_contracts_metadata,


actually can we name it global_contracts_metadata? (permanent --> global)

feat: basic global contract support

70a7637

bowenwang1996 requested a review from a team as a code owner September 26, 2024 16:40

bowenwang1996 requested review from akhi3030 and tayfunelmas September 26, 2024 16:40

wacban reviewed Sep 27, 2024

View reviewed changes

tayfunelmas reviewed Sep 30, 2024

View reviewed changes

tayfunelmas requested a review from pugachAG October 14, 2024 07:31

tayfunelmas reviewed Oct 14, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: basic global contract support #12155

feat: basic global contract support #12155

bowenwang1996 commented Sep 26, 2024

wacban left a comment

wacban Sep 27, 2024

tayfunelmas Sep 30, 2024

wacban Sep 27, 2024

tayfunelmas Sep 30, 2024

wacban Sep 27, 2024

tayfunelmas Sep 30, 2024

wacban Sep 27, 2024

wacban Sep 27, 2024

wacban Sep 27, 2024

wacban Sep 27, 2024

wacban Sep 27, 2024

wacban Sep 27, 2024

wacban Sep 27, 2024

tayfunelmas left a comment

tayfunelmas Sep 30, 2024

tayfunelmas Sep 30, 2024

tayfunelmas Sep 30, 2024

tayfunelmas Sep 30, 2024

tayfunelmas Sep 30, 2024

tayfunelmas Sep 30, 2024

tayfunelmas Sep 30, 2024

tayfunelmas Sep 30, 2024

tayfunelmas Sep 30, 2024

tayfunelmas Sep 30, 2024

tayfunelmas Oct 14, 2024

tayfunelmas Oct 14, 2024

		let trie_key = TrieKey::Account { account_id: account_id.clone() };
		trie_update.set(trie_key, contract_hash.into());

		/// New permanent contracts metadata.
		pub permanent_contracts_metadata: Vec<(AccountId, CryptoHash)>,

		fees.fee(ActionCosts::deploy_contract_base).send_fee(sender_is_receiver)
		+ fees.fee(ActionCosts::deploy_contract_byte).send_fee(sender_is_receiver)

		@@ -0,0 +1 @@
		storage_contract_code_burnt_amount_per_byte: { old: 0, new: 100_000_000_000_000_000_000}

feat: basic global contract support #12155

Are you sure you want to change the base?

feat: basic global contract support #12155

Conversation

bowenwang1996 commented Sep 26, 2024

wacban 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tayfunelmas 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

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

Choose a reason for hiding this comment