feat(starknet_batcher): save BlockExecutionArtifacts in ProposalManager #2725
Conversation
DvirYo-starkware
requested review from
Yael-Starkware,
alonh5,
dafnamatsry,
yair-starkware and
noaov1
|
This change is |
DvirYo-starkware
force-pushed
the
dvir/save_exec_info_in_batcher
branch
from
5f9e18f to
e0adeb1
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 9 files reviewed, 1 unresolved discussion (waiting on @alonh5, @dafnamatsry, @noaov1, @Yael-Starkware, and @yair-starkware)
a discussion (no related file):
I assume that was some discussion about all the optional cloning. If important it can be prevented in two ways:
- add another flag in addition to the
testingflag and importblockierin thebatcherwith this flag - save in the
ProposalManageronly theBlockExecutionArtifactsand only when writing the block to the storage make a conversion. This will increase the latency of the transaction to accept on layer 2 in version14.0
DvirYo-starkware
changed the title
DvirYo-starkware
force-pushed
the
dvir/save_exec_info_in_batcher
branch
from
e0adeb1 to
a74f90c
Compare
DvirYo-starkware
changed the title
There was a problem hiding this comment.
Reviewed 9 of 9 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @alonh5, @dafnamatsry, @noaov1, and @Yael-Starkware)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @DvirYo-starkware, @noaov1, and @Yael-Starkware)
crates/starknet_batcher/src/batcher.rs line 372 at r3 (raw file):
} // TODO(dvir): return `BlockExecutionArtifacts`
Why?
Code quote:
// TODO(dvir): return `BlockExecutionArtifacts`There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5, @dafnamatsry, @noaov1, and @Yael-Starkware)
crates/starknet_batcher/src/batcher.rs line 372 at r3 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
Why?
This information is needed for writing to Aerospike
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @DvirYo-starkware, @noaov1, and @Yael-Starkware)
crates/starknet_batcher/src/utils.rs line 69 at r3 (raw file):
tx_hashes, nonces, block_execution_artifacts: cloned_artifacts,
In this case, consider deleting the ProposalOutput type and storing only the BlockExecutionArtifacts. Fields used from ProposalOutput can be calculated when needed (they are used only once, IIRC).
There was a problem hiding this comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @alonh5, @dafnamatsry, @noaov1, and @Yael-Starkware)
crates/starknet_batcher/src/utils.rs line 69 at r3 (raw file):
Previously, alonh5 (Alon Haramati) wrote…
In this case, consider deleting the
ProposalOutputtype and storing only theBlockExecutionArtifacts. Fields used fromProposalOutputcan be calculated when needed (they are used only once, IIRC).
In addition, it requires more significant changes in the batcher for something that will be deleted after version 14.0, and I try to avoid it.
DvirYo-starkware
force-pushed
the
dvir/save_exec_info_in_batcher
branch
from
a74f90c to
8b9631b
Compare
There was a problem hiding this comment.
Reviewable status: 7 of 10 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @DvirYo-starkware, @noaov1, @Yael-Starkware, and @yair-starkware)
crates/starknet_batcher/src/utils.rs line 69 at r3 (raw file):
Previously, DvirYo-starkware wrote…
In addition, it requires more significant changes in the batcher for something that will be deleted after version 14.0, and I try to avoid it.
On the other hand, we're now doing these calculations for every proposal even though we only need them for the one we decided on.
We recently did a refactor to the batcher (deleting the proposal manager), and I think the ProposalOutput is redundant anyway, this won't be deleted after 0.14.0. Could you try to do this and see if it makes sense? I think it would be better
|
Benchmark movements: |
There was a problem hiding this comment.
Reviewable status: 7 of 10 files reviewed, 2 unresolved discussions (waiting on @dafnamatsry, @DvirYo-starkware, @noaov1, and @yair-starkware)
a discussion (no related file):
Previously, DvirYo-starkware wrote…
I assume that was some discussion about all the optional cloning. If important it can be prevented in two ways:
- add another flag in addition to the
testingflag and importblockierin thebatcherwith this flag- save in the
ProposalManageronly theBlockExecutionArtifactsand only when writing the block to the storage make a conversion. This will increase the latency of the transaction to accept on layer 2 in version14.0
I don't think the consensus can start a new height before decision reached is done, since it will clear all prev height results.
if this is the case, it doesn't really matter if the conversion is done in the propose_block step or in the decision_reached step.
crates/starknet_batcher/src/batcher.rs line 372 at r3 (raw file):
Previously, DvirYo-starkware wrote…
This information is needed for writing to Aerospike
you can also return it in Get_proposal_content and Send_proposal_content
No description provided.