Skip to content
This repository was archived by the owner on Apr 18, 2025. It is now read-only.

feat: add sload op#97

Closed
0xmountaintop wants to merge 8 commits into
mainfrom
re-init
Closed

feat: add sload op#97
0xmountaintop wants to merge 8 commits into
mainfrom
re-init

Conversation

@0xmountaintop
Copy link
Copy Markdown

@0xmountaintop 0xmountaintop commented Feb 13, 2022

reference:
privacy-ethereum/zkevm-specs#106

BTW, I would like to separate sload and sstore PR. because the gas calculation and the TxRefund calculation for sstore are more complicated.

@lispc
Copy link
Copy Markdown

lispc commented Feb 14, 2022

i guess after adding sstore, we can use bus-mapping to generate sload test case?

@0xmountaintop
Copy link
Copy Markdown
Author

i guess after adding sstore, we can use bus-mapping to generate sload test case?

Still no.. we don't have CallContextOp for now:

  1. https://github.com/appliedzkp/zkevm-circuits/blob/main/bus-mapping/src/operation.rs#L626-L627
  2. bus-mapping: Implement CallContextOp privacy-ethereum/zkevm-circuits#225

(I could deal with TxAccessListAccountStorageOp, but CallContextOp requires a lot of work.)

@0xmountaintop
Copy link
Copy Markdown
Author

and that's why I originally had #79 but then gave up and submit this PR.

F::from(*tx_id as u64),
address.to_scalar().unwrap(),
RandomLinearCombination::random_linear_combine(key.to_le_bytes(), randomness),
F::from(*value as u64),
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do need to random_linear_combine of value value_prev or already done when passing as parameter ?

Copy link
Copy Markdown
Author

@0xmountaintop 0xmountaintop Feb 15, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in TxAccessList, both value & value_prev are boolean instead of word.

as in TxAccessListAccount we don't do random_linear_combine, so I don't think we should do random_linear_combine in TxAccessListAccountStorage here

},
ExecStep {
execution_state: ExecutionState::STOP,
rw_counter: 17,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should check result for next step rw_counter ? just like above step rw_indices: (0..8 + if result { 0 } else { 2 }).collect(),

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we don't need to check for revert.

Like we don't check for revert in BEGIN_TX.

in StepStateTransition we use rw_counter: Delta(8.expr()), and here we only have 1 case, instead of also considering revert case

@0xmountaintop
Copy link
Copy Markdown
Author

moved to privacy-ethereum#334

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants