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

refactor(core-code) Optimisation of read-write structures for code instrumentation #4274

Open
wants to merge 35 commits into
base: master
Choose a base branch
from

Conversation

Lazark0x
Copy link
Member

@Lazark0x Lazark0x commented Oct 1, 2024

Code structure constructor now returns 3 parameters: InstrumentedCode, OriginalCode and CodeMetadata

CodeMetadata contains all the necessary data to infer the need for code instrumentation

Rewritten precharge to type-state pattern

@Lazark0x Lazark0x added the A0-pleasereview PR is ready to be reviewed by the team label Oct 1, 2024
@Lazark0x Lazark0x self-assigned this Oct 1, 2024
@Lazark0x Lazark0x requested a review from breathx October 1, 2024 22:07
Copy link
Member

@breathx breathx left a comment

Choose a reason for hiding this comment

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

will be refactored as discussed in dm

@breathx breathx added A1-inprogress Issue is in progress or PR draft is not ready to be reviewed and removed A0-pleasereview PR is ready to be reviewed by the team labels Oct 7, 2024
@breathx breathx added the A0-pleasereview PR is ready to be reviewed by the team label Oct 18, 2024
@Lazark0x Lazark0x changed the title refactor(core-code) InstrumentedCode is made a member of the Code refactor(core-code) Optimisation of read-write structures for code instrumentation Oct 21, 2024
@Lazark0x Lazark0x removed the A1-inprogress Issue is in progress or PR draft is not ready to be reviewed label Oct 21, 2024
@breathx breathx added the A3-gotissues PR occurred to have issues after the review label Nov 4, 2024
@Lazark0x Lazark0x removed the A3-gotissues PR occurred to have issues after the review label Nov 5, 2024
Lazark0x and others added 5 commits December 16, 2024 12:14
…n-refactoring

# Conflicts:
#	ethexe/runtime/common/src/lib.rs
#	gtest/src/manager.rs
#	gtest/src/manager/block_exec.rs
#	pallets/gear-program/src/migrations/add_section_sizes.rs
#	pallets/gear-program/src/migrations/v12_program_code_id_migration.rs
#	pallets/gear/src/tests.rs
#	runtime/vara/src/migrations.rs
# Conflicts:
#	core-processor/src/lib.rs
#	ethexe/runtime/common/src/lib.rs
#	pallets/gear-builtin/src/lib.rs
#	pallets/gear-program/src/migrations/add_section_sizes.rs
#	pallets/gear-program/src/migrations/v12_program_code_id_migration.rs
Copy link
Member

Choose a reason for hiding this comment

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

Remove the migration. It was already executed on chain

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it was executed, it's about transforming H256 into CodeId

pub code_hash: H256,

Copy link
Member

Choose a reason for hiding this comment

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

Migration is removed from the master branch

Comment on lines +125 to +132
if !code_metadata.exports().contains(&dispatch_kind) {
let (destination_id, dispatch, gas_counter, _) = context.into_parts();

// Load instrumented binary code from storage.
let code = T::CodeStorage::get_code(code_id).unwrap_or_else(|| {
// `Program` exists, so do code and code len.
let err_msg = format!(
"run_queue_step: failed to get code for the existing program. \
Program id -'{destination_id:?}', Code id - '{code_id:?}'."
return core_processor::process_success(
SuccessfulDispatchResultKind::Success,
DispatchResult::success(dispatch, destination_id, gas_counter.to_amount()),
);
}
Copy link
Member

Choose a reason for hiding this comment

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

Should be executed after reinstrumentation

Copy link
Member Author

Choose a reason for hiding this comment

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

hmm, is it really possible for exports to change after re-instrumentation?

Copy link
Member Author

Choose a reason for hiding this comment

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

added a note to rewrite in case re-instrumentation will modify exports

# Conflicts:
#	core/src/code/mod.rs
#	ethexe/common/src/db.rs
#	ethexe/db/src/database.rs
#	ethexe/processor/src/host/mod.rs
#	pallets/gear-program/src/migrations/mod.rs
#	pallets/gear-program/src/migrations/v12_program_code_id_migration.rs
#	pallets/gear/src/lib.rs
#	pallets/gear/src/tests.rs
#	runtime/vara/src/migrations.rs
#	utils/calc-stack-height/src/main.rs
#	utils/wasm-builder/src/code_validator.rs
…n-refactoring

# Conflicts:
#	gtest/src/manager.rs
#	gtest/src/manager/memory.rs
…n-refactoring

# Conflicts:
#	ethexe/processor/src/host/mod.rs
#	ethexe/runtime/common/src/lib.rs
#	ethexe/runtime/src/wasm/api/run.rs
#	gcli/src/meta/mod.rs
…n-refactoring

# Conflicts:
#	ethexe/db/src/database.rs
#	runtime/vara/src/migrations.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A0-pleasereview PR is ready to be reviewed by the team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants