feat: add support for configuring network protocol_id's #1093

KolbyML · 2024-01-11T22:04:29Z

What was wrong?

We aren't able to configure protocol_id in the case we want to support different networks like a testnet

How was it fixed?

This is basically the same thing we already did except we use a bidirectional map instead of trait conversions acting as a bidirectional map.

For storing mainnet/future network data we will use a folder structure I seen L1 clients use trin/mainnet/... trin/futurenetworkname/...

Now we can easily configure future networks

njgheorghita

I appreciate the pr & work towards this direction. But imo we shouldn't merge any testnet implementation until ethereum/portal-network-specs#258 is approved by all client teams & merged

KolbyML · 2024-01-12T19:22:14Z

@njgheorghita

Piper seems to have the complete opposite view point. That clients should start doing stuff relating to this and only finalize it in spec, once we have figured shit out and are using it persumably.

By what is said between you and piper this PR and us even doing a testnet is in a deadlock now. This PR more has to do with configuring protocol ID's and I don't see how that a PR formalizing protocol_ids for a testnet blocks a pr allowing the option to configure protocol ID's.

Being able to configure network parameters is a standard feature in all L1 clients. I can comprehend how we wouldn't want to formalize the testnet protocol_ids in our code, but that is 15 lines of 200. This code could have been reviewed and we could have just removed the 15 line testnet part instead of a blanket ignore because the word testnet was mentioned.

Shouldn't by the same logic we should have blocked TraceGossip till it was merged into the spec and reviewed by all client teams? I don't see why 95% (configure protocol id's) of this PR is being blocked for 5% (15 lines of testnet parameters)

I have removed the code relating to "testnet" from the PR so hopefully the code can be reviewed now

pipermerriam · 2024-01-13T19:51:36Z

I wouldn't categorize my opinion as "strong". Only that since we haven't done "testnet" yet my intuition is that actually doing the implementation will likely uncovers somes "unknown unknown" types of learning... but feel free to proceed however you guys feel is best.

I think the only thing I have a "strong" opinion on is that we should not write the spec and then blindly implement the clients to follow the spec. Lets make sure that client implementation learnings are actively used to influence the specification because things don't always unfold the way we expect them to.

njgheorghita

I think kurtosis could be useful in testing this.
Something like...

Spin up 3 trin nodes running testnet
Spin up 3 trin nodes running mainnet
Send a ping between the testnet & mainnet clients
Spin up a bridge gossiping testnet data (eg epoch # 1)
Spin up a bridge gossiping mainnet data (eg epoch # 2)

Test that the mainnet nodes only store epoch # 2 data, and testnet nodes only have epoch # 1 data.

KolbyML · 2024-02-26T20:28:17Z

I think kurtosis could be useful in testing this. Something like...

Send a ping between the testnet & mainnet clients

We can test it with that and also add a test for it in peer tests.

If you try to ping a testnet history node to a mainnet history node the ping will fail because the networks have different Discv5 TalkReq ids which is what we use to route messages with

Assuming we use this proposal for running testnets*

So the test could look like in peertests

Start a history testnet node
Start a history mainnet node
send a ping
expect no result

KolbyML · 2024-04-13T02:42:44Z

@njgheorghita @ogenev ready for a review

I added the small peertest I talked about in the call and comment above

njgheorghita

Looks pretty good, just a few comments on fn naming. But, I'd also like to hold off on merging this until after the db update is deployed, since this will require some changes to the cluster repo, and it'd be better to do the 2 somewhat significant changes in separate goes to isolate their effects/bugs from each other

njgheorghita · 2024-04-15T17:26:13Z

ethportal-api/src/types/cli.rs

+    match network_string {
+        "mainnet" => Ok(MAINNET.clone()),
+        "testnet" => Ok(TESTNET.clone()),
+        _ => Err(format!("Not a valid network: {network_string}")),


"Not a valid network: {network_string}, must be 'testnet' or 'mainnet'"

njgheorghita · 2024-04-15T17:42:28Z

ethportal-api/src/types/portal_wire.rs

+    let mut portal_networks = BiHashMap::new();
+    portal_networks.insert(ProtocolId::State, "0x501A".to_string());
+    portal_networks.insert(ProtocolId::History, "0x501B".to_string());
+    portal_networks.insert(ProtocolId::TransactionGossip, "0x501C".to_string());


I don't see any reason to include TransactionGossip & CanonicalIndices?

I mean it is included in our mainnet code

njgheorghita · 2024-04-15T17:44:53Z

ethportal-api/src/types/portal_wire.rs

    }

    #[test]
    fn protocol_id_encoding() {
        let hex = "0x500A";
-        let protocol_id = ProtocolId::from_str(hex).unwrap();
-        let expected_hex = hex_encode_upper(Vec::try_from(protocol_id).unwrap());
+        let protocol_id = MAINNET.portal_networks.get_by_right(hex).unwrap();


I'm not a huge fan of using the native BiHashMap api. It's not obvious (without looking up the map &/or BiHashMap source code), what these functions do (aka left vs. right)... Could you impl something more intuitive, like get_protocol_id / get_protocol_id_hex (or whatever fn name makes the most sense here)?

njgheorghita · 2024-04-15T17:46:43Z

portalnet/src/events.rs

+        let protocol_id = self
+            .network_spec
+            .portal_networks
+            .get_by_right(&hex_encode_upper(request.protocol()));


Same here. Some custom impl that has a more intuitive name would be better

njgheorghita · 2024-04-15T17:47:53Z

src/lib.rs

@@ -100,7 +111,11 @@ pub async fn run_trin(
        beacon_event_tx,
        beacon_jsonrpc_tx,
        beacon_event_stream,
-    ) = if trin_config.networks.iter().any(|val| val == BEACON_NETWORK) {
+    ) = if trin_config
+        .portal_subnetworks


Think we could also do a custom trin_config.portal_subnetwork.contains() or something similar

njgheorghita · 2024-04-15T17:49:10Z

tests/self_peertest.rs

@@ -206,13 +206,22 @@ async fn peertest_state_offer_contract_bytecode() {
 #[tokio::test(flavor = "multi_thread")]
 #[serial]
 async fn peertest_state_recursive_gossip() {
-    let (peertest, target, handle) = setup_peertest().await;
+    let (peertest, target, handle) = setup_peertest(None).await;


I think it would be a bit better to get rid of the Option and just use "mainnet" as the arg here

KolbyML · 2024-04-15T19:30:20Z

since this will require some changes to the cluster repo,

This PR requires 0 changes to the cluster repo, to my knowledge? Would it be possible to provide an explanation on how that conclusion was achieved? Maybe I am misunderstanding something.

KolbyML · 2024-04-15T23:14:14Z

ready for another look

njgheorghita · 2024-04-16T14:34:10Z

Well, it's not a major change, but we'll need to update the --networks flag to --portal-subnetworks. Anyways, I guess it's not a huge blocker for this pr, just something that we'll make sure we want to do in parallel. And if we forget to do it, the nodes will crash, so the problem will be pretty obvious. So, I guess nvm let's go ahead with the merge here.

KolbyML self-assigned this Jan 11, 2024

KolbyML force-pushed the configure-testnet branch from 69b8891 to 9b07c0c Compare January 12, 2024 16:19

KolbyML requested review from njgheorghita, ogenev and carver January 12, 2024 16:21

KolbyML marked this pull request as ready for review January 12, 2024 16:21

KolbyML force-pushed the configure-testnet branch 2 times, most recently from 8976003 to ea72788 Compare January 12, 2024 16:40

njgheorghita reviewed Jan 12, 2024

View reviewed changes

KolbyML force-pushed the configure-testnet branch from 31fab5d to 2a37f22 Compare January 12, 2024 19:32

KolbyML changed the title ~~feat: support multiple network MAINNET/TESTNET etc~~ feat: support multiple network MAINNET and for future networks Jan 12, 2024

KolbyML requested a review from njgheorghita January 12, 2024 19:33

KolbyML changed the title ~~feat: support multiple network MAINNET and for future networks~~ feat: add support for configuring network protocol_id's Jan 12, 2024

njgheorghita reviewed Feb 26, 2024

View reviewed changes

KolbyML force-pushed the configure-testnet branch 5 times, most recently from 55967aa to e34e9b6 Compare April 12, 2024 23:48

feat: support multiple network MAINNET/TESTNET etc

0a25f79

KolbyML force-pushed the configure-testnet branch from e34e9b6 to 0a25f79 Compare April 13, 2024 01:29

KolbyML requested a review from njgheorghita April 13, 2024 02:41

njgheorghita approved these changes Apr 15, 2024

View reviewed changes

fix: resolve pr concerns

917b0f0

KolbyML merged commit 492f1d8 into ethereum:master Apr 17, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add support for configuring network protocol_id's #1093

feat: add support for configuring network protocol_id's #1093

KolbyML commented Jan 11, 2024 •

edited

Loading

njgheorghita left a comment

KolbyML commented Jan 12, 2024 •

edited

Loading

pipermerriam commented Jan 13, 2024

njgheorghita left a comment

KolbyML commented Feb 26, 2024

KolbyML commented Apr 13, 2024

njgheorghita left a comment

njgheorghita Apr 15, 2024

njgheorghita Apr 15, 2024

KolbyML Apr 15, 2024

njgheorghita Apr 15, 2024

njgheorghita Apr 15, 2024

njgheorghita Apr 15, 2024

njgheorghita Apr 15, 2024

KolbyML commented Apr 15, 2024

KolbyML commented Apr 15, 2024

njgheorghita commented Apr 16, 2024

feat: add support for configuring network protocol_id's #1093

feat: add support for configuring network protocol_id's #1093

Conversation

KolbyML commented Jan 11, 2024 • edited Loading

What was wrong?

How was it fixed?

njgheorghita left a comment

Choose a reason for hiding this comment

KolbyML commented Jan 12, 2024 • edited Loading

pipermerriam commented Jan 13, 2024

njgheorghita left a comment

Choose a reason for hiding this comment

KolbyML commented Feb 26, 2024

KolbyML commented Apr 13, 2024

njgheorghita left a comment

Choose a reason for hiding this comment

njgheorghita Apr 15, 2024

Choose a reason for hiding this comment

njgheorghita Apr 15, 2024

Choose a reason for hiding this comment

KolbyML Apr 15, 2024

Choose a reason for hiding this comment

njgheorghita Apr 15, 2024

Choose a reason for hiding this comment

njgheorghita Apr 15, 2024

Choose a reason for hiding this comment

njgheorghita Apr 15, 2024

Choose a reason for hiding this comment

njgheorghita Apr 15, 2024

Choose a reason for hiding this comment

KolbyML commented Apr 15, 2024

KolbyML commented Apr 15, 2024

njgheorghita commented Apr 16, 2024

KolbyML commented Jan 11, 2024 •

edited

Loading

KolbyML commented Jan 12, 2024 •

edited

Loading