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

Add missing block and receipt message fields #9

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

Conversation

IronGauntlets
Copy link
Collaborator

Please see commit messages for details.

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.
The block header is updated to allow for the P2P spec to be compatible
with the RPC spec. Before these changes, p2p spec was not enough to
serve to block RPC requests.

Block bodies include Transactions and Receipts because RPC requires the
block to have transactions and receipts. A peer could retrieve them in a
separate request, however, since block bodies are retrieved over
multiple messages I don't see much benefit in requesting them
separately.

Also, note Events were added to Receipt common because `BlockID` is not
enough to determine which event belongs to which receipt. We require the
events for the bloom filter.
p2p/proto/block.proto Outdated Show resolved Hide resolved
p2p/proto/block.proto Outdated Show resolved Hide resolved
StateDiff diff = 2;
Classes classes = 3;
BlockProof proof = 4;
Transactions transactions = 6;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

transactoins and events were not supposed to be part of the block body. this can be discussed. the idea was that with stark proofs we don't need to validate transactions by executing them. This is when there's proof which is K blocks behind. Until then the idea was to rely on the consensus (similarly to how Ethereum has block times and epochs).
The protocol can be defined so that txs are mandatory until K and then are optional.
Similarly for receipts & events (which were separated).

minor comment: numbering jumps from 4 to 6

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are all forms of nodes expected to serve the RPC spec? If so, then having transactions and receipts as optional until after K blocks seems unnecessary because all nodes would be fetching them separately to serve the RPC requests. And since block bodies are chunked I don't see any benefit in fetching them separately as that would increase network overhead.

minor comment: numbering jumps from 4 to 6

I will update

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means that to have the data needed to sync with the chain, a node doesn't need to keep transactions before the block with the proof. They can clean their DB. If a node wants to, it can of course keep whatever it wants.
As mentioned, they are not part of the block body, so no need for a client to fetch them. Even if they are included for new blocks, they would be optional and with a separate endpoint to fetch them (from those nodes that keep them beyond the optional point)

Copy link
Collaborator Author

@IronGauntlets IronGauntlets Nov 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What defines whether something should be a part of the block body?

We don't need State Diff or Classes to be part of the block body because State Diff has a commitment which includes the Classes declared in the blocks, therefore, a peer can fetch them separately and verify the state diff commitment with the signature which is over the block hash and state diff commitment.

Note we will have to add BlockID and possibly Classes to State Diff. Also State diff commitment is over the state diff defined by feeder gateway not p2p state diff.

Why can't we include the BlockProof in the BlockHeader? Given other fields are so small 142KB+ other fields' size < 1MB.

If we make the above changes the block bodies can be removed.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, they can fetch them separately. So the thought was what will peers have to fetch in order to be an operational node. They need the current state, they don't need past transactions.

I agree the proof may be part of the header, as optional. Note that we can think of returning many headers in one message (and then it makes sense to have the proof outside)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the thought was what will peers have to fetch in order to be an operational node.

Can you please elaborate on what you mean by an operational node and fetch which messages?

Also, fetching can be done in order but there is no guarantee the messages will arrive in order. So I am not sure what you mean.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was replying to "... includes the Classes declared in the blocks, therefore, a peer can fetch them separately". Operational means a node that can follow the chain. Such a node doesn't need transactions. If a client wants to check transactions state, they will enable that in a node, if they just want to track events, they will enable that. If they want to check state, they can do that.

What messages won't arrive in order? If we support out of order messages in a stream then each will have a reference to be able to order them, if needed.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Operational means a node that can follow the chain. Such a node doesn't need transactions. If a client wants to check transactions state, they will enable that in a node, if they just want to track events, they will enable that. If they want to check state, they can do that.

Why is the RPC spec not sufficient to serve this? I don't see the need to replicate this behaviour on the p2p side. Also, how does a peer verify that the events (or any other date) received are correct?

What messages won't arrive in order? If we support out of order messages in a stream then each will have a reference to be able to order them, if needed.

Messages from a single peer will indeed come in order over a stream but libp2p doesn't allow for ordered messages from multiple peers on a single stream without further multiplexing. If a peer requests information for a block from multiple peers that peer will have multiple streams open, per peer, and thus there is no guarantee block messages part will arrive in order.

p2p/proto/receipt.proto Show resolved Hide resolved
@IronGauntlets IronGauntlets force-pushed the IronGauntlets/update-block-spec branch from 651dfb0 to 1692596 Compare November 17, 2023 13:09
Comment on lines +84 to +85
Transactions transactions = 5;
Receipts receipts = 6;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why should transactions and receipts be included in the response stream?
Why not just use separate queries for them (as in the current spec version)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added the transactions and receipts as part of the block body for completeness as it was represented by the block returned by the feeder gateway. However, you are correct we can fetch them separately.

Based on the same reasoning I don't think we should have a body message at all as we have state commitment which can be used to verify StateDiff and Classes, while the BlockProof can be included in the Header. Also StateDiff, Classes and Block proof are already sent separately just wrapped in a body message.

This also ties in with having multiple protocols instead of one. I may be missing some context but I cannot see under which situation a node would be interested in just transactions/events/receipts/headers etc. How would the node verify the validity of the single message which they retrieved without also retrieving other information such as block header, state diff transactions etc? And why would RPC not be enough (cc:@joshklop)? We can decide to add web sockets for whatever data the user is interested in. In my opinion, we should have a single protocol.

Please see this convo for more context.

@IronGauntlets IronGauntlets marked this pull request as draft November 28, 2023 16:17
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.

6 participants