search

ethereum / trin

An Ethereum portal client: a json-rpc server with nearly instant sync, and low CPU & storage usage
365 stars 113 forks source link

fix: testnet protocol ids to matched spec proposal #1298

Closed KolbyML closed 4 months ago

KolbyML commented 4 months ago

What was wrong?

we were image

How was it fixed?

by matching them to the fix I did in my spec PR https://github.com/ethereum/portal-network-specs/pull/258

morph-dev commented 4 months ago

What if we add 0x501B to the mainnet? (we have 0x501A, so this seems like a reasonable next one).

I would change all testnet to 0x51xx or 0x60xx or something that has significantly lower chance of being needed for the mainnet.

KolbyML commented 4 months ago

What if we add 0x501B to the mainnet? (we have 0x501A, so this seems like a reasonable next one).

I think beacon should have used 0x500E not 0x501A

I would change all testnet to 0x51xx or 0x60xx or something that has significantly lower chance of being needed for the mainnet.

image

There are already reserved protocol IDs for future upgrades. Also all Portal Networks are supposed to include 0x50 as it stands for P or Portal Network.

Protocol ids what are reserved are first come first serve so I don't see a reasonable way to prevent someone in the future from using xyz

morph-dev commented 4 months ago

I think beacon should have used 0x500E not 0x501A

That was my first thought as well, but then I realized that maybe it's on purpose with the idea that 0x500x is for execution data and 0x501x is for consensus data, but I could be wrong on this.

I wasn't aware that 0x50 stands for P. In that case, I would add 80 to the second byte, meaning: MAINNET TESTNET
STATE 0x500A 0x508A
HISTORY 0x500B 0x508B
BEACON 0x501A 0x509A

That way there is nice rule for mapping mainnet <-> testnet.

KolbyML commented 4 months ago

I don't think we can make rules like that. For one we aren't limited to 1 testnet in the future. Second if there is a new network added, there is no guarantee we can continue following that pattern, as any 3rd party is free to take any protocol id's, except the few reserved ones.

For that reason I don't think adding an arbitrary 8 is an enforce-able pattern.

I would be fine with doing, I am fine with any number for X, but I don't like the arbitrary A and 9 because of the mainnet choices

MAINNET TESTNET
STATE 0x500A 0x50XA
HISTORY 0x500B 0x50XB
TransactionGossip 0x500C 0x50XC
CanonicalIndices 0x500D 0x50XD
BEACON 0x501A 0x50XE

As this is the cleanest currently, and obviously we won't be able to follow a order when new networks get added for the reasons mentioned above.

morph-dev commented 4 months ago

I don't think we can make rules like that. For one we aren't limited to 1 testnet in the future. Second if there is a new network added, there is no guarantee we can continue following that pattern, as any 3rd party is free to take any protocol id's, except the few reserved ones.

In my opinion, we can use something other than 0x50 for testnet (either this or future), e.g. 0x54 (for T) doesn't sounds too bad :).

And any 3rd party would have no reason to use 0x50 anyway. In fact, I would argue that they have a reason to pick something else on purpose in order to avoid collision with any of our future protocols.

In this case, I would be fine with adding 20 or 40 to the second byte (and having space for more test networks in the future). Yes, we run into problem if we have more than 32/64 networks, but that doesn't seem likely.

I would be fine with doing, I am fine with any number for X, but I don't like the arbitrary A and 9 because of the mainnet choices

As you said, they are not arbitrary, they are picked because of the mainnet choices :).

MAINNET TESTNET
STATE 0x500A 0x50XA
HISTORY 0x500B 0x50XB
TransactionGossip 0x500C 0x50XC
CanonicalIndices 0x500D 0x50XD
BEACON 0x501A 0x50XE

As this is the cleanest currently, and obviously we won't be able to follow a order when new networks get added for the reasons mentioned above.

I would be fine with this as well, but I would still leave 0x50XE free as I already have PR that sets 0x500E for Verkle mainnet. And I would still go with 0x50Y1 for Beacon testnet (where Y=X+1).

But, I recognize that his is very much personal preferences and ultimately doesn't make big difference one way or the other. If others agree with your approach or don't care, I'm not going to block it. I'm just stating my preference.

KolbyML commented 4 months ago

I think this https://github.com/ethereum/portal-network-specs/pull/258 PR is to discuss that then. Not this PR which resolves the immediate conflict. None of this is in the Portal-Network-Spec yet and can/should be changed once that is finalized, but it would be nice to get something working in for now.

Well we are talking about personal opinions this does block this PR from being merged until a resolution to the problem can be realized.

morph-dev commented 4 months ago

Well we are talking about personal opinions this does block this PR from being merged until a resolution to the problem can be realized.

In order not to block this PR further, just leave 0x501E free for Verkle and pick something else for Beacon.