feat: calculate commitments within the block

[yoavGrs]

This change is 

[dorimedini-starkware]

Reviewed 5 of 9 files at r1, 1 of 4 files at r2, 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @ShahakShama and @yoavGrs)

---

src/block_hash/block_hash_calculator.rs line 50 at r3 (raw file):

        .collect();
    let transactions_commitment =
        calculate_transactions_commitment::<PoseidonHashCalculator>(&transaction_leaf_elements);

add impl From<TransactionHashingData> for TransactionLeafElement and then do this

Suggestion:

    let transactions_commitment = calculate_transactions_commitment::<PoseidonHashCalculator>(
        &transactions_data.iter().map(TransactionLeafElement::from).collect(),
    );

---

src/block_hash/block_hash_calculator.rs line 59 at r3 (raw file):

            event_leaf_elements.push(event_leaf_element);
        }
    }

equivalent, right? (using flat_map).
or, for consistency with the froms (see other comments), consider implementing

fn to_event_leaves(&self) -> impl Iterator<Item = EventLeafElement>

on TransactionHashingData, and doing this:

let events_commitment = calculate_events_commitment::<PoseidonHashCalculator>(
    &transactions_data.iter().flat_map(|tx_data| tx_data.to_event_leaves()).collect()
);

Suggestion:

    let event_leaf_elements: Vec<EventLeafElement> = transactions_data
        .iter()
        .flat_map(|transaction_data| {
            let transaction_hash = transaction.transaction_hash;
            transaction.transaction_output.events().iter().map(|event| EventLeafElement {
                event: event.clone(), transaction_hash
            })
        })
        .collect()

---

src/block_hash/block_hash_calculator.rs line 75 at r3 (raw file):

        l1_data_gas_price_per_token,
        l1_gas_price_per_token,
    );

add impl From<TransactionHashingData> for ReceiptElement and then do this

Suggestion:

    let receipts_commitment = calculate_receipt_commitment::<PoseidonHashCalculator>(
        &transactions_data.iter().map(ReceiptElement::from).collect(),
        l1_data_gas_price_per_token,
        l1_gas_price_per_token,
    );

---

src/block_hash/event_commitment.rs line 16 at r3 (raw file):

    pub event: Event,
    pub transaction_hash: TransactionHash,
}

unless you need this to be public API

Suggestion:

pub struct EventLeafElement {
    pub(crate) event: Event,
    pub(crate) transaction_hash: TransactionHash,
}

---

src/block_hash/event_commitment_test.rs line 0 at r3 (raw file):
why is this change necessary or how is it related to this PR?

---

src/block_hash/receipt_commitment.rs line 21 at r3 (raw file):

    pub transaction_output: TransactionOutput,
    pub transaction_version: TransactionVersion,
}

this is a subset of TransactionHashingData, right?
if they are coupled types, please combine the 3 respective fields of TransactionHashingData into a single receipt_element field.
are they coupled? (non-blocking)

Code quote:

// The elements used to calculate a leaf in the transactions Patricia tree.
#[derive(Clone)]
pub struct ReceiptElement {
    pub transaction_hash: TransactionHash,
    pub transaction_output: TransactionOutput,
    pub transaction_version: TransactionVersion,
}

---

src/block_hash/receipt_commitment_test.rs line 38 at r3 (raw file):

            .unwrap(),
    );
    transaction_receipt.transaction_version = TransactionVersion::THREE;

add newline + comment to make it visible that this is a separate test case

Suggestion:

    );
    
    // Now test for a different transaction version.
    transaction_receipt.transaction_version = TransactionVersion::THREE;

---

src/block_hash/transaction_commitment.rs line 15 at r3 (raw file):

pub struct TransactionLeafElement {
    pub transaction_hash: TransactionHash,
    pub transaction_signature: Option<TransactionSignature>,

must it be public API?

Suggestion:

    pub(crate) transaction_hash: TransactionHash,
    pub(crate) transaction_signature: Option<TransactionSignature>,

---

src/block_hash/transaction_commitment.rs line 20 at r3 (raw file):

/// Returns the root of a Patricia tree where each leaf is
/// Poseidon(transaction_hash, transaction_signature).
/// The leaf of a transaction types without a signature field is: Poseidon(transaction_hash, 0).

hash function is generic in this function... please fix docstring

Suggestion:

/// H(transaction_hash, transaction_signature).
/// The leaf of a transaction types without a signature field is: H(transaction_hash, 0).

---

src/block_hash/transaction_commitment.rs line 36 at r3 (raw file):

                .transaction_signature
                .as_ref()
                .unwrap_or(&TransactionSignature(vec![StarkFelt::ZERO]))

what is this?

1. why is the signature suddenly optional?
2. why is a zero signature the desired fallback behavior and not panic?

Code quote:

.unwrap_or(&TransactionSignature(vec![StarkFelt::ZERO]))

[dorimedini-starkware]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @ShahakShama and @yoavGrs)

---

src/block_hash/block_hash_calculator.rs line 31 at r4 (raw file):

    pub receipts_commitment: ReceiptCommitment,
    pub state_diff_commitment: StateDiffCommitment,
    pub concated_counts: StarkFelt,

