Feat/evm devnet ready#772
Merged
Merged
Conversation
sam0x17
reviewed
Aug 30, 2024
sam0x17
left a comment
sam0x17
left a comment
Collaborator
There was a problem hiding this comment.
looks good, a few areas I think could be improved / explored
Comment on lines
+119
to
+123
| Ok(FrontierPartialComponents { | ||
| filter_pool: Some(Arc::new(Mutex::new(BTreeMap::new()))), | ||
| fee_history_cache: Arc::new(Mutex::new(BTreeMap::new())), | ||
| fee_history_cache_limit: config.fee_history_limit, | ||
| }) |
Collaborator
There was a problem hiding this comment.
Would be good do discuss the performance implications of having a BTreeMap behind an arc+mutex. Perhaps something like a dashmap would be more appropriate if key ordering doesn't matter? Ideally we use something with more fine-grained locking I would think, though I'm not at all familiar with the access pattern here so this might be reasonable. What is the access pattern?
Contributor
Author
There was a problem hiding this comment.
Didn't think about it, tbh. This comes from a Polkadot-SDK repo:
Collaborator
There was a problem hiding this comment.
gotcha. The litmus test would be lots of contention => dashmap
Comment on lines
+127
to
+208
| pub async fn spawn_frontier_tasks( | ||
| task_manager: &TaskManager, | ||
| client: Arc<FullClient>, | ||
| backend: Arc<FullBackend>, | ||
| frontier_backend: Arc<FrontierBackend<FullClient>>, | ||
| filter_pool: Option<FilterPool>, | ||
| storage_override: Arc<dyn StorageOverride<Block>>, | ||
| fee_history_cache: FeeHistoryCache, | ||
| fee_history_cache_limit: FeeHistoryCacheLimit, | ||
| sync: Arc<SyncingService<Block>>, | ||
| pubsub_notification_sinks: Arc< | ||
| fc_mapping_sync::EthereumBlockNotificationSinks< | ||
| fc_mapping_sync::EthereumBlockNotification<Block>, | ||
| >, | ||
| >, | ||
| ) { | ||
| // Spawn main mapping sync worker background task. | ||
| match &*frontier_backend { | ||
| fc_db::Backend::KeyValue(b) => { | ||
| task_manager.spawn_essential_handle().spawn( | ||
| "frontier-mapping-sync-worker", | ||
| Some("frontier"), | ||
| fc_mapping_sync::kv::MappingSyncWorker::new( | ||
| client.import_notification_stream(), | ||
| Duration::new(6, 0), | ||
| client.clone(), | ||
| backend, | ||
| storage_override.clone(), | ||
| b.clone(), | ||
| 3, | ||
| 0, | ||
| fc_mapping_sync::SyncStrategy::Normal, | ||
| sync, | ||
| pubsub_notification_sinks, | ||
| ) | ||
| .for_each(|()| future::ready(())), | ||
| ); | ||
| } | ||
| fc_db::Backend::Sql(b) => { | ||
| task_manager.spawn_essential_handle().spawn_blocking( | ||
| "frontier-mapping-sync-worker", | ||
| Some("frontier"), | ||
| fc_mapping_sync::sql::SyncWorker::run( | ||
| client.clone(), | ||
| backend, | ||
| b.clone(), | ||
| client.import_notification_stream(), | ||
| fc_mapping_sync::sql::SyncWorkerConfig { | ||
| read_notification_timeout: Duration::from_secs(30), | ||
| check_indexed_blocks_interval: Duration::from_secs(60), | ||
| }, | ||
| fc_mapping_sync::SyncStrategy::Parachain, | ||
| sync, | ||
| pubsub_notification_sinks, | ||
| ), | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| // Spawn Frontier EthFilterApi maintenance task. | ||
| if let Some(filter_pool) = filter_pool { | ||
| // Each filter is allowed to stay in the pool for 100 blocks. | ||
| const FILTER_RETAIN_THRESHOLD: u64 = 100; | ||
| task_manager.spawn_essential_handle().spawn( | ||
| "frontier-filter-pool", | ||
| Some("frontier"), | ||
| EthTask::filter_pool_task(client.clone(), filter_pool, FILTER_RETAIN_THRESHOLD), | ||
| ); | ||
| } | ||
|
|
||
| // Spawn Frontier FeeHistory cache maintenance task. | ||
| task_manager.spawn_essential_handle().spawn( | ||
| "frontier-fee-history", | ||
| Some("frontier"), | ||
| EthTask::fee_history_task( | ||
| client, | ||
| storage_override, | ||
| fee_history_cache, | ||
| fee_history_cache_limit, | ||
| ), | ||
| ); | ||
| } |
Collaborator
There was a problem hiding this comment.
this looks good, but we should probably think about adding a mechanism for handling task failures... if any of these tasks fail, it could leave the node in an inconsistent state. Logging and retry mechanisms could be useful here. Basically would feel safer if it was more self-healing
Comment on lines
+211
to
+347
| pub fn create_eth<B, C, BE, P, A, CT, CIDP, EC>( | ||
| mut io: RpcModule<()>, | ||
| deps: EthDeps<B, C, P, A, CT, CIDP>, | ||
| subscription_task_executor: SubscriptionTaskExecutor, | ||
| pubsub_notification_sinks: Arc< | ||
| fc_mapping_sync::EthereumBlockNotificationSinks< | ||
| fc_mapping_sync::EthereumBlockNotification<B>, | ||
| >, | ||
| >, | ||
| ) -> Result<RpcModule<()>, Box<dyn std::error::Error + Send + Sync>> | ||
| where | ||
| B: BlockT<Hash = H256>, | ||
| C: CallApiAt<B> + ProvideRuntimeApi<B>, | ||
| C::Api: AuraApi<B, AuraId> | ||
| + BlockBuilderApi<B> | ||
| + ConvertTransactionRuntimeApi<B> | ||
| + EthereumRuntimeRPCApi<B>, | ||
| C: HeaderBackend<B> + HeaderMetadata<B, Error = BlockChainError>, | ||
| C: BlockchainEvents<B> + AuxStore + UsageProvider<B> + StorageProvider<B, BE> + 'static, | ||
| BE: Backend<B> + 'static, | ||
| P: TransactionPool<Block = B> + 'static, | ||
| A: ChainApi<Block = B> + 'static, | ||
| CT: ConvertTransaction<<B as BlockT>::Extrinsic> + Send + Sync + 'static, | ||
| CIDP: CreateInherentDataProviders<B, ()> + Send + 'static, | ||
| EC: EthConfig<B, C>, | ||
| { | ||
| use fc_rpc::{ | ||
| pending::AuraConsensusDataProvider, Debug, DebugApiServer, Eth, EthApiServer, EthDevSigner, | ||
| EthFilter, EthFilterApiServer, EthPubSub, EthPubSubApiServer, EthSigner, Net, NetApiServer, | ||
| Web3, Web3ApiServer, | ||
| }; | ||
| #[cfg(feature = "txpool")] | ||
| use fc_rpc::{TxPool, TxPoolApiServer}; | ||
|
|
||
| let EthDeps { | ||
| client, | ||
| pool, | ||
| graph, | ||
| converter, | ||
| is_authority, | ||
| enable_dev_signer, | ||
| network, | ||
| sync, | ||
| frontier_backend, | ||
| storage_override, | ||
| block_data_cache, | ||
| filter_pool, | ||
| max_past_logs, | ||
| fee_history_cache, | ||
| fee_history_cache_limit, | ||
| execute_gas_limit_multiplier, | ||
| forced_parent_hashes, | ||
| pending_create_inherent_data_providers, | ||
| } = deps; | ||
|
|
||
| let mut signers = Vec::new(); | ||
| if enable_dev_signer { | ||
| signers.push(Box::new(EthDevSigner::new()) as Box<dyn EthSigner>); | ||
| } | ||
|
|
||
| io.merge( | ||
| Eth::<B, C, P, CT, BE, A, CIDP, EC>::new( | ||
| client.clone(), | ||
| pool.clone(), | ||
| graph.clone(), | ||
| converter, | ||
| sync.clone(), | ||
| signers, | ||
| storage_override.clone(), | ||
| frontier_backend.clone(), | ||
| is_authority, | ||
| block_data_cache.clone(), | ||
| fee_history_cache, | ||
| fee_history_cache_limit, | ||
| execute_gas_limit_multiplier, | ||
| forced_parent_hashes, | ||
| pending_create_inherent_data_providers, | ||
| Some(Box::new(AuraConsensusDataProvider::new(client.clone()))), | ||
| ) | ||
| .replace_config::<EC>() | ||
| .into_rpc(), | ||
| )?; | ||
|
|
||
| if let Some(filter_pool) = filter_pool { | ||
| io.merge( | ||
| EthFilter::new( | ||
| client.clone(), | ||
| frontier_backend.clone(), | ||
| graph.clone(), | ||
| filter_pool, | ||
| 500_usize, // max stored filters | ||
| max_past_logs, | ||
| block_data_cache.clone(), | ||
| ) | ||
| .into_rpc(), | ||
| )?; | ||
| } | ||
|
|
||
| io.merge( | ||
| EthPubSub::new( | ||
| pool, | ||
| client.clone(), | ||
| sync, | ||
| subscription_task_executor, | ||
| storage_override.clone(), | ||
| pubsub_notification_sinks, | ||
| ) | ||
| .into_rpc(), | ||
| )?; | ||
|
|
||
| io.merge( | ||
| Net::new( | ||
| client.clone(), | ||
| network, | ||
| // Whether to format the `peer_count` response as Hex (default) or not. | ||
| true, | ||
| ) | ||
| .into_rpc(), | ||
| )?; | ||
|
|
||
| io.merge(Web3::new(client.clone()).into_rpc())?; | ||
|
|
||
| io.merge( | ||
| Debug::new( | ||
| client.clone(), | ||
| frontier_backend, | ||
| storage_override, | ||
| block_data_cache, | ||
| ) | ||
| .into_rpc(), | ||
| )?; | ||
|
|
||
| #[cfg(feature = "txpool")] | ||
| io.merge(TxPool::new(client, graph).into_rpc())?; | ||
|
|
||
| Ok(io) | ||
| } |
Collaborator
There was a problem hiding this comment.
I recommend breaking this up into helper functions, especially if we think this will grow to be even longer
Comment on lines
+96
to
+103
| pub fn bytes_to_account_id(account_id_bytes: &[u8]) -> Result<AccountId32, PrecompileFailure> { | ||
| AccountId32::from_slice(account_id_bytes).map_err(|_| { | ||
| log::info!("Error parsing account id bytes {:?}", account_id_bytes); | ||
| PrecompileFailure::Error { | ||
| exit_status: ExitError::InvalidRange, | ||
| } | ||
| }) | ||
| } |
Mz/feat/staking precompile
sam0x17
added
the
skip-cargo-audit
orriin
reviewed
Oct 31, 2024
Comment on lines
+1049
to
+1052
| parameter_types! { | ||
| pub const SubtensorChainId: u64 = 0x03B1; // Unicode for lowercase alpha | ||
| // pub const SubtensorChainId: u64 = 0x03C4; // Unicode for lowercase tau | ||
| } |
unconst
approved these changes
Oct 31, 2024
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add EVM functionality to the chain
Related Issue(s)
n/a
Type of Change
Breaking Change
n/a
Checklist
cargo fmtandcargo clippyto ensure my code is formatted and linted correctlyScreenshots (if applicable)
n/a
Additional Notes
Testing can be done with instructions here:
https://github.com/gztensor/evm-demo
fixes #862