From 4bb25042eb957662155079e06bb2efbdd866eb99 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?P=C3=A9ter=20Szil=C3=A1gyi?= Date: Mon, 10 Sep 2018 17:12:39 +0300 Subject: [PATCH] consensus/clique, core: chain maker clique + error tests --- consensus/clique/clique.go | 49 +++++--- consensus/clique/snapshot.go | 16 +-- consensus/clique/snapshot_test.go | 194 ++++++++++++++++++++++-------- core/chain_makers.go | 10 +- miner/worker_test.go | 1 + 5 files changed, 195 insertions(+), 75 deletions(-) diff --git a/consensus/clique/clique.go b/consensus/clique/clique.go index 0ff72e55c..2b69141c1 100644 --- a/consensus/clique/clique.go +++ b/consensus/clique/clique.go @@ -93,27 +93,33 @@ var ( // errMissingSignature is returned if a block's extra-data section doesn't seem // to contain a 65 byte secp256k1 signature. - errMissingSignature = errors.New("extra-data 65 byte suffix signature missing") + errMissingSignature = errors.New("extra-data 65 byte signature suffix missing") // errExtraSigners is returned if non-checkpoint block contain signer data in // their extra-data fields. errExtraSigners = errors.New("non-checkpoint block contains extra signer list") // errInvalidCheckpointSigners is returned if a checkpoint block contains an - // invalid list of signers (i.e. non divisible by 20 bytes, or not the correct - // ones). + // invalid list of signers (i.e. non divisible by 20 bytes). errInvalidCheckpointSigners = errors.New("invalid signer list on checkpoint block") + // errMismatchingCheckpointSigners is returned if a checkpoint block contains a + // list of signers different than the one the local node calculated. + errMismatchingCheckpointSigners = errors.New("mismatching signer list on checkpoint block") + // errInvalidMixDigest is returned if a block's mix digest is non-zero. errInvalidMixDigest = errors.New("non-zero mix digest") // errInvalidUncleHash is returned if a block contains an non-empty uncle list. errInvalidUncleHash = errors.New("non empty uncle hash") - // errInvalidDifficulty is returned if the difficulty of a block is not either - // of 1 or 2, or if the value does not match the turn of the signer. + // errInvalidDifficulty is returned if the difficulty of a block neither 1 or 2. errInvalidDifficulty = errors.New("invalid difficulty") + // errWrongDifficulty is returned if the difficulty of a block doesn't match the + // turn of the signer. + errWrongDifficulty = errors.New("wrong difficulty") + // ErrInvalidTimestamp is returned if the timestamp of a block is lower than // the previous block's timestamp + the minimum block period. ErrInvalidTimestamp = errors.New("invalid timestamp") @@ -122,8 +128,12 @@ var ( // be modified via out-of-range or non-contiguous headers. errInvalidVotingChain = errors.New("invalid voting chain") - // errUnauthorized is returned if a header is signed by a non-authorized entity. - errUnauthorized = errors.New("unauthorized") + // errUnauthorizedSigner is returned if a header is signed by a non-authorized entity. + errUnauthorizedSigner = errors.New("unauthorized signer") + + // errRecentlySigned is returned if a header is signed by an authorized entity + // that already signed a header recently, thus is temporarily not allowed to. + errRecentlySigned = errors.New("recently signed") // errWaitTransactions is returned if an empty block is attempted to be sealed // on an instant chain (0 second period). It's important to refuse these as the @@ -205,6 +215,9 @@ type Clique struct { signer common.Address // Ethereum address of the signing key signFn SignerFn // Signer function to authorize hashes with lock sync.RWMutex // Protects the signer fields + + // The fields below are for testing only + fakeDiff bool // Skip difficulty verifications } // New creates a Clique proof-of-authority consensus engine with the initial @@ -359,7 +372,7 @@ func (c *Clique) verifyCascadingFields(chain consensus.ChainReader, header *type } extraSuffix := len(header.Extra) - extraSeal if !bytes.Equal(header.Extra[extraVanity:extraSuffix], signers) { - return errInvalidCheckpointSigners + return errMismatchingCheckpointSigners } } // All basic checks passed, verify the seal and return @@ -481,23 +494,25 @@ func (c *Clique) verifySeal(chain consensus.ChainReader, header *types.Header, p return err } if _, ok := snap.Signers[signer]; !ok { - return errUnauthorized + return errUnauthorizedSigner } for seen, recent := range snap.Recents { if recent == signer { // Signer is among recents, only fail if the current block doesn't shift it out if limit := uint64(len(snap.Signers)/2 + 1); seen > number-limit { - return errUnauthorized + return errRecentlySigned } } } // Ensure that the difficulty corresponds to the turn-ness of the signer - inturn := snap.inturn(header.Number.Uint64(), signer) - if inturn && header.Difficulty.Cmp(diffInTurn) != 0 { - return errInvalidDifficulty - } - if !inturn && header.Difficulty.Cmp(diffNoTurn) != 0 { - return errInvalidDifficulty + if !c.fakeDiff { + inturn := snap.inturn(header.Number.Uint64(), signer) + if inturn && header.Difficulty.Cmp(diffInTurn) != 0 { + return errWrongDifficulty + } + if !inturn && header.Difficulty.Cmp(diffNoTurn) != 0 { + return errWrongDifficulty + } } return nil } @@ -613,7 +628,7 @@ func (c *Clique) Seal(chain consensus.ChainReader, block *types.Block, results c return err } if _, authorized := snap.Signers[signer]; !authorized { - return errUnauthorized + return errUnauthorizedSigner } // If we're amongst the recent signers, wait for the next block for seen, recent := range snap.Recents { diff --git a/consensus/clique/snapshot.go b/consensus/clique/snapshot.go index 2333d6924..54d4555f3 100644 --- a/consensus/clique/snapshot.go +++ b/consensus/clique/snapshot.go @@ -57,12 +57,12 @@ type Snapshot struct { Tally map[common.Address]Tally `json:"tally"` // Current vote tally to avoid recalculating } -// signers implements the sort interface to allow sorting a list of addresses -type signers []common.Address +// signersAscending implements the sort interface to allow sorting a list of addresses +type signersAscending []common.Address -func (s signers) Len() int { return len(s) } -func (s signers) Less(i, j int) bool { return bytes.Compare(s[i][:], s[j][:]) < 0 } -func (s signers) Swap(i, j int) { s[i], s[j] = s[j], s[i] } +func (s signersAscending) Len() int { return len(s) } +func (s signersAscending) Less(i, j int) bool { return bytes.Compare(s[i][:], s[j][:]) < 0 } +func (s signersAscending) Swap(i, j int) { s[i], s[j] = s[j], s[i] } // newSnapshot creates a new snapshot with the specified startup parameters. This // method does not initialize the set of recent signers, so only ever use if for @@ -214,11 +214,11 @@ func (s *Snapshot) apply(headers []*types.Header) (*Snapshot, error) { return nil, err } if _, ok := snap.Signers[signer]; !ok { - return nil, errUnauthorized + return nil, errUnauthorizedSigner } for _, recent := range snap.Recents { if recent == signer { - return nil, errUnauthorized + return nil, errRecentlySigned } } snap.Recents[number] = signer @@ -298,7 +298,7 @@ func (s *Snapshot) signers() []common.Address { for sig := range s.Signers { sigs = append(sigs, sig) } - sort.Sort(signers(sigs)) + sort.Sort(signersAscending(sigs)) return sigs } diff --git a/consensus/clique/snapshot_test.go b/consensus/clique/snapshot_test.go index 17719884f..71fe7ce8b 100644 --- a/consensus/clique/snapshot_test.go +++ b/consensus/clique/snapshot_test.go @@ -19,24 +19,18 @@ package clique import ( "bytes" "crypto/ecdsa" - "math/big" + "sort" "testing" "github.com/ethereum/go-ethereum/common" "github.com/ethereum/go-ethereum/core" - "github.com/ethereum/go-ethereum/core/rawdb" "github.com/ethereum/go-ethereum/core/types" + "github.com/ethereum/go-ethereum/core/vm" "github.com/ethereum/go-ethereum/crypto" "github.com/ethereum/go-ethereum/ethdb" "github.com/ethereum/go-ethereum/params" ) -type testerVote struct { - signer string - voted string - auth bool -} - // testerAccountPool is a pool to maintain currently active tester accounts, // mapped from textual names used in the tests below to actual Ethereum private // keys capable of signing transactions. @@ -50,17 +44,26 @@ func newTesterAccountPool() *testerAccountPool { } } -func (ap *testerAccountPool) sign(header *types.Header, signer string) { - // Ensure we have a persistent key for the signer - if ap.accounts[signer] == nil { - ap.accounts[signer], _ = crypto.GenerateKey() +// checkpoint creates a Clique checkpoint signer section from the provided list +// of authorized signers and embeds it into the provided header. +func (ap *testerAccountPool) checkpoint(header *types.Header, signers []string) { + auths := make([]common.Address, len(signers)) + for i, signer := range signers { + auths[i] = ap.address(signer) + } + sort.Sort(signersAscending(auths)) + for i, auth := range auths { + copy(header.Extra[extraVanity+i*common.AddressLength:], auth.Bytes()) } - // Sign the header and embed the signature in extra data - sig, _ := crypto.Sign(sigHash(header).Bytes(), ap.accounts[signer]) - copy(header.Extra[len(header.Extra)-65:], sig) } +// address retrieves the Ethereum address of a tester account by label, creating +// a new account if no previous one exists yet. func (ap *testerAccountPool) address(account string) common.Address { + // Return the zero account for non-addresses + if account == "" { + return common.Address{} + } // Ensure we have a persistent key for the account if ap.accounts[account] == nil { ap.accounts[account], _ = crypto.GenerateKey() @@ -69,32 +72,38 @@ func (ap *testerAccountPool) address(account string) common.Address { return crypto.PubkeyToAddress(ap.accounts[account].PublicKey) } -// testerChainReader implements consensus.ChainReader to access the genesis -// block. All other methods and requests will panic. -type testerChainReader struct { - db ethdb.Database -} - -func (r *testerChainReader) Config() *params.ChainConfig { return params.AllCliqueProtocolChanges } -func (r *testerChainReader) CurrentHeader() *types.Header { panic("not supported") } -func (r *testerChainReader) GetHeader(common.Hash, uint64) *types.Header { panic("not supported") } -func (r *testerChainReader) GetBlock(common.Hash, uint64) *types.Block { panic("not supported") } -func (r *testerChainReader) GetHeaderByHash(common.Hash) *types.Header { panic("not supported") } -func (r *testerChainReader) GetHeaderByNumber(number uint64) *types.Header { - if number == 0 { - return rawdb.ReadHeader(r.db, rawdb.ReadCanonicalHash(r.db, 0), 0) +// sign calculates a Clique digital signature for the given block and embeds it +// back into the header. +func (ap *testerAccountPool) sign(header *types.Header, signer string) { + // Ensure we have a persistent key for the signer + if ap.accounts[signer] == nil { + ap.accounts[signer], _ = crypto.GenerateKey() } - return nil + // Sign the header and embed the signature in extra data + sig, _ := crypto.Sign(sigHash(header).Bytes(), ap.accounts[signer]) + copy(header.Extra[len(header.Extra)-extraSeal:], sig) } -// Tests that voting is evaluated correctly for various simple and complex scenarios. -func TestVoting(t *testing.T) { +// testerVote represents a single block signed by a parcitular account, where +// the account may or may not have cast a Clique vote. +type testerVote struct { + signer string + voted string + auth bool + checkpoint []string + newbatch bool +} + +// Tests that Clique signer voting is evaluated correctly for various simple and +// complex scenarios, as well as that a few special corner cases fail correctly. +func TestClique(t *testing.T) { // Define the various voting scenarios to test tests := []struct { epoch uint64 signers []string votes []testerVote results []string + failure error }{ { // Single signer, no votes cast @@ -322,10 +331,49 @@ func TestVoting(t *testing.T) { votes: []testerVote{ {signer: "A", voted: "C", auth: true}, {signer: "B"}, - {signer: "A"}, // Checkpoint block, (don't vote here, it's validated outside of snapshots) + {signer: "A", checkpoint: []string{"A", "B"}}, {signer: "B", voted: "C", auth: true}, }, results: []string{"A", "B"}, + }, { + // An unauthorized signer should not be able to sign blocks + signers: []string{"A"}, + votes: []testerVote{ + {signer: "B"}, + }, + failure: errUnauthorizedSigner, + }, { + // An authorized signer that signed recenty should not be able to sign again + signers: []string{"A", "B"}, + votes: []testerVote{ + {signer: "A"}, + {signer: "A"}, + }, + failure: errRecentlySigned, + }, { + // Recent signatures should not reset on checkpoint blocks imported in a batch + epoch: 3, + signers: []string{"A", "B", "C"}, + votes: []testerVote{ + {signer: "A"}, + {signer: "B"}, + {signer: "A", checkpoint: []string{"A", "B", "C"}}, + {signer: "A"}, + }, + failure: errRecentlySigned, + }, { + // Recent signatures should not reset on checkpoint blocks imported in a new + // batch (https://github.com/ethereum/go-ethereum/issues/17593). Whilst this + // seems overly specific and weird, it was a Rinkeby consensus split. + epoch: 3, + signers: []string{"A", "B", "C"}, + votes: []testerVote{ + {signer: "A"}, + {signer: "B"}, + {signer: "A", checkpoint: []string{"A", "B", "C"}}, + {signer: "A", newbatch: true}, + }, + failure: errRecentlySigned, }, } // Run through the scenarios and test them @@ -356,28 +404,78 @@ func TestVoting(t *testing.T) { genesis.Commit(db) // Assemble a chain of headers from the cast votes - headers := make([]*types.Header, len(tt.votes)) - for j, vote := range tt.votes { - headers[j] = &types.Header{ - Number: big.NewInt(int64(j) + 1), - Time: big.NewInt(int64(j) * 15), - Coinbase: accounts.address(vote.voted), - Extra: make([]byte, extraVanity+extraSeal), + config := *params.TestChainConfig + config.Clique = ¶ms.CliqueConfig{ + Period: 1, + Epoch: tt.epoch, + } + engine := New(config.Clique, db) + engine.fakeDiff = true + + blocks, _ := core.GenerateChain(&config, genesis.ToBlock(db), engine, db, len(tt.votes), func(j int, gen *core.BlockGen) { + // Cast the vote contained in this block + gen.SetCoinbase(accounts.address(tt.votes[j].voted)) + if tt.votes[j].auth { + var nonce types.BlockNonce + copy(nonce[:], nonceAuthVote) + gen.SetNonce(nonce) } + }) + // Iterate through the blocks and seal them individually + for j, block := range blocks { + // Geth the header and prepare it for signing + header := block.Header() if j > 0 { - headers[j].ParentHash = headers[j-1].Hash() + header.ParentHash = blocks[j-1].Hash() } - if vote.auth { - copy(headers[j].Nonce[:], nonceAuthVote) + header.Extra = make([]byte, extraVanity+extraSeal) + if auths := tt.votes[j].checkpoint; auths != nil { + header.Extra = make([]byte, extraVanity+len(auths)*common.AddressLength+extraSeal) + accounts.checkpoint(header, auths) } - accounts.sign(headers[j], vote.signer) + header.Difficulty = diffInTurn // Ignored, we just need a valid number + + // Generate the signature, embed it into the header and the block + accounts.sign(header, tt.votes[j].signer) + blocks[j] = block.WithSeal(header) + } + // Split the blocks up into individual import batches (cornercase testing) + batches := [][]*types.Block{nil} + for j, block := range blocks { + if tt.votes[j].newbatch { + batches = append(batches, nil) + } + batches[len(batches)-1] = append(batches[len(batches)-1], block) } // Pass all the headers through clique and ensure tallying succeeds - head := headers[len(headers)-1] - - snap, err := New(¶ms.CliqueConfig{Epoch: tt.epoch}, db).snapshot(&testerChainReader{db: db}, head.Number.Uint64(), head.Hash(), headers) + chain, err := core.NewBlockChain(db, nil, &config, engine, vm.Config{}) if err != nil { - t.Errorf("test %d: failed to create voting snapshot: %v", i, err) + t.Errorf("test %d: failed to create test chain: %v", i, err) + continue + } + failed := false + for j := 0; j < len(batches)-1; j++ { + if k, err := chain.InsertChain(batches[j]); err != nil { + t.Errorf("test %d: failed to import batch %d, block %d: %v", i, j, k, err) + failed = true + break + } + } + if failed { + continue + } + if _, err = chain.InsertChain(batches[len(batches)-1]); err != tt.failure { + t.Errorf("test %d: failure mismatch: have %v, want %v", i, err, tt.failure) + } + if tt.failure != nil { + continue + } + // No failure was produced or requested, generate the final voting snapshot + head := blocks[len(blocks)-1] + + snap, err := engine.snapshot(chain, head.NumberU64(), head.Hash(), nil) + if err != nil { + t.Errorf("test %d: failed to retrieve voting snapshot: %v", i, err) continue } // Verify the final list of signers against the expected ones diff --git a/core/chain_makers.go b/core/chain_makers.go index de0fc6be9..351673477 100644 --- a/core/chain_makers.go +++ b/core/chain_makers.go @@ -67,6 +67,11 @@ func (b *BlockGen) SetExtra(data []byte) { b.header.Extra = data } +// SetNonce sets the nonce field of the generated block. +func (b *BlockGen) SetNonce(nonce types.BlockNonce) { + b.header.Nonce = nonce +} + // AddTx adds a transaction to the generated block. If no coinbase has // been set, the block's coinbase is set to the zero address. // @@ -190,13 +195,14 @@ func GenerateChain(config *params.ChainConfig, parent *types.Block, engine conse if config.DAOForkSupport && config.DAOForkBlock != nil && config.DAOForkBlock.Cmp(b.header.Number) == 0 { misc.ApplyDAOHardFork(statedb) } - // Execute any user modifications to the block and finalize it + // Execute any user modifications to the block if gen != nil { gen(i, b) } - if b.engine != nil { + // Finalize and seal the block block, _ := b.engine.Finalize(b.chainReader, b.header, statedb, b.txs, b.uncles, b.receipts) + // Write state changes to db root, err := statedb.Commit(config.IsEIP158(b.header.Number)) if err != nil { diff --git a/miner/worker_test.go b/miner/worker_test.go index 6de1177cd..ad10d48ef 100644 --- a/miner/worker_test.go +++ b/miner/worker_test.go @@ -161,6 +161,7 @@ func testPendingStateAndBlock(t *testing.T, chainConfig *params.ChainConfig, eng t.Errorf("account balance mismatch: have %d, want %d", balance, 1000) } b.txPool.AddLocals(newTxs) + // Ensure the new tx events has been processed time.Sleep(100 * time.Millisecond) block, state = w.pending()