Feature: Subintent Execution Implementation Part 1 #1913
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
|
Docker tags |
Benchmark for 38d40a6Click to view benchmark
|
0xOmarA
approved these changes
Sep 13, 2024
0xOmarA
approved these changes
Sep 13, 2024
talekhinezh
changed the base branch from
feature/transaction-processing-v2
to
develop
talekhinezh
force-pushed
the
feature/subintent-impl-1
branch
from
68e6e7a to
76585e4
Compare
dhedey
reviewed
Sep 16, 2024
| txn_threads.execute(api)?; | ||
| let output = txn_threads | ||
| .threads | ||
| .get_mut(0) |
There was a problem hiding this comment.
Perhaps we should change the receipt to have outputs as an IndexMap<IntentHash, Outputs>? Or perhaps even better, separate the root intent and subintent outputs? Have added a test case for this, can do this later.
|
|
||
| pub fn push(&mut self, frame: CallFrame<M::CallFrameData, M::LockData>) { | ||
| let stack = self.stacks.get_mut(self.stack_pointer).unwrap(); | ||
| let parent = mem::replace(&mut stack.current_frame, frame); |
There was a problem hiding this comment.
MINOR: From a code neatness perspective, might be nice if this delegated to stack.push, ditto with stack.pop.
| use radix_engine_interface::blueprints::transaction_processor::{ | ||
| TRANSACTION_PROCESSOR_BLUEPRINT, TRANSACTION_PROCESSOR_RUN_IDENT, | ||
| }; | ||
| use radix_rust::prelude::*; |
There was a problem hiding this comment.
Import wise, lots of this can be replaced with radix_engine::internal_prelude::* to avoid lots of import churn if we move things.
| api.kernel_switch_stack(thread_id)?; | ||
|
|
||
| let mut system_service = SystemService::new(api); | ||
| let virtual_resources = intent |
There was a problem hiding this comment.
We agreed to not use the name virtual for these going forward. Perhaps initial_proof_resources, initial_non_fungible_proofs?
| virtual_non_fungibles, | ||
| )?; | ||
|
|
||
| api.kernel_set_call_frame_data(Actor::Function(FunctionActor { |
There was a problem hiding this comment.
Hmm this feels weird to me - is this right that we're replacing the root call frame with this?
In principle, this isn't a problem, but I think we need to discuss what the abstraction is around Actor::Root and how it interacts with other parts of the codebase (cf comments on #1919 )
| Ok(()) | ||
| } | ||
|
|
||
| pub fn cleanup<Y: SystemBasedKernelApi>(self, api: &mut Y) -> Result<(), RuntimeError> { |
There was a problem hiding this comment.
I think I'd rather run cleanup greedily when each subintent terminates. I think it gives a more intuitive / timely orphaned node error, if a user is thinking of the intents like stack frames.
| cur_thread = parent_stack.pop().unwrap(); | ||
| } | ||
| ResumeResult::Done => { | ||
| if let Some(parent) = parent_stack.pop() { |
There was a problem hiding this comment.
I think I'd like us to be a little stricter here - e.g. set the processor to ProcesserState::Consumed, and ensure that when we yield to child, we check that we're in ProcesserState::Active(processor)
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.
YieldToChildandYieldToParentsupport (though arguments are not yet supported)This is the first part of Subintent execution implementation. Some TODOs for future PRs:
VerifyParentandAssertWorktopIsEmptylock_feein subintents