core/forkid: add two clauses for more precise validation (ethereum#20220

)

core/forkid/forkid.go

@@ -123,10 +123,13 @@ func newFilter(config *params.ChainConfig, genesis common.Hash, headfn func() ui
 	// Create a validator that will filter out incompatible chains
 	return func(id ID) error {
 		// Run the fork checksum validation ruleset:
-		//   1. If local and remote FORK_CSUM matches, connect.
+		//   1. If local and remote FORK_CSUM matches, compare local head to FORK_NEXT.
 		//        The two nodes are in the same fork state currently. They might know
 		//        of differing future forks, but that's not relevant until the fork
 		//        triggers (might be postponed, nodes might be updated to match).
+		//      1a. A remotely announced but remotely not passed block is already passed
+		//          locally, disconnect, since the chains are incompatible.
+		//      1b. No remotely announced fork; or not yet passed locally, connect.
 		//   2. If the remote FORK_CSUM is a subset of the local past forks and the
 		//      remote FORK_NEXT matches with the locally following fork block number,
 		//      connect.
@@ -148,7 +151,12 @@ func newFilter(config *params.ChainConfig, genesis common.Hash, headfn func() ui
 			// Found the first unpassed fork block, check if our current state matches
 			// the remote checksum (rule #1).
 			if sums[i] == id.Hash {
-				// Yay, fork checksum matched, ignore any upcoming fork
+				// Fork checksum matched, check if a remote future fork block already passed
+				// locally without the local node being aware of it (rule #1a).
+				if id.Next > 0 && head >= id.Next {
+					return ErrLocalIncompatibleOrStale
+				}
+				// Haven't passed locally a remote-only fork, accept the connection (rule #1b).
 				return nil
 			}
 			// The local and remote nodes are in different forks currently, check if the

core/forkid/forkid_test.go

@@ -151,7 +151,7 @@ func TestValidation(t *testing.T) {
 
 		// Local is mainnet Petersburg, remote announces Byzantium + knowledge about Petersburg. Remote
 		// is simply out of sync, accept.
-		{7987396, ID{Hash: checksumToBytes(0x668db0af), Next: 7280000}, nil},
+		{7987396, ID{Hash: checksumToBytes(0xa00bc324), Next: 7280000}, nil},
 
 		// Local is mainnet Petersburg, remote announces Spurious + knowledge about Byzantium. Remote
 		// is definitely out of sync. It may or may not need the Petersburg update, we don't know yet.
@@ -178,6 +178,16 @@ func TestValidation(t *testing.T) {
 
 		// Local is mainnet Petersburg, remote is Rinkeby Petersburg.
 		{7987396, ID{Hash: checksumToBytes(0xafec6b27), Next: 0}, ErrLocalIncompatibleOrStale},
+
+		// Local is mainnet Petersburg, far in the future. Remote announces Gopherium (non existing fork)
+		// at some future block 88888888, for itself, but past block for local. Local is incompatible.
+		//
+		// This case detects non-upgraded nodes with majority hash power (typical Ropsten mess).
+		{88888888, ID{Hash: checksumToBytes(0x668db0af), Next: 88888888}, ErrLocalIncompatibleOrStale},
+
+		// Local is mainnet Byzantium. Remote is also in Byzantium, but announces Gopherium (non existing
+		// fork) at block 7279999, before Petersburg. Local is incompatible.
+		{7279999, ID{Hash: checksumToBytes(0xa00bc324), Next: 7279999}, ErrLocalIncompatibleOrStale},
 	}
 	for i, tt := range tests {
 		filter := newFilter(params.MainnetChainConfig, params.MainnetGenesisHash, func() uint64 { return tt.head })