feat(papyrus_protobuf): impl Default for BlockInfo

[asmaastarkware]

No description provided.

[reviewable-StarkWare]

This change is 

[asmaastarkware]

• feat(papyrus_protobuf): impl Default for BlockInfo #3907 👈 (View in Graphite)
• feat(papyrus_protobuf): add BlockInfo to ProposalPart #3884 : 1 other dependent PR (#3908 )
• feat(papyrus_protobuf): add BlockInfo to proto #3878
• main

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

[guy-starkware]

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

---

crates/papyrus_protobuf/src/consensus.rs line 79 at r1 (raw file):

            sequencer_address: Default::default(),
            l1_da_mode: Default::default(),
            l2_gas_price_fri: 1,

I don't have any objections to this specifically. The thing I'm worried about (as with any default) is what happens when people leave it with the default value. Is there a danger that we release very cheap blocks because it got left on the default. I can't really make a judgement call on the implications of having this default.

And also, why not just set some of these to zero? Would it be just as good as 1 in the case of gas price? I would guess it is quite negligible if it is 0 or 1, so I don't know if it matters. Probably the exchange rate set to 1 is better than 0.

Maybe @matan-starkware would have a stronger opinion on the actual values. I'm going to lgtm this in any case.

[matan-starkware]

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

---

crates/papyrus_protobuf/src/consensus.rs line 79 at r1 (raw file):

Previously, guy-starkware wrote…

I don't have any objections to this specifically. The thing I'm worried about (as with any default) is what happens when people leave it with the default value. Is there a danger that we release very cheap blocks because it got left on the default. I can't really make a judgement call on the implications of having this default.

And also, why not just set some of these to zero? Would it be just as good as 1 in the case of gas price? I would guess it is quite negligible if it is 0 or 1, so I don't know if it matters. Probably the exchange rate set to 1 is better than 0.

Maybe @matan-starkware would have a stronger opinion on the actual values. I'm going to lgtm this in any case.

Good questions Guy.

In terms of gas prices 0 is considered an invalid value (the type the Batcher receives is NonzeroGasPrice).

Asmaa, why do we need to implement Default for BlockInfo?

• If this is for tests that really don't care other than basic validations (non zero gas) then I can understand this, but I'd probably prefer the test just define a const.
• If this is for prod I am confused, since we expect prod to actually set all of these fields and validate them.
• If this if for the intermediate step where we don't have the real values for all the fields I prefer you define consts in the sequencer context and then we can remove them when ready (e.g. Guy has the actual L1 gas prices, Ayelet the l2 price, etc.)

[asmaastarkware]

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

---

crates/papyrus_protobuf/src/consensus.rs line 79 at r1 (raw file):

Previously, matan-starkware wrote…

Good questions Guy.

In terms of gas prices 0 is considered an invalid value (the type the Batcher receives is NonzeroGasPrice).

Asmaa, why do we need to implement Default for BlockInfo?

• If this is for tests that really don't care other than basic validations (non zero gas) then I can understand this, but I'd probably prefer the test just define a const.
• If this is for prod I am confused, since we expect prod to actually set all of these fields and validate them.
• If this if for the intermediate step where we don't have the real values for all the fields I prefer you define consts in the sequencer context and then we can remove them when ready (e.g. Guy has the actual L1 gas prices, Ayelet the l2 price, etc.)

no need for it,
closed 👍