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

Feat/##1369 withdraw from beacon chain part 1 - pi circuit #1634

Merged
merged 16 commits into from
Oct 11, 2023
Merged

Feat/##1369 withdraw from beacon chain part 1 - pi circuit #1634

merged 16 commits into from
Oct 11, 2023

Conversation

KimiWu123
Copy link
Contributor

@KimiWu123 KimiWu123 commented Sep 28, 2023 •
edited
Loading

Description

1st part of #1369, mainly on pi circuit integration

Issue Link

#1369

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Contents

  • Withdrawal struct definition and WdTable implementation
  • adding withdrawals and withdrawal_root in block struct
  • wd_table assignment in pi circuit

@KimiWu123 KimiWu123 self-assigned this Sep 28, 2023
@github-actions github-actions bot added crate-bus-mapping Issues related to the bus-mapping workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member crate-integration-tests Issues related to the integration-tests workspace member crate-external-tracer Issues related to the external-tracer workspace member crate-mock Issues related to the mock workspace member crate-eth-types Issues related to the eth-types workspace member labels Sep 28, 2023
@KimiWu123 KimiWu123 marked this pull request as ready for review October 4, 2023 01:46
@KimiWu123 KimiWu123 force-pushed the feat/##1369-withdraw-from-beacon-chain-1 branch from a130477 to 5c8f0d8 Compare October 4, 2023 02:09
@KimiWu123 KimiWu123 mentioned this pull request Oct 4, 2023
Open
@github-actions github-actions bot added T-bench Type: benchmark improvements crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member labels Oct 4, 2023
@KimiWu123 KimiWu123 force-pushed the feat/##1369-withdraw-from-beacon-chain-1 branch from 57db679 to e2365d1 Compare October 4, 2023 02:47
@KimiWu123 KimiWu123 force-pushed the feat/##1369-withdraw-from-beacon-chain-1 branch from e2365d1 to 953da96 Compare October 4, 2023 02:53
@KimiWu123 KimiWu123 force-pushed the feat/##1369-withdraw-from-beacon-chain-1 branch from b389ad1 to 97193e8 Compare October 4, 2023 03:47
@KimiWu123 KimiWu123 changed the title [WIP] Feat/##1369 withdraw from beacon chain part 1 [WIP] Feat/##1369 withdraw from beacon chain part 1 -pi circuit Oct 4, 2023
@KimiWu123 KimiWu123 changed the title [WIP] Feat/##1369 withdraw from beacon chain part 1 -pi circuit Feat/##1369 withdraw from beacon chain part 1 - pi circuit Oct 4, 2023
@hero78119 hero78119 self-requested a review October 4, 2023 08:53
@miha-stopar miha-stopar self-requested a review October 4, 2023 09:07
miha-stopar
miha-stopar reviewed Oct 4, 2023
View reviewed changes
Copy link
Collaborator

@miha-stopar miha-stopar left a comment

Choose a reason for hiding this comment

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

Just some nitpicks, I will have a closer look tomorrow.

bus-mapping/src/circuit_input_builder/withdrawal.rs Outdated
}

