networking: more complete gossipsub spec #56
Conversation
…dded readme on how to select devnet
Signature Aggregation Preparation (devnet 2)
…ig::devnet() and update lib exports
ArtiomTr
left a comment
ArtiomTr
left a comment
There was a problem hiding this comment.
so probably all gossipsub code needs to be deleted. instead, you should implement only ethereum-specific logic on top of existing gossipsub protocol, like how eth2_libp2p is implemented
| /// - Gossipsub v1.2: <https://github.com/libp2p/specs/blob/master/pubsub/gossipsub/gossipsub-v1.2.md> | ||
| use serde::{Deserialize, Serialize}; | ||
|
|
||
| use super::types::MessageId; |
There was a problem hiding this comment.
Why there is whole gossipsub protocol reimplemented? Why libp2p::gossipsub doesn't fit there?
There was a problem hiding this comment.
Good point, removed the implementation, used the existing one.
lean_client/containers/src/types.rs
Outdated
| /// 20-byte array for message IDs (gossipsub message IDs) | ||
| /// Using transparent SSZ encoding - just the raw bytes | ||
| #[derive(Clone, Copy, Debug, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] | ||
| pub struct Bytes20(pub [u8; 20]); |
There was a problem hiding this comment.
so I believe there is no need to create separate type Bytes20 - that is not really idiomatic in rust, since you already have fixed-size slices. It makes sense only if you encapsulate some logic in it (e.g., some additional validation for message ids), otherwise not worth to implement this.
If you want structs with proper ssz support, feel free to use H160 from ethereum_types crate - ssz traits are already implemented for these types: https://github.com/grandinetech/grandine/blob/db5ba01d3eaad36242409834fef1bacec33af0e1/ssz/src/arrays.rs#L52
There was a problem hiding this comment.
Thanks! Used H160
| fn default_protocol_id() -> String { | ||
| "/meshsub/1.3.0".to_string() | ||
| } | ||
|
|
||
| fn default_d() -> usize { | ||
| 8 | ||
| } | ||
|
|
||
| fn default_d_low() -> usize { | ||
| 6 | ||
| } | ||
|
|
||
| fn default_d_high() -> usize { | ||
| 12 | ||
| } | ||
|
|
||
| fn default_d_lazy() -> usize { | ||
| 6 | ||
| } | ||
|
|
||
| fn default_heartbeat_interval_secs() -> f64 { | ||
| 0.7 | ||
| } | ||
|
|
||
| fn default_fanout_ttl_secs() -> u64 { | ||
| 60 | ||
| } | ||
|
|
||
| fn default_mcache_len() -> usize { | ||
| 6 | ||
| } | ||
|
|
||
| fn default_mcache_gossip() -> usize { | ||
| 3 | ||
| } | ||
|
|
||
| fn default_seen_ttl_secs() -> u64 { | ||
| let justification_lookback_slots: u64 = 3; | ||
| let seconds_per_slot: u64 = 12; | ||
| seconds_per_slot * justification_lookback_slots * 2 | ||
| } | ||
|
|
||
| fn default_idontwant_threshold() -> usize { | ||
| 1000 | ||
| } |
There was a problem hiding this comment.
no need to define each of those as separate functions - just put them into Default trait impl
| #[test] | ||
| fn test_default_parameters() { | ||
| let params = GossipsubParameters::default(); | ||
|
|
||
| // Test Ethereum spec values | ||
| assert_eq!(params.d, 8); | ||
| assert_eq!(params.d_low, 6); | ||
| assert_eq!(params.d_high, 12); | ||
| assert_eq!(params.d_lazy, 6); | ||
| assert_eq!(params.heartbeat_interval_secs, 0.7); | ||
| assert_eq!(params.fanout_ttl_secs, 60); | ||
| assert_eq!(params.mcache_len, 6); | ||
| assert_eq!(params.mcache_gossip, 3); | ||
| assert_eq!(params.protocol_id, "/meshsub/1.3.0"); | ||
| assert_eq!(params.idontwant_message_size_threshold, 1000); | ||
|
|
||
| // Test relationships | ||
| assert!(params.d_low < params.d); | ||
| assert!(params.d < params.d_high); | ||
| assert!(params.d_lazy <= params.d); | ||
| assert!(params.mcache_gossip <= params.mcache_len); | ||
| } |
There was a problem hiding this comment.
no need for this test, it doesn't test anything
Implemented equivalent changes that can be seen with this commit: leanEthereum/leanSpec@094cb0b
Added tests for new code
All tests pass both old and new
Tested discovery just in case, works