-
Notifications
You must be signed in to change notification settings - Fork 70
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
feat: Add new DefaultMachineRunner trait for initialization work #469
Merged
Conversation
This file contains 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
Previously, all different machines in ckb-vm might use slightly different initialization process. This commit introduces a new DefaultMachineRunner trait, so the same initialization & usage workflow can be used for all types implementing this trait.
9ccf91e
to
a2534eb
Compare
xxuejie
added a commit
to nervosnetwork/ckb-vm-test-suite
that referenced
this pull request
Mar 21, 2025
mohanson
approved these changes
Mar 21, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
xxuejie
added a commit
to xxuejie/ckb-vm
that referenced
this pull request
Mar 21, 2025
Previously, all different machines in ckb-vm might use slightly different initialization process. This commit introduces a new DefaultMachineRunner trait, so the same initialization & usage workflow can be used for all types implementing this trait. This is a cherry-pick of nervosnetwork#469 to the develop branch. 2 more changes are applied on the develop branch: * Since AsmCoreMachine is now constant, we can simply use AsmCoreMachine instead of `Box<AsmCoreMachine>`. * nervosnetwork#343 was mostly reverted. Upon more thinking, I think the change earlier does not make sense now. It's better we provide unified initialization for the whole machine, not just for the memory part. Due to the new InstDecoder / TraceDecoder structure introduced in develop branch, more changes are required in the develop branch than 0.24.x releases. Some of those changes include: * Assembly machines now require TraceDecoder instead of InstDecoder. This means we cannot use DefaultMachineBuilder to build DefaultMachine structures for both types of VMs. The assembly machine types require more constraints. Given this consideration, DefaultMachineBuilder is removed in this commit. 2 new types RustDefaultMachineBuilder and AsmDefaultMachineBuilder are added for 2 types of VMs respectively. * A new set of types that begin with `Abstract`, such as AbstractDefaultMachineBuilder, AbstractAsmMachine, AbstractTraceMachine are introduced. One is only expected to use those types when one wants to tweak the inst/trace decoder used. Otherwise, one is safe to pick AsmMachine/AsmDefaultMachineBuilder or TraceMachine/RustDefaultMachineBuilder. So the current workflow is like following: 1. Pick a type implementing DefaultMachineRunner traits. Most likely, one would pick either AsmMachine or TraceMachine. 2. Instantiate a core machine using DefaultMachineRunner::Inner::new. 3. Based on the type picked in step 1, use AsmDefaultMachineBuilder or RustDefaultMachineBuilder to build a default machine with possible instruction cycle func, syscalls and debugger plugin. 4. Finally, use DefaultMachineRunner::new to create the final runnable machine from the default machine. For advanced usages where the decoder is customized, one can use AbstractAsmMachine or AbstractTraceMachine type. Then use AbstractDefaultMachineBuilder to build the default machine. And finally create the runnable machine. All those types accept an additional InstDecoder / TraceDecoder trait impl that allows one to customize the decoder for optimizations.
xxuejie
added a commit
to xxuejie/ckb-vm
that referenced
this pull request
Mar 21, 2025
Previously, all different machines in ckb-vm might use slightly different initialization process. This commit introduces a new DefaultMachineRunner trait, so the same initialization & usage workflow can be used for all types implementing this trait. This is a cherry-pick of nervosnetwork#469 to the develop branch. 2 more changes are applied on the develop branch: * Since AsmCoreMachine is now constant, we can simply use AsmCoreMachine instead of `Box<AsmCoreMachine>`. * nervosnetwork#343 was mostly reverted. Upon more thinking, I think the change earlier does not make sense now. It's better we provide unified initialization for the whole machine, not just for the memory part. Due to the new InstDecoder / TraceDecoder structure introduced in develop branch, more changes are required in the develop branch than 0.24.x releases. Some of those changes include: * Assembly machines now require TraceDecoder instead of InstDecoder. This means we cannot use DefaultMachineBuilder to build DefaultMachine structures for both types of VMs. The assembly machine types require more constraints. Given this consideration, DefaultMachineBuilder is removed in this commit. 2 new types RustDefaultMachineBuilder and AsmDefaultMachineBuilder are added for 2 types of VMs respectively. * A new set of types that begin with `Abstract`, such as AbstractDefaultMachineBuilder, AbstractAsmMachine, AbstractTraceMachine are introduced. One is only expected to use those types when one wants to tweak the inst/trace decoder used. Otherwise, one is safe to pick AsmMachine/AsmDefaultMachineBuilder or TraceMachine/RustDefaultMachineBuilder. So the current workflow is like following: 1. Pick a type implementing DefaultMachineRunner traits. Most likely, one would pick either AsmMachine or TraceMachine. 2. Instantiate a core machine using DefaultMachineRunner::Inner::new. 3. Based on the type picked in step 1, use AsmDefaultMachineBuilder or RustDefaultMachineBuilder to build a default machine with possible instruction cycle func, syscalls and debugger plugin. 4. Finally, use DefaultMachineRunner::new to create the final runnable machine from the default machine. For advanced usages where the decoder is customized, one can use AbstractAsmMachine or AbstractTraceMachine type. Then use AbstractDefaultMachineBuilder to build the default machine. And finally create the runnable machine. All those types accept an additional InstDecoder / TraceDecoder trait impl that allows one to customize the decoder for optimizations.
xxuejie
added a commit
to xxuejie/ckb-vm
that referenced
this pull request
Mar 21, 2025
Previously, all different machines in ckb-vm might use slightly different initialization process. This commit introduces a new DefaultMachineRunner trait, so the same initialization & usage workflow can be used for all types implementing this trait. This is a cherry-pick of nervosnetwork#469 to the develop branch. 2 more changes are applied on the develop branch: * Since AsmCoreMachine is now constant, we can simply use AsmCoreMachine instead of `Box<AsmCoreMachine>`. * nervosnetwork#343 was mostly reverted. Upon more thinking, I think the change earlier does not make sense now. It's better we provide unified initialization for the whole machine, not just for the memory part. Due to the new InstDecoder / TraceDecoder structure introduced in develop branch, more changes are required in the develop branch than 0.24.x releases. Some of those changes include: * Assembly machines now require TraceDecoder instead of InstDecoder. This means we cannot use DefaultMachineBuilder to build DefaultMachine structures for both types of VMs. The assembly machine types require more constraints. Given this consideration, DefaultMachineBuilder is removed in this commit. 2 new types RustDefaultMachineBuilder and AsmDefaultMachineBuilder are added for 2 types of VMs respectively. * A new set of types that begin with `Abstract`, such as AbstractDefaultMachineBuilder, AbstractAsmMachine, AbstractTraceMachine are introduced. One is only expected to use those types when one wants to tweak the inst/trace decoder used. Otherwise, one is safe to pick AsmMachine/AsmDefaultMachineBuilder or TraceMachine/RustDefaultMachineBuilder. So the current workflow is like following: 1. Pick a type implementing DefaultMachineRunner traits. Most likely, one would pick either AsmMachine or TraceMachine. 2. Instantiate a core machine using DefaultMachineRunner::Inner::new. 3. Based on the type picked in step 1, use AsmDefaultMachineBuilder or RustDefaultMachineBuilder to build a default machine with possible instruction cycle func, syscalls and debugger plugin. 4. Finally, use DefaultMachineRunner::new to create the final runnable machine from the default machine. For advanced usages where the decoder is customized, one can use AbstractAsmMachine or AbstractTraceMachine type. Then use AbstractDefaultMachineBuilder to build the default machine. And finally create the runnable machine. All those types accept an additional InstDecoder / TraceDecoder trait impl that allows one to customize the decoder for optimizations.
xxuejie
added a commit
to nervosnetwork/ckb-vm-test-suite
that referenced
this pull request
Mar 21, 2025
quake
approved these changes
Mar 24, 2025
xxuejie
added a commit
that referenced
this pull request
Mar 24, 2025
* feat: Add new DefaultMachineRunner trait for initialization work Previously, all different machines in ckb-vm might use slightly different initialization process. This commit introduces a new DefaultMachineRunner trait, so the same initialization & usage workflow can be used for all types implementing this trait. This is a cherry-pick of #469 to the develop branch. 2 more changes are applied on the develop branch: * Since AsmCoreMachine is now constant, we can simply use AsmCoreMachine instead of `Box<AsmCoreMachine>`. * #343 was mostly reverted. Upon more thinking, I think the change earlier does not make sense now. It's better we provide unified initialization for the whole machine, not just for the memory part. Due to the new InstDecoder / TraceDecoder structure introduced in develop branch, more changes are required in the develop branch than 0.24.x releases. Some of those changes include: * Assembly machines now require TraceDecoder instead of InstDecoder. This means we cannot use DefaultMachineBuilder to build DefaultMachine structures for both types of VMs. The assembly machine types require more constraints. Given this consideration, DefaultMachineBuilder is removed in this commit. 2 new types RustDefaultMachineBuilder and AsmDefaultMachineBuilder are added for 2 types of VMs respectively. * A new set of types that begin with `Abstract`, such as AbstractDefaultMachineBuilder, AbstractAsmMachine, AbstractTraceMachine are introduced. One is only expected to use those types when one wants to tweak the inst/trace decoder used. Otherwise, one is safe to pick AsmMachine/AsmDefaultMachineBuilder or TraceMachine/RustDefaultMachineBuilder. So the current workflow is like following: 1. Pick a type implementing DefaultMachineRunner traits. Most likely, one would pick either AsmMachine or TraceMachine. 2. Instantiate a core machine using DefaultMachineRunner::Inner::new. 3. Based on the type picked in step 1, use AsmDefaultMachineBuilder or RustDefaultMachineBuilder to build a default machine with possible instruction cycle func, syscalls and debugger plugin. 4. Finally, use DefaultMachineRunner::new to create the final runnable machine from the default machine. For advanced usages where the decoder is customized, one can use AbstractAsmMachine or AbstractTraceMachine type. Then use AbstractDefaultMachineBuilder to build the default machine. And finally create the runnable machine. All those types accept an additional InstDecoder / TraceDecoder trait impl that allows one to customize the decoder for optimizations. * chore: Remove unneeded PhantomData
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Previously, all different machines in ckb-vm might use slightly different initialization process. This commit introduces a new DefaultMachineRunner trait, so the same initialization & usage workflow can be used for all types implementing this trait.