impl WithdrawalContext {
/// Return id of the this withdrawal.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: redundant "the"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 6301184

bus-mapping/src/circuit_input_builder/withdrawal.rs Outdated

/// Reversible Write Counter tracks the number of write operations in the
/// call. It is incremented when a sub-call in this call succeeds by the
/// number of successful writes in the sub-call.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: perhaps "when there are successful writes in the sub-call of this call."?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 6301184

zkevm-circuits/src/pi_circuit.rs Outdated
validator_id: u64,
address: Address,
amount: u64,
rpi_bytes_keccakrlc: &mut Value<F>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: perhaps rpi_bytes_keccak_rlc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 6301184

zkevm-circuits/src/pi_circuit.rs Outdated
amount: u64,
rpi_bytes_keccakrlc: &mut Value<F>,
challenges: &Challenges<Value<F>>,
current_offset: &mut usize,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps document what's the difference between offset and current_offset.

Copy link
Contributor Author

@KimiWu123 KimiWu123 Oct 5, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

current_offset renamed to current_rpi_offset and fixed in 6301184

zkevm-circuits/src/pi_circuit.rs Outdated
id: u64,
validator_id: u64,
address: Address,
amount: u64,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps use a struct Withdrawal for id, validator_id, address, amount to reduce the number of arguments?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 6301184

zkevm-circuits/src/instance.rs Outdated
@@ -81,6 +82,8 @@ pub struct PublicData {
pub history_hashes: Vec<Word>,
/// Block Transactions
pub transactions: Vec<Transaction>,
/// Block Transactions
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: comment copied?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in 6301184

zkevm-circuits/src/pi_circuit/test.rs Outdated
public_data: PublicData,
) -> Result<(), Vec<VerifyFailure>> {
let mut public_data = public_data;
public_data.chain_id = *MOCK_CHAIN_ID;

let circuit = PiCircuit::<F>::new(max_txs, max_calldata, public_data);
let circuit = PiCircuit::<F>::new(max_txs,max_withdrawals, max_calldata, public_data);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpick: indentation

miha-stopar
miha-stopar reviewed Oct 5, 2023
View reviewed changes
bus-mapping/src/circuit_input_builder/withdrawal.rs Outdated
/// Result of the parsing of an Ethereum Withdrawal.
pub struct Withdrawal {
/// Unique identifier of a withdrawal. This value starts from 0 and then increases
/// monotonically.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need a constraint that it starts from 0 (currently there is only for the increase by 1)?

However, it's a bit confusing because the specs say: "Which means it won't be starting from 0 in most of cases."

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The id is unique and monotonical which means there is only one withdrawal with id == 0 in all withdrawals history. For example, in block n, withdrawal id starts from 5 to 12. In block n + 1, withdrawal id will start from 13. That's why I said: "Which means it won't be starting from 0 in most of cases.". Does it make sense to you?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ok, cool! Perhaps change to Unique identifier of a withdrawal in the whole history of withdrawals.?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed in cad4ea4

hero78119
hero78119 suggested changes Oct 6, 2023
View reviewed changes
Copy link
Member

@hero78119 hero78119 left a comment

Choose a reason for hiding this comment

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

Overall looks nice, just raised few question & suggestions

bus-mapping/src/circuit_input_builder/withdrawal.rs Show resolved Hide resolved
bus-mapping/src/circuit_input_builder/withdrawal.rs Outdated Show resolved Hide resolved
mock/src/test_ctx.rs Show resolved Hide resolved
zkevm-circuits/src/pi_circuit.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/pi_circuit.rs Outdated Show resolved Hide resolved
zkevm-circuits/src/pi_circuit.rs Show resolved Hide resolved
hero78119
hero78119 approved these changes Oct 10, 2023
View reviewed changes
Copy link
Member

@hero78119 hero78119 left a comment

Choose a reason for hiding this comment

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

Overall LGTM! 👍 👍

miha-stopar
miha-stopar approved these changes Oct 10, 2023
View reviewed changes
Copy link
Collaborator

@miha-stopar miha-stopar left a comment

Choose a reason for hiding this comment

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

LGTM!

@KimiWu123 KimiWu123 mentioned this pull request Oct 11, 2023
Open
@KimiWu123 KimiWu123 added this pull request to the merge queue Oct 11, 2023
Merged via the queue into main with commit 9d93c0a Oct 11, 2023
13 checks passed
@KimiWu123 KimiWu123 deleted the feat/##1369-withdraw-from-beacon-chain-1 branch October 11, 2023 05:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@hero78119 hero78119 hero78119 approved these changes

@miha-stopar miha-stopar miha-stopar approved these changes

Assignees

@KimiWu123 KimiWu123

Labels
crate-bus-mapping Issues related to the bus-mapping workspace member crate-circuit-benchmarks Issues related to the circuit-benchmarks workspace member crate-eth-types Issues related to the eth-types workspace member crate-external-tracer Issues related to the external-tracer workspace member crate-integration-tests Issues related to the integration-tests workspace member crate-mock Issues related to the mock workspace member crate-zkevm-circuits Issues related to the zkevm-circuits workspace member T-bench Type: benchmark improvements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@KimiWu123 @miha-stopar @hero78119