EIP 2124: Fork identifier for chain compatibility checks #2124

karalabe · 2019-06-19T11:59:52Z

No description provided.

EIPS/eip-2124.md

axic · 2019-06-19T12:41:17Z

Apart these two comments I think it satisfies the EIP-1 requirements.

axic

I think this looks good. Had a brief look at ENR and the Specification here and it seems reasonably explained.

EIPS/eip-2124.md

axic · 2019-06-19T19:51:40Z

@fjl do you want to review this here before it's merged? Anyone else relevant to review this?

nicksavers · 2019-06-20T21:25:46Z

Author e-mail addresses are put between < and >
For the rest it looks perfectly fine for a Draft.

axic · 2019-06-24T10:38:53Z

Author e-mail addresses are put between < and >

Ping @karalabe

fjl · 2019-06-24T10:53:41Z

Sorry, was out last week. I still want to review this.

fjl · 2019-06-24T11:15:37Z

Here is my feedback:

I wouldn't frame this as a "discovery protocol extension". It's not an extension, just using the protocol as intended...
Simple summary is redundant because Abstract just repeats the same content phrased slightly differently.

Technical things:

I don't get why we would XOR chunks of the genesis hash together. We can just use the prefix
of the hash.
Instead of XORing fork numbers, we should look into using crc32 or any other common checksum.
We should also avoid truncating fork block numbers to 32 bits.

fjl · 2019-06-24T12:59:06Z

Here is my proposal for a better fork hash specification:

To calculate the fork checksum of a fork block i, given a list of all prior big-endian 64 bit fork
block numbers [ b₁, b₂, ..., bᵢ ] and names [ n₁, n₂, ..., nᵢ ], create an IEEE CRC32
digest from numbers and names as follows:

forkhash₀ = 0
forkhash₁ = CRC32_update(CRC32_update(forkhash₀, b₁), n₁)
...
forkhashᵢ = CRC32_update(CRC32_update(forkhashᵢ₋₁, bᵢ), nᵢ)

In Go:

func forkHash(numbers []uint64, names []string) uint32 {
	var buf [8]byte
	var hash uint32
	for i := range numbers {
		binary.BigEndian.PutUint64(buf[:], numbers[i])
		hash = crc32.Update(hash, crc32.IEEETable, buf[:])
		hash = crc32.Update(hash, crc32.IEEETable, []byte(names[i]))
	}
	return hash
}

I think this is better because it adds more context into the checksum. We'd need to specify what the names are, but naming forks hasn't been a problem so far.

fjl · 2019-06-24T13:04:12Z

We might even be able to remove the genesis hash entirely by just hashing it into the checksum.

EIPS/eip-2124.md

fjl · 2019-06-24T13:07:44Z

Note that you can still verify compatibility with past forks using the CRC32 checksum. Just calculate all intermediate checksums and match against all of them.

karalabe · 2019-06-24T13:13:33Z

Simple summary is redundant because Abstract just repeats the same content phrased slightly differently.

Yes, it is. I didn't have it originally. @axic forced me to add it.

I don't get why we would XOR chunks of the genesis hash together. We can just use the prefix
of the hash.

I'd like to take into consideration all bytes of the hash. We can use an alternative scheme, I'm not married to it, but XOR felt the simplest.

Instead of XORing fork numbers, we should look into using crc32 or any other common checksum.

Is CRC common enough nowadays to be available for all programming languages?

Re the fork names, I'm not sure that's a good idea. Different clients define forks differently. EIPXYZ vs. Byantium. Even with names, we have variations: Constantinople vs. Constaninople Fix vs. Petersburg vs. St. Petersburg, and this doesn't even take into account variations in capitalization, spacings, symbols. I think it's asking for it to include the names.

We might even be able to remove the genesis hash entirely by just hashing it into the checksum.

Yes, that did occur to me, just figured having the genesis separately might help cleaner separate networks from each other, independent how the forks are applied (or not). E.g. I can create a crawler for Rinkeby without having the crawler support the forks and constantly having to update it with new fork definitions. That said, due to ETH vs. ETC, it might not be useful enough.

fjl · 2019-06-24T13:57:16Z

Yes, it is. I didn't have it originally. @axic forced me to add it.

