apollo_protobuf: added propeller proto file#10654
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
6cf4ed0 to
d718521
Compare
41b33f7 to
2431498
Compare
guy-starkware
left a comment
There was a problem hiding this comment.
@guy-starkware reviewed 4 of 4 files at r1, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @noamsp-starkware and @ShahakShama)
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 1 file and all commit messages, and made 8 comments.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @noamsp-starkware and @sirandreww-starkware).
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 8 at r1 (raw file):
// option go_package = "github.com/starknet-io/starknet-p2pspecs/p2p/proto/propeller"; message PropellerUnit {
Add documentation to messages and fields if needed
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 9 at r1 (raw file):
message PropellerUnit { uint32 channel = 1;
What is channel? Document it
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 10 at r1 (raw file):
message PropellerUnit { uint32 channel = 1; bytes publisher = 2;
Use PeerID in common.proto
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 11 at r1 (raw file):
uint32 channel = 1; bytes publisher = 2; bytes root = 3;
Put root next to proof as they have the same use case
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 11 at r1 (raw file):
uint32 channel = 1; bytes publisher = 2; bytes root = 3;
rename root to merkle_root and proof to merkle_proof
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 13 at r1 (raw file):
bytes root = 3; bytes signature = 4; uint32 index = 5;
Are you sure 32 bits is enough and we shouldn't prefer uint64?
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 14 at r1 (raw file):
bytes signature = 4; uint32 index = 5; bytes shard = 6;
IMO this should be the first field. Try to sort all fields by importance
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 15 at r1 (raw file):
uint32 index = 5; bytes shard = 6; bytes proof = 7;
Add the structure of a storage proof as a protobuf message
d718521 to
1c3d7be
Compare
2431498 to
f9becd5
Compare
1c3d7be to
b41c142
Compare
f9becd5 to
755eeea
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 8 comments.
Reviewable status: 1 of 4 files reviewed, 8 unresolved discussions (waiting on @guy-starkware, @noamsp-starkware, and @ShahakShama).
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 8 at r1 (raw file):
Previously, ShahakShama wrote…
Add documentation to messages and fields if needed
Done.
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 9 at r1 (raw file):
Previously, ShahakShama wrote…
What is channel? Document it
Done.
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 10 at r1 (raw file):
Previously, ShahakShama wrote…
Use PeerID in common.proto
Done.
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 11 at r1 (raw file):
Previously, ShahakShama wrote…
Put root next to proof as they have the same use case
Done.
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 11 at r1 (raw file):
Previously, ShahakShama wrote…
rename root to merkle_root and proof to merkle_proof
Done.
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 13 at r1 (raw file):
Previously, ShahakShama wrote…
Are you sure 32 bits is enough and we shouldn't prefer uint64?
Done.
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 14 at r1 (raw file):
Previously, ShahakShama wrote…
IMO this should be the first field. Try to sort all fields by importance
Done.
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 15 at r1 (raw file):
Previously, ShahakShama wrote…
Add the structure of a storage proof as a protobuf message
Done.
noamsp-starkware
left a comment
There was a problem hiding this comment.
@noamsp-starkware reviewed 4 files and all commit messages, and made 1 comment.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @ShahakShama and @sirandreww-starkware).
Cargo.lock line 5313 at r2 (raw file):
dependencies = [ "data-encoding", "syn 1.0.109",
?
Same for the rest of the changes in this file.
35b87cc to
3ac804b
Compare
0d3c99a to
cf0c62e
Compare
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 2 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @guy-starkware, @noamsp-starkware, and @sirandreww-starkware).
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 32 at r2 (raw file):
Previously, sirandreww-starkware (Andrew Luka) wrote…
Each channel has it's own set of peers and stake per peer. The routing and route validation of the shard relies on all the nodes having the exact same set of (peer, stake)
Rename according to what we decide on slack
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 32 at r3 (raw file):
bytes signature = 6; // Logical channel identifier for multiplexing different message streams. uint32 channel = 7;
If this is incremented for every epoch, it should probably be uint64 too
noamsp-starkware
left a comment
There was a problem hiding this comment.
@noamsp-starkware reviewed 2 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @guy-starkware and @sirandreww-starkware).
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @guy-starkware and @sirandreww-starkware).
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama made 1 comment.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @guy-starkware and @sirandreww-starkware).
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 24 at r3 (raw file):
uint64 index = 2; // The Merkle root of all shards, used to verify shard integrity. Hash256 merkle_root = 3;
Don't you also need a nonce to avoid replay? Does gossipsub have nonce?
e2cd896 to
caa361b
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 3 comments.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @guy-starkware, @noamsp-starkware, and @ShahakShama).
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 32 at r2 (raw file):
Previously, ShahakShama wrote…
Rename according to what we decide on slack
We decided to keep it as is for now, and possibly rename it after all the code is in because this rename needs to be propagated to other parts of the code. I'll add a TODO.
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 24 at r3 (raw file):
Previously, ShahakShama wrote…
Don't you also need a nonce to avoid replay? Does gossipsub have nonce?
Excellent observation, the short answer is that I thought we had solved this using message cache timeouts but after you asked your question I did more research on how gossipsub works and you might be right that we need to add an extra level of security.
I had gemini do a deep research on how gossipsub prevents replay attacks and got the summary that I added bellow..
In the current propeller implementation, when validating a shard we check if it has been received prior to this point by checking if it is in some set (that has a timeout that is also around 2 minutes). Doing a re-play attack that is very recent would result in shard validation failure which can then result in reporting the peer and probably escalating the situation. I see this as a solved issue in this case.
Doing a re-play attack after the timeout can be harmful, it opens us up to being DOSed by a single peer with a lot of saved shards. Causing us to spawn tasks to handle receiving messages that never come. We should implement a similar mechanism with timestamps to handle this case similarly to gossipsub.
I'll add a TODO to do this plus open a monday task to do this
Gossipsub employs a "Defense-in-Depth" strategy to mitigate replay attacks, layering immediate deduplication with long-term logical validation and economic incentives.
Layer 1: Immediate Deduplication (seen cache) The first line of defense is the seen cache (TimeCache), typically implemented as a sliding window of Bloom filters. It tracks the Message IDs of all recently seen messages for a specific Time-To-Live (TTL), usually defaulting to 2 minutes. This prevents immediate broadcast storms and routing loops by silently dropping duplicates at the wire level before they propagate further.
Layer 2: Infinite-Horizon Protection (seqno) To protect against replays beyond the short cache window, Gossipsub utilizes the BasicSeqnoValidator. Messages carry a monotonically increasing sequence number (seqno), often a timestamp. The validator ensures that a received message's seqno is strictly greater than the highest seqno previously seen from that peer, rejecting any old data regardless of when it arrives. This mechanism relies on strict cryptographic signatures to bind the seqno to the author and payload, preventing attackers from modifying the sequence number to bypass checks.
Layer 3: Economic Deterrence (Peer Scoring) Gossipsub v1.1 introduced Peer Scoring to disincentivize attacks. If a peer transmits a message that fails validation (e.g., a malicious replay or signature failure), they incur a heavy penalty (P4). If their score drops below a threshold, they are "graylisted" and disconnected, turning the attack into a costly action that results in network isolation.
Layer 4: Bandwidth Optimization (IDONTWANT) In v1.2, the protocol introduced IDONTWANT messages. While primarily a bandwidth optimization, this allows nodes to preemptively signal that they already possess a specific message ID, suppressing the transmission of redundant payloads and reducing the impact of replay flooding.
Layer 5: Application Responsibility The protocol relies on the application layer to enforce semantic validity, such as checking embedded timestamps to reject messages that might be technically "new" to the validator (e.g., after a node restart) but are historically obsolete. This necessity was highlighted by incidents like the Ceramic network outage, where a lack of strict timestamp enforcement allowed old messages to persist and saturate the network.
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 32 at r3 (raw file):
Previously, ShahakShama wrote…
If this is incremented for every epoch, it should probably be uint64 too
This also is something we will leave until the code is in. I'll add a TODO.
guy-starkware
left a comment
There was a problem hiding this comment.
@guy-starkware reviewed 2 files and all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @ShahakShama).
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama made 3 comments and resolved 1 discussion.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @sirandreww-starkware).
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 32 at r2 (raw file):
We decided to keep it as is for now
Then there's no need for TODO
regardless, I believe that 2 TODOS in the same line means that the 2nd TODO is swallowed and forgotten
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 32 at r3 (raw file):
Previously, sirandreww-starkware (Andrew Luka) wrote…
This also is something we will leave until the code is in. I'll add a TODO.
Why? Is it purely to avoid restacking or something else? If the former, please remove the part of "consider" from the TODO
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 34 at r4 (raw file):
// Logical channel identifier for multiplexing different message streams. uint32 channel = 7; // CRITICAL TODO(AndrewL): protect against replay attacks using a sequence number with a timestamp.
with a timestamp is a suggestion that I'm not sure about.
caa361b to
86214a9
Compare
sirandreww-starkware
left a comment
There was a problem hiding this comment.
@sirandreww-starkware made 2 comments.
Reviewable status: 2 of 4 files reviewed, 2 unresolved discussions (waiting on @guy-starkware and @ShahakShama).
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 32 at r3 (raw file):
Previously, ShahakShama wrote…
Why? Is it purely to avoid restacking or something else? If the former, please remove the part of "consider" from the TODO
Done.
crates/apollo_protobuf/src/proto/p2p/proto/propeller/propeller.proto line 34 at r4 (raw file):
Previously, ShahakShama wrote…
with a timestamp is a suggestion that I'm not sure about.
Done.
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 2 files and all commit messages, made 1 comment, and resolved 2 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @sirandreww-starkware).
bbfca5f to
f089672
Compare
ShahakShama
left a comment
There was a problem hiding this comment.
@ShahakShama reviewed 3 files and all commit messages, and made 1 comment.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @sirandreww-starkware).
f089672 to
4e9d9c1
Compare
b73eeb8

No description provided.