Skip to content

Commit

Permalink
Fix some bugs of CircuitInputBuilder (privacy-scaling-explorations#434
Browse files Browse the repository at this point in the history
)

* fix: use dummy_gen_call_ops for other CALL-family opcodes to enable integration-test

* fix: don't panic when internal call using CALLDATALOAD

* fix: add address into access set when code information is accessed

* feat: use CircuitInputBuilder in test of CALLDATASIZE to make sure it works

* fix: read caller's memory at correct address

* feat: use rand_bytes as tx.input for testing CALLDATASIZE

* refactor: use call_data instead of call_data_source to avoid mistake
han0110 authored Apr 9, 2022

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
1 parent 358b3a6 commit 602e33d
Showing 6 changed files with 191 additions and 128 deletions.
98 changes: 66 additions & 32 deletions bus-mapping/src/circuit_input_builder.rs
Original file line number Diff line number Diff line change
@@ -15,6 +15,7 @@ use eth_types::{
self, Address, GethExecStep, GethExecTrace, Hash, ToAddress, ToBigEndian, Word, U256,
};
use ethers_core::utils::{get_contract_address, get_create2_address};
use itertools::Itertools;
use std::collections::{hash_map::Entry, BTreeMap, HashMap, HashSet};

use crate::rpc::GethClient;
@@ -462,6 +463,9 @@ pub struct CallContext {
/// call. It is incremented when a subcall in this call succeeds by the
/// number of successful writes in the subcall.
pub reversible_write_counter: usize,
/// Call data (copy of tx input or caller's
/// memory[call_data_offset..call_data_offset + call_data_length])
pub call_data: Vec<u8>,
}

/// A reversion group is the collection of calls and the operations which are
@@ -544,7 +548,7 @@ impl TransactionContext {
calls: Vec::new(),
reversion_groups: Vec::new(),
};
tx_ctx.push_call_ctx(0);
tx_ctx.push_call_ctx(0, eth_tx.input.to_vec());

Ok(tx_ctx)
}
@@ -587,7 +591,7 @@ impl TransactionContext {
}