If you want to reduce complexity, I think the best way is removing all the "discovery protocol" stuff from this EIP. There is really no reason to bother with any particular discovery mechanism here. People
shouldn't need to understand networking or know about any particular discovery protocol to understand this EIP.

It should be enough to mention that publishing a record containing the fork information improves connectivity because other nodes can verify compatibility with the issuer of the record. That's it.

fjl · 2019-06-24T14:01:55Z

Is CRC common enough nowadays to be available for all programming languages?

Yes, a lot of languages have it. Go, Python, Rust have it in the standard library.

I think it's asking for it to include the names.

It's OK to list all canonical fork names so far in this EIP if you worry about compatibility,
but every fork so far had a canonical name. It's up to the client to get that right.

fjl · 2019-06-25T12:43:19Z

Peter and I just had a long call where we tried to resolve some of the design questions
here. We agreed that CRC32 checksum is the way to go. Per discussion, here is my next
attempt:

The forkhash is a 32-bit checksum identifying a blockchain and its effective execution
rule set at the client's current block height. Our assumption is that any blockchain has
an initial rule set and subsequent rule modifications activate at defined fork block
numbers. To calculate forkhash for the 32-byte genesis hash G at block height h,
compute the set of 64 bit fork block numbers { b₁, b₂, ..., bᵢ } where bᵢ > 0 and
bᵢ <= h and create an IEEE CRC32 digest as follows:

forkhash₀ = CRC32_update(0, G)
forkhash₁ = CRC32_update(forkhash₀, b₁)
...
forkhashᵢ = CRC32_update(forkhashᵢ₋₁, bᵢ)

Note that forkhash is oblivious to the nature and number of distinct changes
at any given fork block. What matters is whether there was any potentially
incompatible rule change at all at the fork block.

Example for mainnet:

forkhash₀ = 4234472452  (Genesis)
forkhash₁ = 2546123596  (Homestead at 1150000)
forkhash₂ = 2446457160  (DAO hard fork 1920000)

Example for the Ropsten test network:

forkhash₀ = 818404796   (Genesis)
forkhash₁ = 1668678032  (EIP150, EIP158 forks at block 10)
forkhash₂ = 1050761671  (Byzantium at 1700000)

axic · 2019-06-25T12:48:31Z

You should specify how the block numbers are serialised before hashing. I assume as 64-bit big endian?

fjl · 2019-06-25T12:48:58Z

Yes, that's correct.

axic · 2019-06-27T11:58:38Z

@karalabe @fjl will you update this EIP with the discussion above?

karalabe · 2019-07-03T15:24:32Z

@axic Updated. @fjl PTAL.

fjl · 2019-07-03T16:40:42Z

EIPS/eip-2124.md

+
+This EIP does not attempt to solve the clean separation of 3-way-forks! If at the same future block number, the network splits into three (non-fork, fork-A and fork-B), separating the forkers from each another will need case-by-case special handling. Not handling this keeps the proposal pragmatic, simple and also avoids making it too easy to fork off mainnet.
+
+To keep the scope limited, this EIP only defines the identity scheme and validation rules. The same scheme and algorithm can be embedded into various networking protocols, allowing both the `eth/6x` handshake to be more precise (Ethereum vs. Ethereum Classic); as well as the discovery to be more useful (reject surely peers without ever connecting).


"only defines the identity scheme..." is confusing because ENR also has 'identity schemes' which are unrelated.
Maybe best to just delete the entire paragraph TBH.

Gotta say the whole Abstract could be removed and some of the content moved into Motivation + Rationale where it really belongs.

Yes, but the EIP process requires these 3 completely redundant pieces to exist, and @axic requested that I explicitly have all of them. My original EIP didn't have them. It's stupid to have them. If I've however been requested to have them, I'm not deleting them any more.

"only defines the identity scheme..." is confusing because ENR also has 'identity schemes' which are unrelated.

