Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(consensus): remove the tx_hash from transactionBatch #2459

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

Yael-Starkware
Copy link
Contributor

No description provided.

@reviewable-StarkWare
Copy link

This change is Reviewable

@Yael-Starkware Yael-Starkware marked this pull request as ready for review December 4, 2024 14:44
@Yael-Starkware Yael-Starkware force-pushed the yael/convert_transaction_to_executable_transaction branch from c461854 to d90d875 Compare December 4, 2024 14:55
@Yael-Starkware Yael-Starkware force-pushed the yael/remove_tx_hash_from_node_communication branch from cab7be5 to 39614b8 Compare December 4, 2024 14:55
@Yael-Starkware Yael-Starkware force-pushed the yael/convert_transaction_to_executable_transaction branch from d90d875 to 3f229bf Compare December 4, 2024 15:38
@Yael-Starkware Yael-Starkware force-pushed the yael/remove_tx_hash_from_node_communication branch from 39614b8 to 237b239 Compare December 4, 2024 15:43
@Yael-Starkware Yael-Starkware force-pushed the yael/convert_transaction_to_executable_transaction branch from 3f229bf to a23e3cf Compare December 5, 2024 07:57
@Yael-Starkware Yael-Starkware force-pushed the yael/remove_tx_hash_from_node_communication branch from 237b239 to 443acfa Compare December 5, 2024 08:01
@Yael-Starkware Yael-Starkware force-pushed the yael/convert_transaction_to_executable_transaction branch from a23e3cf to 3a3d2bf Compare December 5, 2024 08:23
@Yael-Starkware Yael-Starkware force-pushed the yael/remove_tx_hash_from_node_communication branch from 443acfa to ee60997 Compare December 5, 2024 08:23
@Yael-Starkware Yael-Starkware force-pushed the yael/convert_transaction_to_executable_transaction branch from 3a3d2bf to 0a4b210 Compare December 5, 2024 08:36
@Yael-Starkware Yael-Starkware force-pushed the yael/remove_tx_hash_from_node_communication branch from ee60997 to 8f0301e Compare December 5, 2024 08:36
@Yael-Starkware Yael-Starkware force-pushed the yael/convert_transaction_to_executable_transaction branch from 0a4b210 to f377d1f Compare December 5, 2024 13:06
@Yael-Starkware Yael-Starkware force-pushed the yael/remove_tx_hash_from_node_communication branch from 8f0301e to e10c2a4 Compare December 5, 2024 13:28
@Yael-Starkware Yael-Starkware force-pushed the yael/convert_transaction_to_executable_transaction branch from f377d1f to c3a2d27 Compare December 5, 2024 13:34
@Yael-Starkware Yael-Starkware changed the title chore(sequencer_consensus_context): remove the tx_hash from transactionBatch chore(consensus): remove the tx_hash from transactionBatch Dec 5, 2024
@Yael-Starkware Yael-Starkware force-pushed the yael/convert_transaction_to_executable_transaction branch from c3a2d27 to 46f5219 Compare December 5, 2024 13:34
@Yael-Starkware Yael-Starkware force-pushed the yael/remove_tx_hash_from_node_communication branch from e10c2a4 to 99a88ed Compare December 5, 2024 13:34
Copy link

codecov bot commented Dec 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 32.92%. Comparing base (2eb8a6e) to head (7558332).

Additional details and impacted files
@@                                   Coverage Diff                                   @@
##           yael/convert_transaction_to_executable_transaction    #2459       +/-   ##
=======================================================================================
+ Coverage                                               18.44%   32.92%   +14.47%     
=======================================================================================
  Files                                                     225      119      -106     
  Lines                                                   30735    14144    -16591     
  Branches                                                30735    14144    -16591     
=======================================================================================
- Hits                                                     5670     4657     -1013     
+ Misses                                                  24632     8928    -15704     
- Partials                                                  433      559      +126     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Yael-Starkware Yael-Starkware force-pushed the yael/convert_transaction_to_executable_transaction branch from 46f5219 to 2eb8a6e Compare December 5, 2024 13:47
@Yael-Starkware Yael-Starkware force-pushed the yael/remove_tx_hash_from_node_communication branch from 99a88ed to 5f72901 Compare December 5, 2024 13:47
Copy link
Contributor

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 6 files at r1, 5 of 7 files at r3, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5, @matan-starkware, and @Yael-Starkware)


crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 468 at r4 (raw file):

                    transactions.push(tx.into());
                }
                debug!("Broadcasting proposal content: {transaction_hashes:?}");

I think this debug (and the extraction of the hashes right above it) was here before I added the hashes to the transaction batch. It may be better to leave it here.

@Yael-Starkware Yael-Starkware force-pushed the yael/remove_tx_hash_from_node_communication branch from 5f72901 to 7558332 Compare December 8, 2024 07:55
Copy link
Contributor Author

@Yael-Starkware Yael-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5, @guy-starkware, and @matan-starkware)


crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 468 at r4 (raw file):

Previously, guy-starkware wrote…

I think this debug (and the extraction of the hashes right above it) was here before I added the hashes to the transaction batch. It may be better to leave it here.

Done.

Copy link
Collaborator

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 6 files at r1, 5 of 7 files at r3, 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guy-starkware and @matan-starkware)

@Yael-Starkware Yael-Starkware force-pushed the yael/convert_transaction_to_executable_transaction branch 3 times, most recently from c2a12a3 to 6da7a51 Compare December 16, 2024 06:07
@Yael-Starkware Yael-Starkware force-pushed the yael/remove_tx_hash_from_node_communication branch from 7558332 to 28cf4d2 Compare December 16, 2024 06:21
Copy link
Collaborator

@alonh5 alonh5 left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r6, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guy-starkware and @matan-starkware)

@Yael-Starkware Yael-Starkware force-pushed the yael/convert_transaction_to_executable_transaction branch 3 times, most recently from fd4efc9 to 0b267dd Compare December 17, 2024 05:36
@Yael-Starkware Yael-Starkware changed the base branch from yael/convert_transaction_to_executable_transaction to graphite-base/2459 December 17, 2024 08:40
@Yael-Starkware Yael-Starkware force-pushed the yael/remove_tx_hash_from_node_communication branch from 28cf4d2 to 3e78cac Compare December 17, 2024 08:41
@Yael-Starkware Yael-Starkware changed the base branch from graphite-base/2459 to main December 17, 2024 08:41
@Yael-Starkware Yael-Starkware force-pushed the yael/remove_tx_hash_from_node_communication branch from 3e78cac to 2f37611 Compare December 17, 2024 08:41
Copy link
Contributor

@guy-starkware guy-starkware left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 8 of 8 files at r8.
Reviewable status: all files reviewed (commit messages unreviewed), all discussions resolved (waiting on @matan-starkware)

Copy link
Contributor

@matan-starkware matan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 8 files at r7, 7 of 8 files at r8, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Yael-Starkware)


crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context_test.rs line 61 at r8 (raw file):

    static ref EXECUTABLE_TX_BATCH: Vec<ExecutableTransaction> =
        TX_BATCH.iter().map(|tx| (tx.clone(), &CHAIN_ID).try_into().unwrap()).collect();
}

Suggestion:

lazy_static! {
    static ref TX_BATCH: Vec<Transaction> = (0..3).map(generate_invoke_tx).collect();
    static ref EXECUTABLE_TX_BATCH: Vec<ExecutableTransaction> =
        TX_BATCH.iter().map(|tx| (tx.clone(), &CHAIN_ID).try_into().unwrap()).collect();
}

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 401 at r8 (raw file):

        let chain_id = self.chain_id.clone();

        let handle = tokio::spawn({

Why add another scope?

Code quote:

        let handle = tokio::spawn({

@Yael-Starkware Yael-Starkware force-pushed the yael/remove_tx_hash_from_node_communication branch 2 times, most recently from 60f2f25 to b1d2a08 Compare December 18, 2024 08:56
@Yael-Starkware Yael-Starkware force-pushed the yael/remove_tx_hash_from_node_communication branch from b1d2a08 to d04655f Compare December 18, 2024 08:57
Copy link
Contributor Author

@Yael-Starkware Yael-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 6 of 8 files reviewed, 2 unresolved discussions (waiting on @guy-starkware and @matan-starkware)


crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 401 at r8 (raw file):

Previously, matan-starkware wrote…

Why add another scope?

was just left here by accident form the previous PR on stack.
anyway, deleted now.

Copy link
Contributor

@matan-starkware matan-starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 8 files at r7, 1 of 8 files at r8, 2 of 2 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @Yael-Starkware)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants