diff --git a/core/forkid/forkid.go b/core/forkid/forkid.go index 3845742ad..28e2452fb 100644 --- a/core/forkid/forkid.go +++ b/core/forkid/forkid.go @@ -120,10 +120,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. @@ -145,7 +148,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 diff --git a/core/forkid/forkid_test.go b/core/forkid/forkid_test.go index 304894023..ca7e6c3cb 100644 --- a/core/forkid/forkid_test.go +++ b/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 })