Well, this EIP is not about ENR per your request, so you should not know about ENR identity schemes at all (as a matter of fact, I've no clue what they are :D).

Yes, but the EIP process requires these 3 completely redundant pieces to exist, and @axic requested that I explicitly have all of them.

Don't blame it on me, I'm just following EIP-1, hehe.

I don't think the intention of EIP-1 is to make you repeat the same text twice with tweaked wording just so you have both sections. Maybe EIP-1 should be changed. Honestly, "simple summary" is the definition of Abstract.

@karalabe I don't understand why the fork ID is an 'identity scheme'. This sentence is the only place where 'identity' appears in the whole document. An 'identity scheme' is a way to assign identities, i.e. on a national identity card. In ENR, an identity scheme is the name of a signing function and public key crypto parameters that define how to assign a node identity.

Well, id in forkid means identity :)

I thought it means identifier.

Hmm, yes, the forkid is an identifier, but doesn't the EIP define the rules (i.e. scheme) of how the ID is generated and regenerated over time? Maybe I have a different mental definition of "scheme". Anyway, I don't much care about the exact wording if you have a better alternative.

Like I said I'd just kill the entire paragraph.

EIPS/eip-2124.md

fjl · 2019-07-03T16:42:26Z

EIPS/eip-2124.md

+
+Here's a full suite of tests for all possible fork IDs that Mainnet, Ropsten, Rinkeby and Görli can advertise given the Petersburg fork cap (time of writing).
+
+```go


We should find a format for the test cases that isn't Go.

Meh. The Go code is table driven, so it's fairly trivial to copy paste. Whether it's a Go "table" or a markdown table, it makes no difference. You still need to copy paste each cell individually.

From my perspective as an author however, I don't have to screw around with converting Go tables to markdown tables, and then when something changes (like XOR to CRC), I don't have to throw all that work out and redo everything all over.

I.e. It saves time for me, it's more robust against change, and it doesn't matter to anyone else.

axic · 2019-07-25T11:57:40Z

@fjl @karalabe is this ready for merge?

karalabe · 2019-07-25T12:03:20Z

From my part yes.

* EIP 2124: Zero RTT netsplit on chain mismatch * Assign number to EIP 2124 * Address review comments on EIP 2124. * Rename EIP 2124 to better reflect content * Fix cardinality * Fix email address in 2124 * Update fork identifier EIP based on review discussions. * Fixup EIP name for the fork identifier. * Remove stale mention of ENR from forkid EIP. * Add RLP encoding test cases for forkid EIP. * Add Felix too to the forkid EIP author list. * Get rid of mentioning the MTU stuffin the forkid EIP.

EIP 2124: Zero RTT netsplit on chain mismatch

0266db4

karalabe mentioned this pull request Jun 19, 2019

EIP-2124: Fork identifier for chain compatibility checks #2125

Closed

Assign number to EIP 2124

0a88392

axic reviewed Jun 19, 2019

View reviewed changes

EIPS/eip-2124.md Show resolved Hide resolved

axic reviewed Jun 19, 2019

View reviewed changes

EIPS/eip-2124.md Outdated Show resolved Hide resolved

Address review comments on EIP 2124.

8925dfb

axic approved these changes Jun 19, 2019

View reviewed changes

axic reviewed Jun 19, 2019

View reviewed changes

EIPS/eip-2124.md Outdated Show resolved Hide resolved

karalabe added 2 commits June 19, 2019 17:38

Rename EIP 2124 to better reflect content

3926e39

Fix cardinality

753fb10

karalabe changed the title ~~EIP 2124: Zero RTT netsplit on chain mismatch~~ EIP 2124: Zero RTT chain compatibility check Jun 19, 2019

axic added the EIP label Jun 19, 2019

Fix email address in 2124

aac35ac

fjl reviewed Jun 24, 2019

View reviewed changes

EIPS/eip-2124.md Outdated Show resolved Hide resolved

karalabe mentioned this pull request Jul 3, 2019

core/forkid: implement the forkid EIP, announce via ENR ethereum/go-ethereum#19738

Merged

Update fork identifier EIP based on review discussions.

406dadf

Fixup EIP name for the fork identifier.

ef75b41

karalabe changed the title ~~EIP 2124: Zero RTT chain compatibility check~~ EIP 2124: Fork identifier for chain compatibility checks Jul 3, 2019

karalabe added 2 commits July 3, 2019 18:46

Remove stale mention of ENR from forkid EIP.

9ee95e2

Add RLP encoding test cases for forkid EIP.

8616237

karalabe mentioned this pull request Jul 3, 2019

Ethereum Core Devs Meeting 64 Agenda ethereum/pm#107

Closed

Add Felix too to the forkid EIP author list.

9f8e59d

fjl suggested changes Jul 3, 2019

View reviewed changes

fjl reviewed Jul 3, 2019

View reviewed changes

Get rid of mentioning the MTU stuffin the forkid EIP.

b4306af

axic added type: Networking and removed EIP labels Jul 4, 2019

fjl approved these changes Aug 6, 2019

View reviewed changes

Merge remote-tracking branch 'ethereum/master' into HEAD

43e20a7

axic approved these changes Aug 8, 2019

View reviewed changes

axic merged commit a7c13f4 into ethereum:master Aug 8, 2019

benbaley mentioned this pull request Aug 24, 2021

[update needed] implement the forkid EIP, announce via ENR AlayaNetwork/Alaya-Go#108

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EIP 2124: Fork identifier for chain compatibility checks #2124

EIP 2124: Fork identifier for chain compatibility checks #2124

karalabe commented Jun 19, 2019

axic commented Jun 19, 2019

axic left a comment

axic commented Jun 19, 2019

nicksavers commented Jun 20, 2019

axic commented Jun 24, 2019

fjl commented Jun 24, 2019

fjl commented Jun 24, 2019 •

edited

Loading

fjl commented Jun 24, 2019 •

edited

Loading

fjl commented Jun 24, 2019

fjl commented Jun 24, 2019

karalabe commented Jun 24, 2019

fjl commented Jun 24, 2019

fjl commented Jun 24, 2019

fjl commented Jun 25, 2019 •

edited

Loading

axic commented Jun 25, 2019

fjl commented Jun 25, 2019

axic commented Jun 27, 2019

karalabe commented Jul 3, 2019

fjl Jul 3, 2019

karalabe Jul 3, 2019 •

edited

Loading

karalabe Jul 3, 2019

axic Jul 3, 2019

fjl Jul 8, 2019

fjl Jul 8, 2019

karalabe Jul 8, 2019

fjl Jul 8, 2019

karalabe Jul 8, 2019

fjl Jul 8, 2019

fjl Jul 3, 2019

karalabe Jul 3, 2019

axic commented Jul 25, 2019

karalabe commented Jul 25, 2019


		This EIP does not attempt to solve the clean separation of 3-way-forks! If at the same future block number, the network splits into three (non-fork, fork-A and fork-B), separating the forkers from each another will need case-by-case special handling. Not handling this keeps the proposal pragmatic, simple and also avoids making it too easy to fork off mainnet.

		To keep the scope limited, this EIP only defines the identity scheme and validation rules. The same scheme and algorithm can be embedded into various networking protocols, allowing both the `eth/6x` handshake to be more precise (Ethereum vs. Ethereum Classic); as well as the discovery to be more useful (reject surely peers without ever connecting).


		Here's a full suite of tests for all possible fork IDs that Mainnet, Ropsten, Rinkeby and Görli can advertise given the Petersburg fork cap (time of writing).

		```go