rename

Suggestion:

pub concatenated_counts: StarkFelt

[yoavGrs]

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @dorimedini-starkware and @ShahakShama)

---

src/block_hash/block_hash_calculator.rs line 50 at r3 (raw file):

Previously, dorimedini-starkware wrote…

add impl From<TransactionHashingData> for TransactionLeafElement and then do this

Done.

---

src/block_hash/block_hash_calculator.rs line 59 at r3 (raw file):

Previously, dorimedini-starkware wrote…

equivalent, right? (using flat_map).
or, for consistency with the froms (see other comments), consider implementing

fn to_event_leaves(&self) -> impl Iterator<Item = EventLeafElement>

on TransactionHashingData, and doing this:

let events_commitment = calculate_events_commitment::<PoseidonHashCalculator>(
    &transactions_data.iter().flat_map(|tx_data| tx_data.to_event_leaves()).collect()
);

I take the first suggestion.

---

src/block_hash/block_hash_calculator.rs line 75 at r3 (raw file):

Previously, dorimedini-starkware wrote…

add impl From<TransactionHashingData> for ReceiptElement and then do this

Done.

---

src/block_hash/event_commitment.rs line 16 at r3 (raw file):

Previously, dorimedini-starkware wrote…

unless you need this to be public API

Done.

---

src/block_hash/event_commitment_test.rs line at r3 (raw file):

Previously, dorimedini-starkware wrote…

why is this change necessary or how is it related to this PR?

Reverted.

---

src/block_hash/receipt_commitment.rs line 21 at r3 (raw file):

Previously, dorimedini-starkware wrote…

this is a subset of TransactionHashingData, right?
if they are coupled types, please combine the 3 respective fields of TransactionHashingData into a single receipt_element field.
are they coupled? (non-blocking)

TransactionLeafElement is a subset of TransactionHashingData as well.
Each commitment calculation takes some fields from TransactionHashingData.
It is better not to touch these fields.

---

src/block_hash/receipt_commitment_test.rs line 38 at r3 (raw file):

Previously, dorimedini-starkware wrote…

add newline + comment to make it visible that this is a separate test case

Done.

---

src/block_hash/transaction_commitment.rs line 20 at r3 (raw file):

Previously, dorimedini-starkware wrote…

hash function is generic in this function... please fix docstring

Right!!

---

src/block_hash/transaction_commitment.rs line 36 at r3 (raw file):

Previously, dorimedini-starkware wrote…

what is this?

1. why is the signature suddenly optional?
2. why is a zero signature the desired fallback behavior and not panic?

Deploy and L1Handler txs do not have a signature.

[dorimedini-starkware]

Reviewed 6 of 6 files at r5, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ShahakShama and @yoavGrs)

---

src/block_hash/receipt_commitment.rs line 21 at r3 (raw file):

Previously, yoavGrs wrote…

TransactionLeafElement is a subset of TransactionHashingData as well.
Each commitment calculation takes some fields from TransactionHashingData.
It is better not to touch these fields.

ah, each leaf needs a different subset?
can you impl From<TransactionHashingData> for any such "subset types"? so it's documented in code that TransactionHashingData is basically a superset object

---

src/block_hash/receipt_commitment.rs line 26 at r5 (raw file):

impl From<&TransactionHashingData> for ReceiptElement {
    fn from(transaction_data: &TransactionHashingData) -> Self {
        ReceiptElement {

non-blocking

Suggestion:

Self

---

src/block_hash/transaction_commitment.rs line 21 at r5 (raw file):

impl From<&TransactionHashingData> for TransactionLeafElement {
    fn from(transaction_data: &TransactionHashingData) -> Self {
        TransactionLeafElement {

non-blocking

Suggestion:

Self

[yoavGrs]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dorimedini-starkware and @ShahakShama)

---

src/block_hash/block_hash_calculator.rs line 31 at r4 (raw file):

Previously, dorimedini-starkware wrote…

rename

Done.

[yoavGrs]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dorimedini-starkware and @ShahakShama)

---

src/block_hash/receipt_commitment.rs line 21 at r3 (raw file):

Previously, dorimedini-starkware wrote…

ah, each leaf needs a different subset?
can you impl From<TransactionHashingData> for any such "subset types"? so it's documented in code that TransactionHashingData is basically a superset object

This is already included in your previous comments :)

[dorimedini-starkware]

Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ShahakShama and @yoavGrs)

---

src/block_hash/block_hash_calculator.rs line 31 at r4 (raw file):

Previously, yoavGrs wrote…

Done.

where?

[yoavGrs]

Reviewable status: 8 of 9 files reviewed, 1 unresolved discussion (waiting on @dorimedini-starkware and @ShahakShama)

---

src/block_hash/block_hash_calculator.rs line 31 at r4 (raw file):

Previously, dorimedini-starkware wrote…

where?

Done.

[dorimedini-starkware]

Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ShahakShama)

---

src/block_hash/block_hash_calculator.rs line 31 at r4 (raw file):

Previously, yoavGrs wrote…

Done.

ah, here