Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Reload the MoveOS instance automatically for the gas schedule upgrade events. #2432

Merged
merged 15 commits into from
Aug 17, 2024

Conversation

steelgeek091
Copy link
Collaborator

resolve #2399

Copy link

vercel bot commented Aug 14, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
rooch-portal ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 16, 2024 7:11am
1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
rooch ⬜️ Ignored (Inspect) Visit Preview Aug 16, 2024 7:11am

@@ -88,6 +97,26 @@ impl Handler<ExecuteViewFunctionMessage> for ReaderExecutorActor {
let function_result = self
.moveos()
.execute_view_function(self.root.clone(), msg.call);

if let Some(event_bus) = self.event_bus.clone() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

是不是不应该在 ViewFuntion 里去Reload ReaderEexcutor 的 MoveOS 实例

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

execute_view_function 函数里面吗?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

有一个思路,把 moveos reload 放到一个 actor 消息里执行,event bus notify 时候触发,不过怎么在 notify 时触发执行 actor 消息,这个地方还得想想

pub fn notify<T: 'static + Send + Clone>(&self, event_data: T) {
let event_type_id = TypeId::of::<T>();
{
let senders = self.senders.read().unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

直接 unwrap 容易导致 panic

system_post_execute_functions(),
self.event_bus.clone(),
)?;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

既然有订阅机制,应该是从 EventBus push 消息过来,不应该每次都从 event bus 拉取消息。
感觉应该把 EventBus 封装在 actor 里,EventBus actor send Event 给 subscriber,subscriber 实现一个 handle 去处理消息就可以。

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

好,我修改一下。

Copy link
Collaborator Author

@steelgeek091 steelgeek091 Aug 14, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

尝试用 EventActor 封装消息发送,遇到了一些不好处理的地方:

  1. 建立一个 Executor::prepare_load 这个回调函数,注册到 EventBus:
event_bus.subscribe::<GasUpgradeEventMessage, _>("executor", move |_| {
        let _ = block_on(ExecutorActor::prepare_reload(executor.clone().into()));
    });

需要处理在闭包中调用 async 函数的问题。

Executor::prepare_load 函数如下:

pub async fn prepare_reload(executor_actor_ref: ActorRef<ExecutorActor>) -> Result<()> {
        executor_actor_ref.send(GasUpgradeEventMessage {}).await?
    }

Executor 收到 GasUpgradeEventMessage 类型消息后 reload MoveOS.

  1. 如果注册类似与 executor.reload 函数到 EventBus,在这个函数内直接 reload MoveOS,则需要将 Executor 实例使用 Arc<Muext<>> 封装,因为要保存到 EventBus 中,并且保存到 Precessor 中。

目前第一个方案,初步测试无法收到消息,可能 block_on 方式运行有问题。
@jolestar

如果不用 block_on 函数,则需要在闭包中运行 async 函数,类似于 async move || { xxx.await } 这种形式,目前 Rust 还不支持。

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

不要用回调函数,EventBus Actor 直接把消息通过 actor_ref 转发给订阅者。订阅者订阅的时候也用自己的 actor ref 来订阅。这样就没有闭包的难题。

where
F: Fn(Box<dyn Any + Send>) + Send + Sync + 'static,
{
self.event_bus.subscribe::<T, F>(subscriber, callback)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里传递订阅者的 Actor Ref 进来,就不需要 callback 了。

@@ -134,6 +143,13 @@ impl ExecutorActor {
.moveos_store
.handle_tx_output(tx_hash, output.clone())?;

if self.moveos.cost_table.read().is_none() {
if let Some(actor_ref) = self.event_actor_ref.clone() {
let _ = block_on(actor_ref.send(GasUpgradeEventMessage {}));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里可以用 actor_ref.notify 就不需要 block_on 了。

@@ -63,6 +66,7 @@ impl ExecutorActor {
rooch_store: RoochStore,
registry: &Registry,
event_bus: Option<EventBus>,
event_actor_ref: Option<ActorRef<EventActor>>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

感觉这里可以要求 ActorRef 必须作为参数传递。另外有了 ActorRef,应该就不需要 event_bus: Option<EventBus> EventBus 参数了。EventBus 应该被封装在 EventActor 中了。

pub struct MoveOS {
vm: MoveOSVM,
pub db: MoveOSStore,
pub cost_table: Arc<RwLock<Option<CostTable>>>,
system_pre_execute_functions: Vec<FunctionCall>,
system_post_execute_functions: Vec<FunctionCall>,
event_bus: Option<EventBus>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MoveOS 应该不需要关心 EventBus 吧?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

是的,应该从 MoveOS 中移除。

Comment on lines 545 to 547
if let Some(event_bus) = self.event_bus.clone() {
event_bus.notify::<GasUpgradeEvent>(GasUpgradeEvent {});
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

这里把 gas scheduled updated 信息通过 output 返回就可以了。

crates/rooch-event/src/actor.rs Outdated Show resolved Hide resolved
crates/rooch-event/src/actor.rs Outdated Show resolved Hide resolved
crates/rooch-executor/src/actor/executor.rs Outdated Show resolved Hide resolved
@jolestar jolestar merged commit 61cbc13 into rooch-network:main Aug 17, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Gas] Global notification mechanism for Gas schedule upgrade events.
3 participants