Update reward manager (#417)

* fixed block validation during reward manager deactivation

* add more comments

* Update miner/worker.go

accounts/abi/bind/backends/simulated.go

@@ -112,7 +112,7 @@ func NewSimulatedBackendWithDatabase(database ethdb.Database, alloc core.Genesis
 	genesis := core.Genesis{Config: cpcfg, GasLimit: gasLimit, Alloc: alloc}
 	genesis.MustCommit(database)
 	cacheConfig := &core.CacheConfig{}
-	blockchain, _ := core.NewBlockChain(database, cacheConfig, genesis.Config, dummy.NewFaker(), vm.Config{}, common.Hash{})
+	blockchain, _ := core.NewBlockChain(database, cacheConfig, genesis.Config, dummy.NewCoinbaseFaker(), vm.Config{}, common.Hash{})
 
 	backend := &SimulatedBackend{
 		database:   database,

consensus/dummy/consensus.go

@@ -30,12 +30,11 @@ var (
 	errBaseFeeNil           = errors.New("base fee is nil")
 )
 
-type Mode uint
-
-const (
-	ModeSkipHeader   Mode = 1 // Skip over header verification
-	ModeSkipBlockFee Mode = 2 // Skip block fee verification
-)
+type Mode struct {
+	ModeSkipHeader   bool
+	ModeSkipBlockFee bool
+	ModeSkipCoinbase bool
+}
 
 type (
 	DummyEngine struct {
@@ -47,7 +46,7 @@ type (
 func NewETHFaker() *DummyEngine {
 	return &DummyEngine{
 		clock:         &mockable.Clock{},
-		consensusMode: ModeSkipBlockFee,
+		consensusMode: Mode{ModeSkipBlockFee: true},
 	}
 }
 
@@ -63,15 +62,32 @@ func NewFakerWithClock(clock *mockable.Clock) *DummyEngine {
 	}
 }
 
+func NewFakerWithMode(mode Mode) *DummyEngine {
+	return &DummyEngine{
+		clock:         &mockable.Clock{},
+		consensusMode: mode,
+	}
+}
+
+func NewCoinbaseFaker() *DummyEngine {
+	return &DummyEngine{
+		clock:         &mockable.Clock{},
+		consensusMode: Mode{ModeSkipCoinbase: true},
+	}
+}
+
 func NewFullFaker() *DummyEngine {
 	return &DummyEngine{
 		clock:         &mockable.Clock{},
-		consensusMode: ModeSkipHeader,
+		consensusMode: Mode{ModeSkipHeader: true},
 	}
 }
 
 // verifyCoinbase checks that the coinbase is valid for the given [header] and [parent].
 func (self *DummyEngine) verifyCoinbase(config *params.ChainConfig, header *types.Header, parent *types.Header, chain consensus.ChainHeaderReader) error {
+	if self.consensusMode.ModeSkipCoinbase {
+		return nil
+	}
 	// get the coinbase configured at parent
 	configuredAddressAtParent, isAllowFeeRecipients, err := chain.GetCoinbaseAt(parent)
 	if err != nil {
@@ -101,6 +117,11 @@ func (self *DummyEngine) verifyHeaderGasFields(config *params.ChainConfig, heade
 	if header.GasUsed > header.GasLimit {
 		return fmt.Errorf("invalid gasUsed: have %d, gasLimit %d", header.GasUsed, header.GasLimit)
 	}
+	// We verify the current block by checking the parent fee config
+	// this is because the current block cannot set the fee config for itself
+	// Fee config might depend on the state when precompile is activated
+	// but we don't know the final state while forming the block.
+	// See worker package for more details.
 	feeConfig, _, err := chain.GetFeeConfigAt(parent)
 	if err != nil {
 		return err
@@ -196,12 +217,9 @@ func (self *DummyEngine) verifyHeader(chain consensus.ChainHeaderReader, header
 	if err := self.verifyHeaderGasFields(config, header, parent, chain); err != nil {
 		return err
 	}
-	// Ensure that coinbase is valid if reward manager is enabled
-	// If reward manager is disabled, this will be handled in syntactic verification
-	if config.IsRewardManager(timestamp) {
-		if err := self.verifyCoinbase(config, header, parent, chain); err != nil {
-			return err
-		}
+	// Ensure that coinbase is valid
+	if err := self.verifyCoinbase(config, header, parent, chain); err != nil {
+		return err
 	}
 
 	// Verify the header's timestamp
@@ -226,7 +244,7 @@ func (self *DummyEngine) Author(header *types.Header) (common.Address, error) {
 
 func (self *DummyEngine) VerifyHeader(chain consensus.ChainHeaderReader, header *types.Header) error {
 	// If we're running a full engine faking, accept any input as valid
-	if self.consensusMode == ModeSkipHeader {
+	if self.consensusMode.ModeSkipHeader {
 		return nil
 	}
 	// Short circuit if the header is known, or it's parent not
@@ -260,7 +278,7 @@ func (self *DummyEngine) verifyBlockFee(
 	txs []*types.Transaction,
 	receipts []*types.Receipt,
 ) error {
-	if self.consensusMode == ModeSkipBlockFee {
+	if self.consensusMode.ModeSkipBlockFee {
 		return nil
 	}
 	if baseFee == nil || baseFee.Sign() <= 0 {
@@ -315,6 +333,8 @@ func (self *DummyEngine) verifyBlockFee(
 
 func (self *DummyEngine) Finalize(chain consensus.ChainHeaderReader, block *types.Block, parent *types.Header, state *state.StateDB, receipts []*types.Receipt) error {
 	if chain.Config().IsSubnetEVM(new(big.Int).SetUint64(block.Time())) {
+		// we use the parent to determine the fee config
+		// since the current block has not been finalized yet.
 		feeConfig, _, err := chain.GetFeeConfigAt(parent)
 		if err != nil {
 			return err
@@ -352,6 +372,8 @@ func (self *DummyEngine) FinalizeAndAssemble(chain consensus.ChainHeaderReader,
 	uncles []*types.Header, receipts []*types.Receipt,
 ) (*types.Block, error) {
 	if chain.Config().IsSubnetEVM(new(big.Int).SetUint64(header.Time)) {
+		// we use the parent to determine the fee config
+		// since the current block has not been finalized yet.
 		feeConfig, _, err := chain.GetFeeConfigAt(parent)
 		if err != nil {
 			return nil, err

core/blockchain_reader.go

@@ -381,7 +381,9 @@ func (bc *BlockChain) GetFeeConfigAt(parent *types.Header) (commontype.FeeConfig
 	return storedFeeConfig, lastChangedAt, nil
 }
 
-// GetCoinbaseAt returns the configured coinbase address at [parent]. If fee recipients are allowed, returns true in the second return value.
+// GetCoinbaseAt returns the configured coinbase address at [parent].
+// If RewardManager is activated at [parent], returns the reward manager config in the precompile contract state.
+// If fee recipients are allowed, returns true in the second return value.
 func (bc *BlockChain) GetCoinbaseAt(parent *types.Header) (common.Address, bool, error) {
 	config := bc.Config()
 	bigTime := new(big.Int).SetUint64(parent.Time)

core/blockchain_test.go

@@ -57,7 +57,7 @@ func createBlockChain(
 		db,
 		cacheConfig,
 		chainConfig,
-		dummy.NewFaker(),
+		dummy.NewCoinbaseFaker(),
 		vm.Config{},
 		lastAcceptedHash,
 	)
@@ -375,9 +375,10 @@ func TestBlockChainOfflinePruningUngracefulShutdown(t *testing.T) {
 		return createBlockChain(db, pruningConfig, chainConfig, lastAcceptedHash)
 	}
 	for _, tt := range tests {
+		tst := tt
 		t.Run(tt.Name, func(t *testing.T) {
 			t.Parallel()
-			tt.testFunc(t, create)
+			tst.testFunc(t, create)
 		})
 	}
 }
@@ -677,7 +678,7 @@ func TestCanonicalHashMarker(t *testing.T) {
 				BaseFee: big.NewInt(params.TestInitialBaseFee),
 			}
 			genesis = gspec.MustCommit(db)
-			engine  = dummy.NewFaker()
+			engine  = dummy.NewCoinbaseFaker()
 		)
 		forkA, _, err := GenerateChain(params.TestChainConfig, genesis, engine, db, c.forkA, 10, func(i int, gen *BlockGen) {})
 		if err != nil {

core/chain_makers_test.go

@@ -61,7 +61,7 @@ func ExampleGenerateChain() {
 	// each block and adds different features to gen based on the
 	// block index.
 	signer := types.HomesteadSigner{}
-	chain, _, err := GenerateChain(gspec.Config, genesis, dummy.NewFaker(), db, 3, 10, func(i int, gen *BlockGen) {
+	chain, _, err := GenerateChain(gspec.Config, genesis, dummy.NewCoinbaseFaker(), db, 3, 10, func(i int, gen *BlockGen) {
 		switch i {
 		case 0:
 			// In block 1, addr1 sends addr2 some ether.
@@ -82,7 +82,7 @@ func ExampleGenerateChain() {
 	}
 
 	// Import the chain. This runs all block validation rules.
-	blockchain, _ := NewBlockChain(db, DefaultCacheConfig, gspec.Config, dummy.NewFaker(), vm.Config{}, common.Hash{})
+	blockchain, _ := NewBlockChain(db, DefaultCacheConfig, gspec.Config, dummy.NewCoinbaseFaker(), vm.Config{}, common.Hash{})
 	defer blockchain.Stop()
 
 	if i, err := blockchain.InsertChain(chain); err != nil {

core/headerchain_test.go

@@ -80,16 +80,16 @@ func TestHeaderInsertion(t *testing.T) {
 			Config:  params.TestChainConfig,
 		}).MustCommit(db)
 	)
-	chain, err := NewBlockChain(db, DefaultCacheConfig, params.TestChainConfig, dummy.NewFaker(), vm.Config{}, common.Hash{})
+	chain, err := NewBlockChain(db, DefaultCacheConfig, params.TestChainConfig, dummy.NewCoinbaseFaker(), vm.Config{}, common.Hash{})
 	if err != nil {
 		t.Fatal(err)
 	}
 	// chain A: G->A1->A2...A128
-	chainA, _, _ := GenerateChain(params.TestChainConfig, types.NewBlockWithHeader(genesis.Header()), dummy.NewFaker(), db, 128, 10, func(i int, b *BlockGen) {
+	chainA, _, _ := GenerateChain(params.TestChainConfig, types.NewBlockWithHeader(genesis.Header()), dummy.NewCoinbaseFaker(), db, 128, 10, func(i int, b *BlockGen) {
 		b.SetCoinbase(common.Address{0: byte(10), 19: byte(i)})
 	})
 	// chain B: G->A1->B2...B128
-	chainB, _, _ := GenerateChain(params.TestChainConfig, types.NewBlockWithHeader(chainA[0].Header()), dummy.NewFaker(), db, 128, 10, func(i int, b *BlockGen) {
+	chainB, _, _ := GenerateChain(params.TestChainConfig, types.NewBlockWithHeader(chainA[0].Header()), dummy.NewCoinbaseFaker(), db, 128, 10, func(i int, b *BlockGen) {
 		b.SetCoinbase(common.Address{0: byte(10), 19: byte(i)})
 	})
 	log.Root().SetHandler(log.StdoutHandler)

core/state_processor_test.go

@@ -86,7 +86,7 @@ func TestStateProcessorErrors(t *testing.T) {
 				GasLimit: params.TestChainConfig.FeeConfig.GasLimit.Uint64(),
 			}
 			genesis       = gspec.MustCommit(db)
-			blockchain, _ = NewBlockChain(db, DefaultCacheConfig, gspec.Config, dummy.NewFaker(), vm.Config{}, common.Hash{})
+			blockchain, _ = NewBlockChain(db, DefaultCacheConfig, gspec.Config, dummy.NewCoinbaseFaker(), vm.Config{}, common.Hash{})
 		)
 		defer blockchain.Stop()
 		bigNumber := new(big.Int).SetBytes(common.FromHex("0xffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffffff"))
@@ -185,7 +185,7 @@ func TestStateProcessorErrors(t *testing.T) {
 				want: "could not apply tx 0 [0xd82a0c2519acfeac9a948258c47e784acd20651d9d80f9a1c67b4137651c3a24]: insufficient funds for gas * price + value: address 0x71562b71999873DB5b286dF957af199Ec94617F7 have 2000000000000000000 want 2431633873983640103894990685182446064918669677978451844828609264166175722438635000",
 			},
 		} {
-			block := GenerateBadBlock(genesis, dummy.NewFaker(), tt.txs, gspec.Config)
+			block := GenerateBadBlock(genesis, dummy.NewCoinbaseFaker(), tt.txs, gspec.Config)
 			_, err := blockchain.InsertChain(types.Blocks{block})
 			if err == nil {
 				t.Fatal("block imported without errors")
@@ -224,7 +224,7 @@ func TestStateProcessorErrors(t *testing.T) {
 				GasLimit: params.TestChainConfig.FeeConfig.GasLimit.Uint64(),
 			}
 			genesis       = gspec.MustCommit(db)
-			blockchain, _ = NewBlockChain(db, DefaultCacheConfig, gspec.Config, dummy.NewFaker(), vm.Config{}, common.Hash{})
+			blockchain, _ = NewBlockChain(db, DefaultCacheConfig, gspec.Config, dummy.NewCoinbaseFaker(), vm.Config{}, common.Hash{})
 		)
 		defer blockchain.Stop()
 		for i, tt := range []struct {
@@ -238,7 +238,7 @@ func TestStateProcessorErrors(t *testing.T) {
 				want: "could not apply tx 0 [0x88626ac0d53cb65308f2416103c62bb1f18b805573d4f96a3640bbbfff13c14f]: transaction type not supported",
 			},
 		} {
-			block := GenerateBadBlock(genesis, dummy.NewFaker(), tt.txs, gspec.Config)
+			block := GenerateBadBlock(genesis, dummy.NewCoinbaseFaker(), tt.txs, gspec.Config)
 			_, err := blockchain.InsertChain(types.Blocks{block})
 			if err == nil {
 				t.Fatal("block imported without errors")
@@ -265,7 +265,7 @@ func TestStateProcessorErrors(t *testing.T) {
 				GasLimit: params.TestChainConfig.FeeConfig.GasLimit.Uint64(),
 			}
 			genesis       = gspec.MustCommit(db)
-			blockchain, _ = NewBlockChain(db, DefaultCacheConfig, gspec.Config, dummy.NewFaker(), vm.Config{}, common.Hash{})
+			blockchain, _ = NewBlockChain(db, DefaultCacheConfig, gspec.Config, dummy.NewCoinbaseFaker(), vm.Config{}, common.Hash{})
 		)
 		defer blockchain.Stop()
 		for i, tt := range []struct {
@@ -279,7 +279,7 @@ func TestStateProcessorErrors(t *testing.T) {
 				want: "could not apply tx 0 [0x88626ac0d53cb65308f2416103c62bb1f18b805573d4f96a3640bbbfff13c14f]: sender not an eoa: address 0x71562b71999873DB5b286dF957af199Ec94617F7, codehash: 0x9280914443471259d4570a8661015ae4a5b80186dbc619658fb494bebc3da3d1",
 			},
 		} {
-			block := GenerateBadBlock(genesis, dummy.NewFaker(), tt.txs, gspec.Config)
+			block := GenerateBadBlock(genesis, dummy.NewCoinbaseFaker(), tt.txs, gspec.Config)
 			_, err := blockchain.InsertChain(types.Blocks{block})
 			if err == nil {
 				t.Fatal("block imported without errors")
@@ -334,7 +334,7 @@ func TestBadTxAllowListBlock(t *testing.T) {
 			GasLimit: params.TestChainConfig.FeeConfig.GasLimit.Uint64(),
 		}
 		genesis       = gspec.MustCommit(db)
-		blockchain, _ = NewBlockChain(db, DefaultCacheConfig, gspec.Config, dummy.NewFaker(), vm.Config{}, common.Hash{})
+		blockchain, _ = NewBlockChain(db, DefaultCacheConfig, gspec.Config, dummy.NewCoinbaseFaker(), vm.Config{}, common.Hash{})
 	)
 
 	mkDynamicTx := func(nonce uint64, to common.Address, gasLimit uint64, gasTipCap, gasFeeCap *big.Int) *types.Transaction {
@@ -361,7 +361,7 @@ func TestBadTxAllowListBlock(t *testing.T) {
 			want: "could not apply tx 0 [0xc5725e8baac950b2925dd4fea446ccddead1cc0affdae18b31a7d910629d9225]: cannot issue transaction from non-allow listed address: 0x71562b71999873DB5b286dF957af199Ec94617F7",
 		},
 	} {
-		block := GenerateBadBlock(genesis, dummy.NewFaker(), tt.txs, gspec.Config)
+		block := GenerateBadBlock(genesis, dummy.NewCoinbaseFaker(), tt.txs, gspec.Config)
 		_, err := blockchain.InsertChain(types.Blocks{block})
 		if err == nil {
 			t.Fatal("block imported without errors")

core/tx_pool.go

@@ -1441,6 +1441,10 @@ func (pool *TxPool) reset(oldHead, newHead *types.Header) {
 
 	// when we reset txPool we should explicitly check if fee struct for min base fee has changed
 	// so that we can correctly drop txs with < minBaseFee from tx pool.
+	// TODO: this should be checking IsSubnetEVM since we also support minimumFee for SubnetEVM
+	// without requiring FeeConfigManager is enabled.
+	// This is already being set by SetMinFee when gas price updater starts.
+	// However tests are currently failing if we change this check to IsSubnetEVM.
 	if pool.chainconfig.IsFeeConfigManager(new(big.Int).SetUint64(newHead.Time)) {
 		feeConfig, _, err := pool.chain.GetFeeConfigAt(newHead)
 		if err != nil {

eth/tracers/api_test.go

@@ -71,7 +71,7 @@ type testBackend struct {
 func newTestBackend(t *testing.T, n int, gspec *core.Genesis, generator func(i int, b *core.BlockGen)) *testBackend {
 	backend := &testBackend{
 		chainConfig: params.TestChainConfig,
-		engine:      dummy.NewETHFaker(),
+		engine:      dummy.NewFakerWithMode(dummy.Mode{ModeSkipBlockFee: true, ModeSkipCoinbase: true}),
 		chaindb:     rawdb.NewMemoryDatabase(),
 	}
 	// Generate blocks for testing

miner/worker.go

@@ -117,22 +117,25 @@ func (w *worker) commitNewWork() (*types.Block, error) {
 	defer w.mu.RUnlock()
 
 	tstart := w.clock.Time()
-	timestamp := tstart.Unix()
+	timestamp := uint64(tstart.Unix())
 	parent := w.chain.CurrentBlock()
 	// Note: in order to support asynchronous block production, blocks are allowed to have
 	// the same timestamp as their parent. This allows more than one block to be produced
 	// per second.
-	if parent.Time() >= uint64(timestamp) {
-		timestamp = int64(parent.Time())
+	if parent.Time() >= timestamp {
+		timestamp = parent.Time()
 	}
 
+	bigTimestamp := new(big.Int).SetUint64(timestamp)
 	var gasLimit uint64
+	// The fee config manager relies on the state of the parent block to set the fee config
+	// because the fee config may be changed by the current block.
 	feeConfig, _, err := w.chain.GetFeeConfigAt(parent.Header())
 	if err != nil {
 		return nil, err
 	}
 	configuredGasLimit := feeConfig.GasLimit.Uint64()
-	if w.chainConfig.IsSubnetEVM(big.NewInt(timestamp)) {
+	if w.chainConfig.IsSubnetEVM(bigTimestamp) {
 		gasLimit = configuredGasLimit
 	} else {
 		// The gas limit is set in SubnetEVMGasLimit because the ceiling and floor were set to the same value
@@ -145,13 +148,12 @@ func (w *worker) commitNewWork() (*types.Block, error) {
 		Number:     num.Add(num, common.Big1),
 		GasLimit:   gasLimit,
 		Extra:      nil,
-		Time:       uint64(timestamp),
+		Time:       timestamp,
 	}
 
-	bigTimestamp := big.NewInt(timestamp)
 	if w.chainConfig.IsSubnetEVM(bigTimestamp) {
 		var err error
-		header.Extra, header.BaseFee, err = dummy.CalcBaseFee(w.chainConfig, feeConfig, parent.Header(), uint64(timestamp))
+		header.Extra, header.BaseFee, err = dummy.CalcBaseFee(w.chainConfig, feeConfig, parent.Header(), timestamp)
 		if err != nil {
 			return nil, fmt.Errorf("failed to calculate new base fee: %w", err)
 		}

plugin/evm/block_verification.go

@@ -9,11 +9,9 @@ import (
 
 	"github.com/ethereum/go-ethereum/common"
 
-	"github.com/ava-labs/subnet-evm/constants"
 	"github.com/ava-labs/subnet-evm/core/types"
 	"github.com/ava-labs/subnet-evm/params"
 	"github.com/ava-labs/subnet-evm/trie"
-	"github.com/ava-labs/subnet-evm/vmerrs"
 )
 
 var legacyMinGasPrice = big.NewInt(params.MinGasPrice)
@@ -97,17 +95,6 @@ func (v blockValidator) SyntacticVerify(b *Block, rules params.Rules) error {
 		return fmt.Errorf("invalid uncle hash %v does not match calculated uncle hash %v", ethHeader.UncleHash, uncleHash)
 	}
 
-	switch {
-	case rules.IsRewardManagerEnabled:
-		// if reward manager is enabled, Coinbase depends on state.
-		// State is not available here, so skip checking the coinbase here.
-	case rules.IsSubnetEVM && b.vm.chainConfig.AllowFeeRecipients:
-		// If Subnet EVM and AllowFeeRecipients is enabled we don't check the coinbase.
-	case b.ethBlock.Coinbase() != constants.BlackholeAddr:
-		// If Subnet EVM or AllowFeeRecipients is not enabled, Coinbase must be BlackholeAddr.
-		return fmt.Errorf("%w: %v does not match required blackhole address %v", vmerrs.ErrInvalidCoinbase, ethHeader.Coinbase, constants.BlackholeAddr)
-	}
-
 	// Block must not have any uncles
 	if len(b.ethBlock.Uncles()) > 0 {
 		return errUnclesUnsupported