fix(consensus): calculate the executable_transaction in consensus_context

[Yael-Starkware]

chore(starknet_consensus_manager): add chain_id to config

feat(consensus): calculate the executable_transaction in consensus_context

[Yael-Starkware]

• chore(consensus): remove the tx_hash from transactionBatch #2459
• fix(consensus): calculate the executable_transaction in consensus_context #2458 👈 (View in Graphite)
• chore(starknet_api): shorten executable_transaction usage path #2457
• chore(starknet_consensus_manager): add chain_id to config #2433
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.

[reviewable-StarkWare]

This change is 

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.394 ms 34.429 ms 34.468 ms]
change: [-4.5836% -3.0048% -1.6206%] (p = 0.00 < 0.05)
Performance has improved.
Found 6 outliers among 100 measurements (6.00%)
2 (2.00%) high mild
4 (4.00%) high severe

[codecov]

Codecov Report

Attention: Patch coverage is 90.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 18.44%. Comparing base (e3165c4) to head (2eb8a6e).
Report is 720 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/starknet_api/src/transaction.rs	83.33%	0 Missing and 1 partial ⚠️
...tarknet_consensus_manager/src/consensus_manager.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2458       +/-   ##
===========================================
- Coverage   40.10%   18.44%   -21.66%     
===========================================
  Files          26      225      +199     
  Lines        1895    30735    +28840     
  Branches     1895    30735    +28840     
===========================================
+ Hits          760     5670     +4910     
- Misses       1100    24632    +23532     
- Partials       35      433      +398     

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

[github-actions]

Benchmark movements:
tree_computation_flow performance improved 😺
tree_computation_flow time: [34.338 ms 34.389 ms 34.444 ms]
change: [-5.2537% -3.7415% -2.4099%] (p = 0.00 < 0.05)
Performance has improved.
Found 4 outliers among 100 measurements (4.00%)
3 (3.00%) high mild
1 (1.00%) high severe

[Yael-Starkware]

Reviewable status: 1 of 6 files reviewed, 7 unresolved discussions (waiting on @alonh5, @guy-starkware, @matan-starkware, and @MohammadNassar1)

---

crates/papyrus_node/src/config/snapshots/papyrus_node__config__config_test__dump_default_config.snap.new line at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Why is there a new file in this PR?

yeah it seems like it slipped here by accident. removing.

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [35.003 ms 35.460 ms 35.993 ms]
change: [+1.2759% +2.7099% +4.4436%] (p = 0.00 < 0.05)
Performance has regressed.
Found 8 outliers among 100 measurements (8.00%)
1 (1.00%) high mild
7 (7.00%) high severe

[MohammadNassar1]

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

---

crates/starknet_api/src/transaction.rs line 142 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

after digging into this function, it seems that a failure here indicates a bug in the code.
but I will double check this.
@MohammadNassar1 , I see your name on some of this code . how can this function fail?

The function fails when the transaction fields are inappropriate, such as invalid resources. It also fails if an incorrect format for the provided chain_id.

[matan-starkware]

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alonh5 and @guy-starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 539 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

This entire code does not return errors , only panics.
Panic can be handled as error when returning from the handle.join(), but in fact I couldn't find this handling in the code.
@matan-starkware can you explain how you handle these panics?

We don't handle panics as errors. We planned to map out the failure paths and break them into those that cause us to reach an invalid state (think the mutex on valid_proposals being poisoned) or those that simply mean a specific flow failed. I was actually waiting for you to answer Alon's first question to comment here. As in:

• If building the executable TX means we received invalid input from the network then this just means this proposal is invalid and we want to handle that.
• If this instead points to an error within the node maybe it means we really do need to panic?
  I can't answer how we should handle this failure since IDK what this failure means.

[MohammadNassar1]

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

---

crates/starknet_api/src/transaction.rs line 142 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

The function fails when the transaction fields are inappropriate, such as invalid resources. It also fails if an incorrect format for the provided chain_id.

After another look, the resources can no longer be invalid, as the resource mapping was updated from the old dictionary-based implementation to a struct-based approach.

[Yael-Starkware]

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

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 539 at r1 (raw file):

Previously, matan-starkware wrote…

We don't handle panics as errors. We planned to map out the failure paths and break them into those that cause us to reach an invalid state (think the mutex on valid_proposals being poisoned) or those that simply mean a specific flow failed. I was actually waiting for you to answer Alon's first question to comment here. As in:

• If building the executable TX means we received invalid input from the network then this just means this proposal is invalid and we want to handle that.
• If this instead points to an error within the node maybe it means we really do need to panic?
  I can't answer how we should handle this failure since IDK what this failure means.

this specific failure means a real panic, specifically that the ChainId is illegal.

I'm asking cause I was searching the code to see where these errors are mapped to panic/invalid and couldn't find it. Now I understand that this part was not implemented yet, right?

---

crates/starknet_api/src/transaction.rs line 142 at r1 (raw file):

Previously, MohammadNassar1 (mohammad-starkware) wrote…

After another look, the resources can no longer be invalid, as the resource mapping was updated from the old dictionary-based implementation to a struct-based approach.

Continuing the previous discussion:
So the conclusion is that this can only return error in case of invalid ChainId- which means the code should panic.

[matan-starkware]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @alonh5, @guy-starkware, and @Yael-Starkware)

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 539 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

this specific failure means a real panic, specifically that the ChainId is illegal.

I'm asking cause I was searching the code to see where these errors are mapped to panic/invalid and couldn't find it. Now I understand that this part was not implemented yet, right?

Yep not implemented.

Ok so if this does not mean the TX was invalid then expect is good and we do not want to handle this.

---

crates/starknet_api/src/transaction.rs line 142 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

Continuing the previous discussion:
So the conclusion is that this can only return error in case of invalid ChainId- which means the code should panic.

Thanks @MohammadNassar1

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 542 at r2 (raw file):

                        (tx, &chain_id)
                            .try_into()
                            .expect("Failed to convert transaction to executable_transation.")

Suggestion:

                        // An error means we have an invalid chain_id.
                        (tx, &chain_id)
                            .try_into()
                            .expect("Failed to convert transaction to executable_transation.")

[Yael-Starkware]

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

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 539 at r1 (raw file):

Previously, matan-starkware wrote…

Yep not implemented.

Ok so if this does not mean the TX was invalid then expect is good and we do not want to handle this.

Done.

---

crates/sequencing/papyrus_consensus_orchestrator/src/sequencer_consensus_context.rs line 542 at r2 (raw file):

                        (tx, &chain_id)
                            .try_into()
                            .expect("Failed to convert transaction to executable_transation.")

done

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [34.832 ms 35.289 ms 35.828 ms]
change: [+1.4674% +2.7192% +4.4002%] (p = 0.00 < 0.05)
Performance has regressed.
Found 12 outliers among 100 measurements (12.00%)
4 (4.00%) high mild
8 (8.00%) high severe

[guy-starkware]

Reviewable status: 4 of 6 files reviewed, 4 unresolved discussions (waiting on @alonh5, @matan-starkware, and @Yael-Starkware)

[alonh5]

Reviewed 3 of 5 files at r2, 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @matan-starkware and @Yael-Starkware)

---

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

        let chain_id = self.chain_id.clone();

        let handle = tokio::spawn({

I don't think you need these brackets now.

Code quote:

{

[matan-starkware]

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

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [35.503 ms 36.025 ms 36.644 ms]
change: [+1.5432% +3.0877% +5.0155%] (p = 0.00 < 0.05)
Performance has regressed.
Found 12 outliers among 100 measurements (12.00%)
3 (3.00%) high mild
9 (9.00%) high severe

[Yael-Starkware]

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

[Yael-Starkware]

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

---

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

Previously, alonh5 (Alon Haramati) wrote…

I don't think you need these brackets now.

done

[guy-starkware]

Reviewable status: 5 of 6 files reviewed, all discussions resolved (waiting on @alonh5 and @matan-starkware)

[github-actions]

Benchmark movements:
tree_computation_flow performance regressed!
tree_computation_flow time: [35.283 ms 35.779 ms 36.361 ms]
change: [+2.6471% +3.8837% +5.1648%] (p = 0.00 < 0.05)
Performance has regressed.
Found 9 outliers among 100 measurements (9.00%)
1 (1.00%) high mild
8 (8.00%) high severe

[alonh5]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @matan-starkware)