Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Commit

Permalink
add more tests related intermediate chunk and fixed bugs
Browse files Browse the repository at this point in the history
  • Loading branch information
@hero78119
hero78119 committed Feb 29, 2024
1 parent 8aaaa0d commit 4870080
Show file tree
Hide file tree
Showing 13 changed files with 368 additions and 272 deletions.
113 changes: 80 additions & 33 deletions bus-mapping/src/circuit_input_builder.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
// Optain the first op of the next GethExecStep, for fixed case also lookahead
let (mut cib, mut tx, mut tx_ctx) = (self.clone(), tx, tx_ctx);
let mut cib_ref = cib.state_ref(&mut tx, &mut tx_ctx);
let next_ops = if let Some((i, step)) = next_geth_step {
let mut next_ops = if let Some((i, step)) = next_geth_step {
log::trace!("chunk at {}th opcode {:?} ", i, step.op);
gen_associated_ops(&step.op, &mut cib_ref, &geth_trace.struct_logs[i..])?.remove(0)
} else {
Expand All @@ -368,10 +368,14 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
let last_copy = self.block.copy_events.len();
// Generate EndChunk and proceed to the next if it's not the last chunk
// Set next step pre-state as end_chunk state
self.set_end_chunk(&next_ops);
self.set_end_chunk(&next_ops, Some(&tx));

// need to update next_ops.rwc to catch block_ctx.rwc in `set_end_chunk`
next_ops.rwc = self.block_ctx.rwc;

// tx.id start from 1, so it's equivalent to `next_tx_index`
self.commit_chunk_ctx(true, tx.id as usize, last_copy, last_call);
self.set_begin_chunk(&next_ops);
self.set_begin_chunk(&next_ops, Some(&tx));

Ok(())
}
Expand All @@ -394,13 +398,12 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
let mut tx_ctx = TransactionContext::new(eth_tx, geth_trace, is_last_tx)?;

let res = if !geth_trace.invalid {
let mut last_call = None;

// Generate BeginTx step
let begin_tx_step = gen_associated_steps(
&mut self.state_ref(&mut tx, &mut tx_ctx),
ExecState::BeginTx,
)?;
let mut last_call = Some(tx.calls().get(begin_tx_step.call_index).unwrap().clone());
tx.steps_mut().push(begin_tx_step);

let mut trace = geth_trace.struct_logs.iter().enumerate().peekable();
Expand All @@ -416,9 +419,10 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
// Proceed to the next step
let (i, step) = trace.next().expect("Peeked step should exist");
log::trace!(
"handle {}th opcode {:?} rws = {:?}",
"handle {}th opcode {:?} {:?} rws = {:?}",
i,
step.op,
step,
self.chunk_rws()
);
let exec_steps = gen_associated_ops(
Expand Down Expand Up @@ -466,9 +470,13 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
Ok(res)
}

// TODO Fix this, for current logic on processing `call` is incorrect
// TODO re-design `gen_chunk_associated_steps` to separate RW
fn gen_chunk_associated_steps(&mut self, step: &mut ExecStep, rw: RW) {
// generate chunk related steps
fn gen_chunk_associated_steps(
&mut self,
step: &mut ExecStep,
rw: RW,
tx: Option<&Transaction>,
) {
let STEP_STATE_LEN = 10;
let mut dummy_tx = Transaction::default();
let mut dummy_tx_ctx = TransactionContext::default();
Expand All @@ -483,12 +491,10 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {

let tags = {
let state = self.state_ref(&mut dummy_tx, &mut dummy_tx_ctx);
let last_call = state
.block
.txs
.last()
.map(|tx| tx.calls[0].clone())
.unwrap_or_else(Call::default);
let last_call = tx
.map(|tx| tx.calls()[step.call_index].clone())
.or_else(|| state.block.txs.last().map(|tx| tx.calls[0].clone()))
.unwrap();
[
(StepStateField::CodeHash, last_call.code_hash.to_word()),
(StepStateField::CallID, Word::from(last_call.call_id)),
Expand Down Expand Up @@ -551,24 +557,44 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
self.chunk_ctx.end_copy_index = next_copy_index;
self.cur_chunk_mut().ctx = self.chunk_ctx.clone();
if to_next {
// here use `-1` to include previous set
self.chunk_ctx
.bump(self.block_ctx.rwc.0, next_tx_index, next_copy_index);
.bump(self.block_ctx.rwc.0, next_tx_index - 1, next_copy_index);
// println!("bump last_call {:?}", last_call);
if last_call.is_none() {
panic!("??")
}
self.cur_chunk_mut().prev_last_call = last_call;
}
}

fn set_begin_chunk(&mut self, first_step: &ExecStep) {
let mut begin_chunk = first_step.clone();
begin_chunk.exec_state = ExecState::BeginChunk;
self.gen_chunk_associated_steps(&mut begin_chunk, RW::READ);
fn set_begin_chunk(&mut self, first_step: &ExecStep, tx: Option<&Transaction>) {
let mut begin_chunk = ExecStep {
exec_state: ExecState::BeginChunk,
rwc: first_step.rwc,
gas_left: first_step.gas_left,
call_index: first_step.call_index,
..ExecStep::default()
};
self.gen_chunk_associated_steps(&mut begin_chunk, RW::READ, tx);
println!("in set begin chunk {:?}", begin_chunk);
self.chunks[self.chunk_ctx.idx].begin_chunk = Some(begin_chunk);
}

fn set_end_chunk(&mut self, next_step: &ExecStep) {
let mut end_chunk = next_step.clone();
end_chunk.exec_state = ExecState::EndChunk;
self.gen_chunk_associated_steps(&mut end_chunk, RW::WRITE);
fn set_end_chunk(&mut self, next_step: &ExecStep, tx: Option<&Transaction>) {
println!("before self.block_ctx.rwc.0 {}", self.block_ctx.rwc.0);
// println!("next step {:?}", next_step);
let mut end_chunk = ExecStep {
exec_state: ExecState::EndChunk,
rwc: next_step.rwc,
rwc_inner_chunk: next_step.rwc_inner_chunk,
gas_left: next_step.gas_left,
call_index: next_step.call_index,
..ExecStep::default()
};
self.gen_chunk_associated_steps(&mut end_chunk, RW::WRITE, tx);
self.gen_chunk_padding(&mut end_chunk);
println!("after self.block_ctx.rwc.0 {}", self.block_ctx.rwc.0);
self.chunks[self.chunk_ctx.idx].end_chunk = Some(end_chunk);
}

Expand All @@ -578,8 +604,6 @@ impl<'a, C: CircuitsParams> CircuitInputBuilder<C> {
let total_rws = end_rwc - 1;
let max_rws = self.cur_chunk().fixed_param.max_rws;

// We need at least 1 extra row at offset 0 for chunk continuous
// FIXME(Cecilia): adding + 1 fail some tests
assert!(
total_rws < max_rws,
"total_rws <= max_rws, total_rws={}, max_rws={}",
Expand Down Expand Up @@ -707,17 +731,26 @@ impl CircuitInputBuilder<FixedCParams> {
eth_block: &EthBlock,
geth_traces: &[eth_types::GethExecTrace],
) -> Result<CircuitInputBuilder<FixedCParams>, Error> {
println!("--------------{:?}", self.circuits_params);
// accumulates gas across all txs in the block
let (last_step, last_call) = self.begin_handle_block(eth_block, geth_traces)?;
let last_step = last_step.unwrap_or_default();
// since there is no next step, we cook dummy next step from last step to reuse
// existing field while update its `rwc`.
let mut dummy_next_step = {
let mut dummy_next_step = last_step.unwrap_or_default();
// raise last step rwc to match with next step
(0..dummy_next_step.rw_indices_len()).for_each(|_| {
dummy_next_step.rwc.inc_pre();
dummy_next_step.rwc_inner_chunk.inc_pre();
});
dummy_next_step
};

assert!(self.circuits_params.max_rws().is_some());

let last_copy = self.block.copy_events.len();

// TODO figure out and resolve generic param type and move fixed_param set inside
// commit_chunk_ctx After fixed, then we can set fixed_param on all chunks
// commit_chunk_ctx. After fixed, then we can set fixed_param on all chunks
(0..self.circuits_params.total_chunks()).for_each(|idx| {
self.get_chunk_mut(idx).fixed_param = self.circuits_params;
});
Expand All @@ -734,16 +767,29 @@ impl CircuitInputBuilder<FixedCParams> {
last_call.clone(),
);
} else {
// it doent matter what step was put to set_end_chunk/set_begin_chunk on no-used
// chunks before end_block. Just need to make sure it's step lookup is consistency.
self.set_end_chunk(&last_step);
self.set_end_chunk(&dummy_next_step, None);

self.commit_chunk_ctx(
true,
eth_block.transactions.len(),
last_copy,
last_call.clone(),
);
self.set_begin_chunk(&last_step);
// update dummy_next_step rwc to be used for next
dummy_next_step.rwc = self.block_ctx.rwc;
dummy_next_step.rwc_inner_chunk = self.chunk_ctx.rwc;
self.set_begin_chunk(&dummy_next_step, None);
dummy_next_step.rwc = self.block_ctx.rwc;
dummy_next_step.rwc_inner_chunk = self.chunk_ctx.rwc;
// update virtual step: end_block/padding so it can carry state context correctly
// TODO: enhance virtual step updating mechanism by having `running_next_step`
// defined in circuit_input_builder, so we dont need to
self.block.end_block = dummy_next_step.clone();
self.cur_chunk_mut().padding = {
let mut padding = dummy_next_step.clone();
padding.exec_state = ExecState::Padding;
Some(padding)
};
}
Ok::<(), Error>(())
})?;
Expand Down Expand Up @@ -773,6 +819,7 @@ impl CircuitInputBuilder<FixedCParams> {
fn set_end_block(&mut self) -> Result<(), Error> {
let mut end_block = self.block.end_block.clone();
end_block.rwc = self.block_ctx.rwc;
end_block.exec_state = ExecState::EndBlock;
end_block.rwc_inner_chunk = self.chunk_ctx.rwc;

let mut dummy_tx = Transaction::default();
Expand Down
9 changes: 3 additions & 6 deletions testool/src/statetest/executor.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -348,12 +348,9 @@ pub fn run_test(

let block: Block<Fr> =
zkevm_circuits::evm_circuit::witness::block_convert(&builder).unwrap();
let chunk: Chunk<Fr> =
zkevm_circuits::evm_circuit::witness::chunk_convert(&block, &builder)
.unwrap()
.remove(0);

CircuitTestBuilder::<1, 1>::new_from_block(block, chunk)
let chunks: Vec<Chunk<Fr>> =
zkevm_circuits::evm_circuit::witness::chunk_convert(&block, &builder).unwrap();
CircuitTestBuilder::<1, 1>::new_from_block(block, chunks)
.run_with_result()
.map_err(|err| match err {
CircuitTestError::VerificationFailed { reasons, .. } => {
Expand Down
4 changes: 3 additions & 1 deletion zkevm-circuits/src/evm_circuit.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -638,7 +638,9 @@ mod evm_circuit_stats {
TestContext::<0, 0>::new(None, |_| {}, |_, _| {}, |b, _| b).unwrap(),
)
.block_modifier(Box::new(|_block, chunk| {
chunk.fixed_param.max_evm_rows = (1 << 18) - 100
chunk
.iter_mut()
.for_each(|chunk| chunk.fixed_param.max_evm_rows = (1 << 18) - 100);
}))
.run();
}
Expand Down
56 changes: 44 additions & 12 deletions zkevm-circuits/src/evm_circuit/execution.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -815,6 +815,10 @@ impl<F: Field> ExecutionConfig<F> {

let step_curr_rw_counter = cb.curr.state.rw_counter.clone();
let step_curr_rw_counter_offset = cb.rw_counter_offset();
if execution_state == ExecutionState::BeginChunk {
cb.debug_expression("step_curr_rw_counter.expr()", step_curr_rw_counter.expr());
}

let debug_expressions = cb.debug_expressions.clone();

// Extract feature config here before cb is built.
Expand Down Expand Up @@ -979,9 +983,10 @@ impl<F: Field> ExecutionConfig<F> {
.collect(),
),
(
"Only EndTx or InvalidTx or EndBlock or Padding can transit to EndBlock",
"Only BeginChunk or EndTx or InvalidTx or EndBlock or Padding can transit to EndBlock",
ExecutionState::EndBlock,
vec![
ExecutionState::BeginChunk,
ExecutionState::EndTx,
ExecutionState::EndBlock,
ExecutionState::Padding,
Expand Down Expand Up @@ -1125,24 +1130,26 @@ impl<F: Field> ExecutionConfig<F> {
self.q_step_first.enable(&mut region, offset)?;

let dummy_tx = Transaction::default();
let chunk_txs = block
// chunk_txs is just a super set of execstep including both belong to this chunk and
// outside of this chunk
let chunk_txs: &[Transaction] = block
.txs
.get(chunk.chunk_context.initial_tx_index..chunk.chunk_context.end_tx_index)
.unwrap_or_default();

let last_call = chunk_txs
// If it's the very first chunk in a block set last call & begin_chunk to default
let prev_chunk_last_call = chunk.prev_last_call.clone().unwrap_or_default();
let cur_chunk_last_call = chunk_txs
.last()
.map(|tx| tx.calls()[0].clone())
.unwrap_or_else(Call::default);
// If it's the very first chunk in a block set last call & begin_chunk to default
let prev_last_call = chunk.prev_last_call.clone().unwrap_or_default();
.unwrap_or_else(|| prev_chunk_last_call.clone());

let padding = chunk.padding.as_ref().expect("padding can't be None");

// conditionally adding first step as begin chunk
let maybe_begin_chunk = {
if let Some(begin_chunk) = &chunk.begin_chunk {
vec![(&dummy_tx, &prev_last_call, begin_chunk)]
vec![(&dummy_tx, &prev_chunk_last_call, begin_chunk)]
} else {
vec![]
}
Expand All @@ -1153,10 +1160,16 @@ impl<F: Field> ExecutionConfig<F> {
.chain(chunk_txs.iter().flat_map(|tx| {
tx.steps()
.iter()
// chunk_txs is just a super set of execstep. To filter targetting
// execstep we need to further filter by [initial_rwc, end_rwc)
.filter(|step| {
step.rwc.0 >= chunk.chunk_context.initial_rwc
&& step.rwc.0 < chunk.chunk_context.end_rwc
})
.map(move |step| (tx, &tx.calls()[step.call_index], step))
}))
// this dummy step is just for real step assignment proceed to `second last`
.chain(std::iter::once((&dummy_tx, &last_call, padding)))
.chain(std::iter::once((&dummy_tx, &cur_chunk_last_call, padding)))
.peekable();

let evm_rows = chunk.fixed_param.max_evm_rows;
Expand Down Expand Up @@ -1228,6 +1241,7 @@ impl<F: Field> ExecutionConfig<F> {
if next.is_none() {
break;
}

second_last_real_step = Some(cur);
// record offset of current step before assignment
second_last_real_step_offset = offset;
Expand All @@ -1243,7 +1257,7 @@ impl<F: Field> ExecutionConfig<F> {
next_step_after_real_step = Some(padding.clone());
}
offset = assign_padding_or_step(
(&dummy_tx, &last_call, padding),
(&dummy_tx, &cur_chunk_last_call, padding),
offset,
None,
Some(evm_rows - 1),
Expand All @@ -1254,7 +1268,7 @@ impl<F: Field> ExecutionConfig<F> {
if let Some(end_chunk) = &chunk.end_chunk {
debug_assert_eq!(ExecutionState::EndChunk.get_step_height(), 1);
offset = assign_padding_or_step(
(&dummy_tx, &last_call, end_chunk),
(&dummy_tx, &cur_chunk_last_call, end_chunk),
offset,
None,
None,
Expand All @@ -1269,7 +1283,7 @@ impl<F: Field> ExecutionConfig<F> {
);
debug_assert_eq!(ExecutionState::EndBlock.get_step_height(), 1);
offset = assign_padding_or_step(
(&dummy_tx, &last_call, &block.end_block),
(&dummy_tx, &cur_chunk_last_call, &block.end_block),
offset,
None,
None,
Expand All @@ -1286,7 +1300,11 @@ impl<F: Field> ExecutionConfig<F> {
_ = assign_padding_or_step(
last_real_step,
second_last_real_step_offset,
Some((&dummy_tx, &last_call, &next_step_after_real_step.unwrap())),
Some((
&dummy_tx,
&cur_chunk_last_call,
&next_step_after_real_step.unwrap(),
)),
None,
)?;
}
Expand Down Expand Up @@ -1421,6 +1439,13 @@ impl<F: Field> ExecutionConfig<F> {
step,
call
);
// println!(
// "assign_exec_step offset: {} state {:?} step: {:?} call: {:?}",
// offset,
// step.execution_state(),
// step,
// call
// );
}
// Make the region large enough for the current step and the next step.
// The next step's next step may also be accessed, so make the region large
Expand All @@ -1438,6 +1463,13 @@ impl<F: Field> ExecutionConfig<F> {
// so their witness values need to be known to be able
// to correctly calculate the intermediate value.
if let Some(next_step) = next_step {
// println!(
// "assign_exec_step nextstep offset: {} state {:?} step: {:?} call: {:?}",
// offset,
// next_step.2.execution_state(),
// next_step.2,
// next_step.1
// );
self.assign_exec_step_int(
region,
offset + height,
Expand Down
Loading

0 comments on commit 4870080

Please sign in to comment.