/// Push a new call context and its index into the call stack.
fn push_call_ctx(&mut self, call_idx: usize) {
fn push_call_ctx(&mut self, call_idx: usize, call_data: Vec<u8>) {
if !self.call_is_success[call_idx] {
self.reversion_groups.push(ReversionGroup {
calls: vec![(call_idx, 0)],
@@ -615,6 +619,7 @@ impl TransactionContext {
self.calls.push(CallContext {
index: call_idx,
reversible_write_counter: 0,
call_data,
});
}

@@ -918,11 +923,32 @@ impl<'a> CircuitInputStateRef<'a> {

/// Push a new [`Call`] into the [`Transaction`], and add its index and
/// [`CallContext`] in the `call_stack` of the [`TransactionContext`]
pub fn push_call(&mut self, call: Call) {
let call_id = call.call_id;
pub fn push_call(&mut self, call: Call, step: &GethExecStep) {
let call_data = match call.kind {
CallKind::Call | CallKind::CallCode | CallKind::DelegateCall | CallKind::StaticCall => {
let call_data = if step.memory.0.len() < call.call_data_offset as usize {
&[]
} else {
&step.memory.0[call.call_data_offset as usize..]
};
if call_data.len() < call.call_data_length as usize {
// Expand call_data to expected size
call_data
.iter()
.cloned()
.pad_using(call.call_data_length as usize, |_| 0)
.collect()
} else {
call_data[..call.call_data_length as usize].to_vec()
}
}
CallKind::Create | CallKind::Create2 => Vec::new(),
};

let call_id = call.call_id;
let call_idx = self.tx.calls.len();
self.tx_ctx.push_call_ctx(call_idx);

self.tx_ctx.push_call_ctx(call_idx, call_data);
self.tx.push_call(call);

self.block_ctx
@@ -1659,6 +1685,7 @@ impl From<Vec<Access>> for AccessSet {
}
},
AccessValue::Code { address } => {
state.entry(address).or_insert_with(HashSet::new);
code.insert(address);
}
}
@@ -2354,7 +2381,7 @@ mod tracer_tests {
let mut builder = CircuitInputBuilderTx::new(&block, step);
// Set up call context at CREATE2
builder.tx_ctx.call_is_success.push(false);
builder.state_ref().push_call(mock_internal_create());
builder.state_ref().push_call(mock_internal_create(), step);
// Set up account and contract that exist during the second CREATE2
builder.builder.sdb.set_account(
&ADDR_B,
@@ -2481,7 +2508,7 @@ mod tracer_tests {
let mut builder = CircuitInputBuilderTx::new(&block, step);
// Set up call context at CREATE
builder.tx_ctx.call_is_success.push(false);
builder.state_ref().push_call(mock_internal_create());
builder.state_ref().push_call(mock_internal_create(), step);
assert_eq!(
builder.state_ref().get_step_err(step, next_step).unwrap(),
Some(ExecError::CodeStoreOutOfGas)
@@ -2587,7 +2614,7 @@ mod tracer_tests {
let mut builder = CircuitInputBuilderTx::new(&block, step);
// Set up call context at RETURN
builder.tx_ctx.call_is_success.push(false);
builder.state_ref().push_call(mock_internal_create());
builder.state_ref().push_call(mock_internal_create(), step);
assert_eq!(
builder.state_ref().get_step_err(step, next_step).unwrap(),
Some(ExecError::InvalidCreationCode)
@@ -2694,7 +2721,7 @@ mod tracer_tests {
let mut builder = CircuitInputBuilderTx::new(&block, step);
// Set up call context at RETURN
builder.tx_ctx.call_is_success.push(false);
builder.state_ref().push_call(mock_internal_create());
builder.state_ref().push_call(mock_internal_create(), step);
assert_eq!(
builder.state_ref().get_step_err(step, next_step).unwrap(),
Some(ExecError::MaxCodeSizeExceeded)
@@ -2788,7 +2815,7 @@ mod tracer_tests {
let mut builder = CircuitInputBuilderTx::new(&block, step);
// Set up call context at STOP
builder.tx_ctx.call_is_success.push(false);
builder.state_ref().push_call(mock_internal_create());
builder.state_ref().push_call(mock_internal_create(), step);
assert_eq!(
builder.state_ref().get_step_err(step, next_step).unwrap(),
None
@@ -3287,26 +3314,29 @@ mod tracer_tests {

let mut builder = CircuitInputBuilderTx::new(&block, step);
builder.tx_ctx.call_is_success.push(false);
builder.state_ref().push_call(Call {
call_id: 0,
caller_id: 0,
kind: CallKind::StaticCall,
is_static: true,
is_root: false,
is_persistent: false,
is_success: false,
rw_counter_end_of_reversion: 0,
caller_address: *ADDR_A,
address: *ADDR_B,
code_source: CodeSource::Address(*ADDR_B),
code_hash: Hash::zero(),
depth: 2,
value: Word::zero(),
call_data_offset: 0,
call_data_length: 0,
return_data_offset: 0,
return_data_length: 0,
});
builder.state_ref().push_call(
Call {
call_id: 0,
caller_id: 0,
kind: CallKind::StaticCall,
is_static: true,
is_root: false,
is_persistent: false,
is_success: false,
rw_counter_end_of_reversion: 0,
caller_address: *ADDR_A,
address: *ADDR_B,
code_source: CodeSource::Address(*ADDR_B),
code_hash: Hash::zero(),
depth: 2,
value: Word::zero(),
call_data_offset: 0,
call_data_length: 0,
return_data_offset: 0,
return_data_length: 0,
},
step,
);

assert_eq!(
builder.state_ref().get_step_err(step, next_step).unwrap(),
@@ -3499,7 +3529,9 @@ mod tracer_tests {
let mut builder = CircuitInputBuilderTx::new(&block, step_create2);
// Set up call context at CREATE2
builder.tx_ctx.call_is_success.push(false);
builder.state_ref().push_call(mock_internal_create());
builder
.state_ref()
.push_call(mock_internal_create(), step_create2);
let addr = builder.state_ref().create2_address(step_create2).unwrap();

assert_eq!(addr.to_word(), addr_expect);
@@ -3605,7 +3637,9 @@ mod tracer_tests {
let mut builder = CircuitInputBuilderTx::new(&block, step_create);
// Set up call context at CREATE
builder.tx_ctx.call_is_success.push(false);
builder.state_ref().push_call(mock_internal_create());
builder
.state_ref()
.push_call(mock_internal_create(), step_create);
builder.builder.sdb.set_account(
&ADDR_B,
Account {
37 changes: 35 additions & 2 deletions bus-mapping/src/evm/opcodes.rs
Original file line number Diff line number Diff line change
@@ -227,8 +227,10 @@ fn fn_gen_associated_ops(opcode_id: &OpcodeId) -> FnGenAssociatedOps {
// TODO: Handle REVERT by its own gen_associated_ops.
OpcodeId::REVERT => Stop::gen_associated_ops,
// OpcodeId::SELFDESTRUCT => {},
// _ => panic!("Opcode {:?} gen_associated_ops not implemented",
// self),
OpcodeId::CALLCODE | OpcodeId::DELEGATECALL | OpcodeId::STATICCALL => {
warn!("Using dummy gen_call_ops for opcode {:?}", opcode_id);
dummy_gen_call_ops
}
_ => {
warn!("Using dummy gen_associated_ops for opcode {:?}", opcode_id);
dummy_gen_associated_ops
@@ -500,3 +502,34 @@ pub fn gen_end_tx_ops(state: &mut CircuitInputStateRef) -> Result<ExecStep, Erro

Ok(exec_step)
}

fn dummy_gen_call_ops(
state: &mut CircuitInputStateRef,
geth_steps: &[GethExecStep],
) -> Result<Vec<ExecStep>, Error> {
let geth_step = &geth_steps[0];
let exec_step = state.new_step(geth_step)?;

let call = state.call()?.clone();
let callee = state.parse_call(geth_step)?;

let (_, account) = state.sdb.get_account(&callee.address);
let callee_code_hash = account.code_hash;

state.push_call(callee.clone(), geth_step);

match (
state.is_precompiled(&call.address),
callee_code_hash.to_fixed_bytes() == *EMPTY_HASH,
) {
// 1. Call to precompiled.
(true, _) => Ok(vec![exec_step]),
// 2. Call to account with empty code.
(_, true) => {
state.handle_return()?;
Ok(vec![exec_step])
}
// 3. Call to account with non-empty code.
(_, false) => Ok(vec![exec_step]),
}
}
2 changes: 1 addition & 1 deletion bus-mapping/src/evm/opcodes/call.rs
Original file line number Diff line number Diff line change
@@ -93,7 +93,7 @@ impl Opcode for Call {
)?;

// Switch to callee's call context
state.push_call(callee.clone());
state.push_call(callee.clone(), geth_step);

for (field, value) in [
(CallContextField::RwCounterEndOfReversion, 0.into()),
16 changes: 10 additions & 6 deletions bus-mapping/src/evm/opcodes/calldatacopy.rs
Original file line number Diff line number Diff line change
@@ -2,7 +2,7 @@ use super::Opcode;
use crate::circuit_input_builder::{
CircuitInputStateRef, CopyToMemoryAuxData, ExecState, ExecStep, StepAuxiliaryData,
};
use crate::operation::{CallContextField, CallContextOp, RW};
use crate::operation::{CallContextField, CallContextOp, MemoryOp, RW};
use crate::Error;
use eth_types::GethExecStep;

@@ -98,12 +98,16 @@ fn gen_memory_copy_step(
for idx in 0..std::cmp::min(bytes_left, MAX_COPY_BYTES) {
let addr = src_addr + idx as u64;
let byte = if addr < src_addr_end {
if is_root {
state.tx.input[addr as usize]
} else {
// TODO: read caller memory
unimplemented!()
let byte =
state.call_ctx()?.call_data[(addr - state.call()?.call_data_offset) as usize];
if !is_root {
state.push_op(
exec_step,
RW::READ,
MemoryOp::new(state.call()?.caller_id, (addr as usize).into(), byte),
);
}
byte
} else {
0
};
Loading

0 comments on commit 602e33d

Please sign in to comment.