-
Notifications
You must be signed in to change notification settings - Fork 856
Bytecode, and a collection of bytecodes. #1529
Conversation
eth-types/src/bytecode.rs
Outdated
.code() | ||
.iter() | ||
.cloned() | ||
.chain(0u8..((32 - len % 32) as u8)) |
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.
I keep the original code from tracer tests. But I keep wondering if we should pad only zeros but not 0...something
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.
oh right, This should a list of zeroes, not a sequence!
I guess most of the times it works because the first zero would be a STOP, so the rest are ignored. And also the although in memory it's multiples of 32, when using this bytecode in memory a length is specified, so the padding will be unused. But I still think it's confusing not having just zeroes!
I think it's worth changing this.
testool/src/statetest/executor.rs
Outdated
std::borrow::Cow::Owned(Vec::new()) | ||
vec![] | ||
} else { | ||
std::borrow::Cow::Borrowed(&builder.code_db.0[&actual.code_hash]) |
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.
I don't get why we currently need std::borrow::Cow::Owned
and std::borrow::Cow::Borrowed
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.
Based on the usage of Borrowed/Owned It's can allow auto
lazily clone only on write happened. It's for memory usage optimisation. Maybe it's still valuable to keep it in this place since bytecode might be pretty longer and introduce memory overhead
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.
I discussed this with @ed255. We concluded that the Cow was there to prevent cloning twice.
- Cow is used to keep
actual_code
a reference to the byte vector so that we don't copy bytecodes from codeDB here. - The line
found: Bytes::from(actual_code.to_vec()),
below clones bytes with theto_vec
method.
I resolved with another method that only clones once.
challenges: &Challenges<Value<F>>, | ||
fail_fast: bool, |
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.
I removed fail_fast because it was never triggered.
@@ -484,13 +567,12 @@ impl<F: Field> BytecodeCircuitConfig<F> { | |||
&self, | |||
layouter: &mut impl Layouter<F>, | |||
size: usize, | |||
witness: &[UnrolledBytecode<F>], | |||
overwrite: &UnrolledBytecode<F>, |
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.
Overwrite is gone in the assignment code.
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.
Just have a first pass, all changes outside bytecode circuit LGTM! thanks for using new api to clean up codebase quite a lot!
Will have second review on bytecode circuit logic thoroughly sooner.
testool/src/statetest/executor.rs
Outdated
std::borrow::Cow::Owned(Vec::new()) | ||
vec![] | ||
} else { | ||
std::borrow::Cow::Borrowed(&builder.code_db.0[&actual.code_hash]) |
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.
Based on the usage of Borrowed/Owned It's can allow auto
lazily clone only on write happened. It's for memory usage optimisation. Maybe it's still valuable to keep it in this place since bytecode might be pretty longer and introduce memory overhead
d9f720a
to
84bc322
Compare
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.
LGTM! The rewrite of bytecode witness assignment and unify padding/normal row into single set_row api is awesome, plus the bytecode test api reorganized.
Just have few nitpick regarding to function naming, and return type suggestion.
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.
Overall LGTM! Very nice clean up 🎉
I left 3 comments, please take a look!
eth-types/src/bytecode.rs
Outdated
.code() | ||
.iter() | ||
.cloned() | ||
.chain(0u8..((32 - len % 32) as u8)) |
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.
oh right, This should a list of zeroes, not a sequence!
I guess most of the times it works because the first zero would be a STOP, so the rest are ignored. And also the although in memory it's multiples of 32, when using this bytecode in memory a length is specified, so the padding will be unused. But I still think it's confusing not having just zeroes!
I think it's worth changing this.
address: Address::repeat_byte(0xff), | ||
code: code.into(), | ||
nonce: U64::from(!is_empty as u64), | ||
balance: if is_empty { 0 } else { 0xdeadbeefu64 }.into(), |
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 this I think this function generates an account that is either empty, or has code and positive balance. So I would change the function documentation accordingly, and maybe rename the function to mock_code_balance
, or something that doesn't imply "code or balance".
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.
Nice catch! I renamed the function and updated the documentation 2228e4d
84bc322
to
2228e4d
Compare
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.
LGTM! Great clean up 👯♂️
Description
We revisit the handling of the bytecodes in the codebase. We reduce manual manipulation of bytes and use a more streamlined API with eth_types::Bytecode and bus_mapping::CodeDB.
Issue Link
Missed out work in #1391
Type of change
Refactor
Contents
Rationale