-
Notifications
You must be signed in to change notification settings - Fork 38
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
Add context pruning #112
Add context pruning #112
Changes from 7 commits
02ba230
d7fe960
13eeeb8
6c01ea3
fea4185
3c5f3df
f5a5769
91c02d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -178,7 +178,10 @@ fn test_extcodesize() -> Result<()> { | |
HashMap::from([(keccak(&code), code.clone())]); | ||
interpreter.run()?; | ||
|
||
assert_eq!(interpreter.stack(), vec![code.len().into()]); | ||
assert_eq!( | ||
interpreter.stack(), | ||
vec![U256::one() << CONTEXT_SCALING_FACTOR, code.len().into()] | ||
); | ||
Comment on lines
+181
to
+184
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. DQ: why do we expect this to be the content of the stack here? And why only for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's because |
||
|
||
Ok(()) | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,7 +23,7 @@ use crate::memory::segments::Segment; | |
use crate::util::u256_to_usize; | ||
use crate::witness::errors::ProgramError; | ||
use crate::witness::memory::MemoryChannel::GeneralPurpose; | ||
use crate::witness::memory::{MemoryAddress, MemoryOp, MemoryState}; | ||
use crate::witness::memory::{MemoryAddress, MemoryContextState, MemoryOp, MemoryState}; | ||
use crate::witness::memory::{MemoryOpKind, MemorySegmentState}; | ||
use crate::witness::operation::{generate_exception, Operation}; | ||
use crate::witness::state::RegistersState; | ||
|
@@ -212,7 +212,15 @@ pub(crate) trait State<F: Field> { | |
if !running { | ||
assert_eq!(self.get_clock() - final_clock, NUM_EXTRA_CYCLES_AFTER - 1); | ||
} | ||
let final_mem = self.get_full_memory(); | ||
let final_mem = if let Some(mut mem) = self.get_full_memory() { | ||
// Clear memory we will not use again. | ||
for &ctx in &self.get_generation_state().pruned_contexts { | ||
hratoanina marked this conversation as resolved.
Show resolved
Hide resolved
|
||
mem.contexts[ctx] = MemoryContextState::default(); | ||
} | ||
Some(mem) | ||
} else { | ||
None | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For my education, what is the case when There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When you're generating the proof (with the full trace), you don't need this memory. The |
||
}; | ||
#[cfg(not(test))] | ||
self.log_info(format!("CPU halted after {} cycles", self.get_clock())); | ||
return Ok((final_registers, final_mem)); | ||
|
@@ -326,6 +334,10 @@ pub struct GenerationState<F: Field> { | |
pub(crate) memory: MemoryState, | ||
pub(crate) traces: Traces<F>, | ||
|
||
/// Memory used by stale contexts can be pruned so proving segments can be | ||
/// smaller. | ||
pub(crate) pruned_contexts: Vec<usize>, | ||
hratoanina marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
/// Prover inputs containing RLP data, in reverse order so that the next | ||
/// input can be obtained via `pop()`. | ||
pub(crate) rlp_prover_inputs: Vec<U256>, | ||
|
@@ -375,6 +387,7 @@ impl<F: Field> GenerationState<F> { | |
registers: Default::default(), | ||
memory: MemoryState::new(kernel_code), | ||
traces: Traces::default(), | ||
pruned_contexts: Vec::new(), | ||
rlp_prover_inputs, | ||
withdrawal_prover_inputs, | ||
state_key_to_address: HashMap::new(), | ||
|
@@ -462,6 +475,7 @@ impl<F: Field> GenerationState<F> { | |
registers: self.registers, | ||
memory: self.memory.clone(), | ||
traces: Traces::default(), | ||
pruned_contexts: Vec::new(), | ||
rlp_prover_inputs: self.rlp_prover_inputs.clone(), | ||
state_key_to_address: self.state_key_to_address.clone(), | ||
bignum_modmul_result_limbs: self.bignum_modmul_result_limbs.clone(), | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to add these changes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
codecopy_within_bounds
callscodecopy_after
instead ofwcopy_after
. I agree that's more or less code duplication, but I didn't see an easy way to do it more cleanly.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My problem is that I can't see where are you changing the context here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding, the problem with codecopy etc. is that they do not change contexts, but write code in another context. And the idea here is to still be able to prune that extra context when needed by setting the context in codecopy_after.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh, and why
src_ctx
will be never accessed again?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the case of
extcodecopy
andextcodesize
, we generatesrc_ctx
withnext_context_id
which increments@GLOBAL_METADATA_LARGEST_CONTEXT
, meaning that any other created context will have a different id.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahhh ok, I think see now. It's because you just need the
new_ctx
for writing the code there (and check the hash etc) but after you copy the code you can forget about it?