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

Juno MVP #20

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from
Draft

Juno MVP #20

wants to merge 8 commits into from

Conversation

IronGauntlets
Copy link
Collaborator

@IronGauntlets IronGauntlets commented Dec 1, 2023

This change is Reviewable

IronGauntlets and others added 8 commits November 29, 2023 16:55
To verify the state diff commitment we need to have access to all the
fields present in the `StateDiff` returned by the feeder gateway.
Therefore, we need to have `deployed_contract` and `replaced_classes` in
the `StateDiff` messages.

The reason for not including the `DeclaredV0Classes` and
`DeclaredV1Classes` is because these can be derived from the `Classes`
message, which all Cairo0 and Cairo1 classes can be mapped to
`DeclaredV0Classes` and `DeclaredV1Classes`, respectively.
The response to `CurrentBlockHeaderRequest{}` is the peer's local
current header. This can be used to get a rough idea of what the latest
block is as viewed by other peers on the network. The header can be
compared with a subset of peers to determine an efficient way of pulling
blocks from the network while syncing.

Initially, we modified the iteration message to include a `latest`
option, however, since the iteration message is shared by multiple
requests it didn't make sense for each of the messages to have access to
it.
Co-authored-by: Krzysztof Lis <klis33@gmail.com>
Co-authored-by: Krzysztof Lis <klis33@gmail.com>
@ArielElp ArielElp self-requested a review December 7, 2023 11:19
Copy link
Contributor

@ArielElp ArielElp 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: 0 of 4 files reviewed, 4 unresolved discussions (waiting on @IronGauntlets)


p2p/proto/block.proto line 33 at r1 (raw file):

    Merkle                    receipts          = 10;  // By order of issuance.
    string                    protocol_version  = 11; // Starknet version
    Felt252                   gas_price         = 12;

I think that we need two numbers for the gas price, similarly to the rpc-spec (eth_l1_gas_price/strk_l1_gas_price)


p2p/proto/receipt.proto line 32 at r1 (raw file):

      uint32 poseidon    = 6;
      uint32 keccak      = 7;
      uint32 output      = 8;

output is not needed here, we also removed it from the spec IIRC


p2p/proto/receipt.proto line 46 at r1 (raw file):

    ExecutionResources execution_resources           = 4;
    string             revert_reason                 = 5;
    optional           MessageToL2 consumed_message  = 6;

Is this needed? we have message hashes in the L1_HANDLER tx receipts, this looks based on the feeder


p2p/proto/state.proto line 26 at r1 (raw file):

    }

    uint32   domain                      = 1;  // volition state domain

Not a blocker for this one, but domain probably not be in the main level (e.g. deployed contracts / declared classes aren't domain-specific)

@IronGauntlets
Copy link
Collaborator Author

@ArielElp

I think that we need two numbers for the gas price, similarly to the rpc-spec (eth_l1_gas_price/strk_l1_gas_price)

Yes I will add it in a separate PR and update this branch

output is not needed here, we also removed it from the spec IIRC

Sure will remove

Is this needed? we have message hashes in the L1_HANDLER tx receipts, this looks based on the feeder

I think this has already been reverted.

Not a blocker for this one, but domain probably not be in the main level (e.g. deployed contracts / declared classes aren't domain-specific)

Sure, although this PR doesn't modify domain.

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.

2 participants