-
Notifications
You must be signed in to change notification settings - Fork 853
Circuit for opcode COINBASE #259
Circuit for opcode COINBASE #259
Conversation
@han0110 Could you please have a look ? |
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.
Aside from @han0110's comment on the Lookup of previous blocks by hash and the panic question, the rest looks good to me!
Nice job!!! 😄
) { | ||
self.add_lookup(Lookup::Block { | ||
field_tag: tag, | ||
number: number.unwrap_or_else(|| 0.expr()), |
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.
If the block comes without a number provided shouldn't we panic directly? As it means that some deep internal logic must be wrong.
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.
if the block
construct not initialize explicitly with a number, the default value will set to zero, so will not panic directly, does it make sense?
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.
But 0 would probbaly not be the correct value no? So we will be passing a wrong block to the circuit condtructor IIUC.
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.
the block constants include block number will be pub input of circuit eventually as planned, so it will be easy to figure out if it is incorrect .
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.
Yes, what I meant is that if you check the code, if there's any unexpected error/behavior and we receive a None/Err
, putting a 0 will not be correct.
Therefore the question of paniking.
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.
Ah, that is it , at first I used number
parameter type Expression
directly without Option
wrapper, but considering maybe zero in cases , change to Option
as now. what is your ideas ?
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.
To me it looks like number
should not be an Option
or Result
and instead a pure value directly.
In that case, it's impossible to construct a non-valid number
and you don't need to unwrap_or_zero
it.
Maybe @han0110 has some insight on that. I'm just wondering that this cannot be the source of errors in the future. If it cannot, we can simply resolve the conversation.
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.
IMO it's just semantic difference, since the block_table
now is constructed as it is, the field number
will always be 0
for all fields except history_hashes
. So it would not be source of errors, but just might not look so straightforward.
Another approach is to split this into 2 function w/o number
as input, to make it less ambiguous.
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.
Then you can proceed as you wish @DreamWuGit !!
7429811
to
2c5bffd
Compare
state_ref.push_stack_op( | ||
RW::WRITE, | ||
StackAddress::from(1024 - 1), | ||
block.ctants.coinbase.to_word(), |
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 want to refactor to use BlockConstants
's coinbase field here and remove ChainConstants
coinbase field, not sure why exists in chain constant of bussmapping before, want to hear from you guys first @ChihChengLiang @han0110 @CPerezz
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.
Maybe @ed255 can provide some insight on the bus-mapping related stuff.
From my perspective, the coinbase will always be the same. And therefore is only passed once via ChainConstants
instead of passing it on each tx/block.
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.
coinbase is only block's field not related to tx, each block can have different coinbase
address, chainid is global which make sense exists in ChainConstants
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 originally put the coinbase
in ChainConstants
. I had the wrong idea of what the coinbase
was, so it was a mistake. coinbase
should be removed from ChainConstants
.
The coinbase
that you need here I think should be block.eth_block.author
. See documentation here https://docs.rs/ethers/latest/ethers/core/types/struct.Block.html#structfield.author
Block miner
, author
, coinbase
are all the same thing.
The struct BlockConstants
defined in external_tracer
has a name which is a bit misleading. That struct is really a configuration parameter to setup an environment for the external tracer.
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.
Ohh my bad...
I understood coinbase on a different way...
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.
thanks @ed255 , I will try remove coinbase
in ChainConstants
at this PR!
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.
fixed in 21ad52f
// converting to block context | ||
let context = BlockContext { | ||
coinbase: b.constants.coinbase, | ||
..Default::default() |
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.
Here there should be a TODO: bus_mapping::circuit_input_builder::Block
should contain all the fields required in the BlockContext
:
pub coinbase: Address, // u160
pub gas_limit: u64,
pub block_number: F,
pub time: u64,
pub difficulty: Word,
pub base_fee: Word,
pub previous_block_hashes: Vec<Word>,
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 can take care of that if you want! Just let me know and I'll open the issue and address it myself.
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.
changed to use BlockConstant which holds all fields now.
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!!
@DreamWuGit it seems the branch has conflicts with main. |
|
Co-authored-by: Haichen Shen <shenhaichen@gmail.com>
Co-authored-by: Chih Cheng Liang <chihchengliang@gmail.com>
8522e39
to
723912d
Compare
rebased in 723912d |
Tests are failing @DreamWuGit Maybe was introduced some inconistency in tbhe rebase? |
I am looking into this :) |
Need 3 fixes of the doc in crate + //! use bus_mapping::external_tracer::BlockConstants; - //! coinbase: Address::zero(), - //! let mut builder =
- //! CircuitInputBuilder::new(sdb, CodeDB::new(), ð_block, ctants);
+ //! let mut builder = CircuitInputBuilder::new(
+ //! sdb,
+ //! CodeDB::new(),
+ //! ð_block,
+ //! ctants.clone(),
+ //! BlockConstants::from_eth_block(
+ //! ð_block,
+ //! &Word::from(ctants.chain_id),
+ //! ),
+ //! ); |
Is an example of usage. We have it there so that is easier to see which are the purposes and the recommended way to use the bus-mapping. Since it's an example we can also compiled on each test run and notice regressions in API or any other similar error introduced. I'm not against removing it. But IMO it is useful. |
understood, THX! |
The PR includes the following features:
The spec can be found at privacy-scaling-explorations/zkevm-specs#83