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

feat: Use stateful instruction in AUTH #22

Merged
merged 36 commits into from Mar 26, 2024

Conversation

fgimenez
Copy link
Member

@fgimenez fgimenez commented Mar 22, 2024

Towards #5

Includes the changes from bluealloy/revm#1194 to use stateful instructions.

  • Bump dependencies
  • Define CustomContext with an address for both AUTH and AUTHCALL
  • Use boxed instructions and capture the custom context in clausures
  • Modify the signature of the functions that implement the instructions to accept the captured context as a parameter
  • Use interior mutability in the AUTH implementation to modify the context's address with the extracted authority (default address if the process failed).

@fgimenez fgimenez changed the base branch from fgimenez/eip2537-precompiles to main March 22, 2024 13:53
@fgimenez fgimenez force-pushed the fgimenez/use-stateful-instruction-in-auth branch from d93216c to 0d4bedd Compare March 22, 2024 13:54
@fgimenez fgimenez marked this pull request as ready for review March 22, 2024 15:02
crates/instructions/src/eip3074.rs Outdated Show resolved Hide resolved
crates/instructions/src/eip3074.rs Outdated Show resolved Hide resolved
crates/node/src/evm.rs Outdated Show resolved Hide resolved
@fgimenez fgimenez force-pushed the fgimenez/use-stateful-instruction-in-auth branch from f27d596 to 0056c71 Compare March 22, 2024 16:10
@fgimenez fgimenez added C-enhancement New feature or request A-instruction Related to an alphanet instruction labels Mar 22, 2024
@fgimenez fgimenez force-pushed the fgimenez/use-stateful-instruction-in-auth branch from 691ae79 to 7ed825b Compare March 25, 2024 15:47
@fgimenez fgimenez force-pushed the fgimenez/use-stateful-instruction-in-auth branch from 7ed825b to 08f6307 Compare March 25, 2024 15:48
crates/instructions/src/context.rs Outdated Show resolved Hide resolved
};

ctx.set_named_variable(AUTORIZED_VAR_NAME, Vec::from(to_persist_authority));
Copy link
Contributor

Choose a reason for hiding this comment

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

I see we're setting this but, we don't read from it yet?

Copy link
Member

@Rjected Rjected Mar 25, 2024

Choose a reason for hiding this comment

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

authcall will read from this, auth doesn't afaik

crates/instructions/src/context.rs Outdated Show resolved Hide resolved
crates/instructions/src/context.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm!

Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

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

LGTM

interp.shared_memory.resize(rounded_size);
}
}
resize_memory!(interp, offset, length);
Copy link
Member

Choose a reason for hiding this comment

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

nice!

Comment on lines +4 to +12
/// Context variables to be used in instructions. The data set here is expected
/// to live for the duration of a single transaction.
/// Similar to TStore for arbitrary data.
pub struct InstructionsContext {
/// Contains the actual variables. Is meant to be accessed both for reads
/// and writes using interior mutability, so that the Instruction and
/// BoxedInstruction signatures are observed.
inner: Rc<RefCell<HashMap<&'static str, Vec<u8>>>>,
}
Copy link
Member

Choose a reason for hiding this comment

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

the docs here are great

Comment on lines +55 to +100
let to_addr = address!("ffffffffffffffffffffffffffffffffffffffff");

// initialize the custom context and make sure it's None for a given key
let custom_context = InstructionsContext::default();
let key = "my-key";
assert_eq!(custom_context.get(key), None);

let to_capture_instructions = custom_context.clone();
let to_capture_post_execution = custom_context.clone();
let mut evm = Evm::builder()
.with_db(InMemoryDB::default())
.modify_db(|db| {
db.insert_account_info(to_addr, AccountInfo::new(U256::ZERO, 0, code_hash, code))
})
.modify_tx_env(|tx| tx.transact_to = TransactTo::Call(to_addr))
.append_handler_register_box(Box::new(move |handler| {
let writer_context = to_capture_instructions.clone();
let writer_instruction = Box::new(
move |_interp: &mut Interpreter, _host: &mut Evm<'_, (), InMemoryDB>| {
// write into the context variable.
writer_context.set(key, vec![0x01, 0x02]);
},
);
let reader_context = to_capture_instructions.clone();
let reader_instruction = Box::new(
move |_interp: &mut Interpreter, _host: &mut Evm<'_, (), InMemoryDB>| {
// read from context variable and clear.
assert_eq!(reader_context.get(key).unwrap(), vec![0x01, 0x02]);
},
);

let mut table = handler.take_instruction_table();
table = table.map(|mut table| {
table.insert_boxed(0xEE, writer_instruction);
table.insert_boxed(0xEF, reader_instruction);
table
});
handler.instruction_table = table;

let post_execution_context = to_capture_post_execution.clone();
#[allow(clippy::arc_with_non_send_sync)]
{
handler.post_execution.end = Arc::new(move |_, outcome: _| {
post_execution_context.clear();
outcome
});
Copy link
Member

Choose a reason for hiding this comment

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

love the test

@fgimenez fgimenez merged commit 569fb93 into main Mar 26, 2024
12 checks passed
@fgimenez fgimenez deleted the fgimenez/use-stateful-instruction-in-auth branch April 6, 2024 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-instruction Related to an alphanet instruction C-enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants