chore(starknet_batcher): set block timestamp in build block input

[ArniStarkware]

No description provided.

[ArniStarkware]

• chore(starknet_batcher): pass block info from consensus to batcher #2238 : 3 dependent PRs (#2267 , #2302 , #2409 )
• chore(starknet_consensus_manager): set proposer address in propose block input #2346
• chore(starknet_batcher): set use_kzg_da flag in build block input #2345
• chore(starknet_batcher): set temp gas prices in propose block input #2341
• chore(starknet_batcher): set block timestamp in build block input #2342 👈 (View in Graphite)
• main

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

[reviewable-StarkWare]

This change is 

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 17.98%. Comparing base (e3165c4) to head (5e5126a).
Report is 655 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #2342       +/-   ##
===========================================
- Coverage   40.10%   17.98%   -22.13%     
===========================================
  Files          26      119       +93     
  Lines        1895    13954    +12059     
  Branches     1895    13954    +12059     
===========================================
+ Hits          760     2509     +1749     
- Misses       1100    11174    +10074     
- Partials       35      271      +236     

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

[Yael-Starkware]

Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @alonh5 and @ArniStarkware)

---

crates/starknet_batcher/src/block_builder.rs line 321 at r1 (raw file):

                    .try_into()
                    .map_err(BlockBuilderError::BadTimestamp)?,
            ),

why did you change it?
I think the previous version was easier to read.

This code will be deleted anyway, right?

Code quote:

            block_timestamp: BlockTimestamp(
                chrono::Utc::now()
                    .timestamp()
                    .try_into()
                    .map_err(BlockBuilderError::BadTimestamp)?,
            ),

---

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

                gas_prices: TEMPORARY_GAS_PRICES,
                block_timestamp: BlockTimestamp(
                    now.timestamp().try_into().expect("Failed to convert timestamp"),

same for the next change.

Suggestion:

chrono::Utc::now().timestamp().try_into().expect("Failed to convert timestamp"),

[alonh5]

Reviewed 2 of 2 files at r1, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ArniStarkware)

---

crates/starknet_batcher/src/block_builder.rs line 49 at r1 (raw file):

pub enum BlockBuilderError {
    #[error(transparent)]
    BadTimestamp(#[from] std::num::TryFromIntError),

Why this change?

[ArniStarkware]

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

Previously, Yael-Starkware (YaelD) wrote…

same for the next change.

So, Should I have two invocations of chorono::Utc::now() five lines apart? Once for deadline and once for block_timestamp.

In my opinion, it is bad - as these will be slightly different from each other - based on how long these lines take to execute.

[ArniStarkware]

Dismissed @alonh5 from a discussion.
Reviewable status: 0 of 2 files reviewed, 2 unresolved discussions (waiting on @alonh5 and @Yael-Starkware)

---

crates/starknet_batcher/src/block_builder.rs line 49 at r1 (raw file):

Previously, alonh5 (Alon Haramati) wrote…

Why this change?

As @Yael-Starkware pointed out - this will be deleted by the next PR anyway.
Reverted.

---

crates/starknet_batcher/src/block_builder.rs line 321 at r1 (raw file):

Previously, Yael-Starkware (YaelD) wrote…

why did you change it?
I think the previous version was easier to read.

This code will be deleted anyway, right?

1. Yes - the code will be deleted anyway. Reverted.
2. I think it is bad that a generic TryFromIntError is always converted to a BadTimestamp. (Again - in theory), what happens if another conversion from integer happens in the scope of Block-build, but has nothing to do with timestamp?

[alonh5]

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

[Yael-Starkware]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @ArniStarkware)

---

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

Previously, ArniStarkware (Arnon Hod) wrote…

So, Should I have two invocations of chorono::Utc::now() five lines apart? Once for deadline and once for block_timestamp.

In my opinion, it is bad - as these will be slightly different from each other - based on how long these lines take to execute.

oh sorry I missed the deadline usage. retracting.

[ArniStarkware]

Reviewed 1 of 2 files at r3, all commit messages.
Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @alonh5)

[ArniStarkware]

Reviewed 1 of 2 files at r3.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @alonh5)