EIP 2124: Fork identifier for chain compatibility checks #2124

EIP 2124: Fork identifier for chain compatibility checks #2124

Conversation

karalabe commented Jun 19, 2019

axic commented Jun 19, 2019

axic left a comment

Choose a reason for hiding this comment

axic commented Jun 19, 2019

nicksavers commented Jun 20, 2019

axic commented Jun 24, 2019

fjl commented Jun 24, 2019

fjl commented Jun 24, 2019 • edited Loading

fjl commented Jun 24, 2019 • edited Loading

fjl commented Jun 24, 2019

fjl commented Jun 24, 2019

karalabe commented Jun 24, 2019

fjl commented Jun 24, 2019

fjl commented Jun 24, 2019

fjl commented Jun 25, 2019 • edited Loading

axic commented Jun 25, 2019

fjl commented Jun 25, 2019

axic commented Jun 27, 2019

karalabe commented Jul 3, 2019

Choose a reason for hiding this comment

karalabe Jul 3, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

axic commented Jul 25, 2019

karalabe commented Jul 25, 2019

fjl commented Jun 24, 2019 •

edited

Loading

fjl commented Jun 24, 2019 •

edited

Loading

fjl commented Jun 25, 2019 •

edited

Loading

karalabe Jul 3, 2019 •

edited

Loading