fix: qbft issues with block rewards and beneficary

- multiple issues were found with coinbase rewards when running qbft
- fix up issues with MiningBeneficiary

Author: antonydenyer

cmd/utils/flags.go

@@ -2536,7 +2536,6 @@ func MakeChain(ctx *cli.Context, stack *node.Node, useExist bool) (chain *core.B
 		qbftConfig := setBFTConfig(config.QBFT.BFTConfig)
 		qbftConfig.BlockReward = config.QBFT.BlockReward
 		qbftConfig.BeneficiaryMode = config.QBFT.BeneficiaryMode     // beneficiary mode: list, besu, validators
-		qbftConfig.BeneficiaryList = config.QBFT.BeneficiaryList     // list mode
 		qbftConfig.MiningBeneficiary = config.QBFT.MiningBeneficiary // auto (undefined mode) and besu mode
 		qbftConfig.TestQBFTBlock = big.NewInt(0)
 		qbftConfig.Transitions = config.Transitions

consensus/istanbul/config.go

@@ -124,7 +124,6 @@ func (p *ProposerPolicy) ClearRegistry() {
 
 type Config struct {
 	RequestTimeout         uint64                `toml:",omitempty"` // The timeout for each Istanbul round in milliseconds.
-	BlockReward            *math.HexOrDecimal256 `toml:",omitempty"` // Reward
 	BlockPeriod            uint64                `toml:",omitempty"` // Default minimum difference between two consecutive block's timestamps in second
 	EmptyBlockPeriod       uint64                `toml:",omitempty"` // Default minimum difference between a block and empty block's timestamps in second
 	ProposerPolicy         *ProposerPolicy       `toml:",omitempty"` // The policy for proposer selection
@@ -133,13 +132,13 @@ type Config struct {
 	AllowedFutureBlockTime uint64                `toml:",omitempty"` // Max time (in seconds) from current time allowed for blocks, before they're considered future blocks
 	TestQBFTBlock          *big.Int              `toml:",omitempty"` // Fork block at which block confirmations are done using qbft consensus instead of ibft
 	BeneficiaryMode        *string               `toml:",omitempty"` // Mode for setting the beneficiary, either: list, besu, validators (beneficiary list is the list of validators)
-	BeneficiaryList        []common.Address      `toml:",omitempty"` // List of wallet addresses that have benefit at every new block (list mode)
+	BlockReward            *math.HexOrDecimal256 `toml:",omitempty"` // Reward
 	MiningBeneficiary      *common.Address       `toml:",omitempty"` // Wallet address that benefits at every new block (besu mode)
+	ValidatorContract      common.Address        `toml:",omitempty"`
+	Validators             []common.Address      `toml:",omitempty"`
+	ValidatorSelectionMode *string               `toml:",omitempty"`
+	Client                 bind.ContractCaller   `toml:",omitempty"`
 	Transitions            []params.Transition
-	ValidatorContract      common.Address      `toml:",omitempty"`
-	Validators             []common.Address    `toml:",omitempty"`
-	ValidatorSelectionMode *string             `toml:",omitempty"`
-	Client                 bind.ContractCaller `toml:",omitempty"`
 }
 
 var DefaultConfig = &Config{
@@ -201,13 +200,10 @@ func (c Config) GetConfig(blockNumber *big.Int) Config {
 		if transition.EmptyBlockPeriodSeconds != nil {
 			newConfig.EmptyBlockPeriod = *transition.EmptyBlockPeriodSeconds
 		}
-		if transition.BeneficiaryMode != nil { // besu mode
+		if transition.BeneficiaryMode != nil {
 			newConfig.BeneficiaryMode = transition.BeneficiaryMode
 		}
-		if transition.BeneficiaryList != nil { // list mode
-			newConfig.BeneficiaryList = transition.BeneficiaryList
-		}
-		if transition.BlockReward != nil { // besu mode
+		if transition.BlockReward != nil {
 			newConfig.BlockReward = transition.BlockReward
 		}
 		if transition.MiningBeneficiary != nil {

consensus/istanbul/qbft/engine/engine.go

@@ -4,7 +4,6 @@ import (
 	"bytes"
 	"fmt"
 	"math/big"
-	"strings"
 	"time"
 
 	"github.com/ethereum/go-ethereum/accounts/abi/bind"
@@ -383,7 +382,7 @@ func WriteValidators(validators []common.Address) ApplyQBFTExtra {
 // consensus rules that happen at finalization (e.g. block rewards).
 func (e *Engine) Finalize(chain consensus.ChainHeaderReader, header *types.Header, state *state.StateDB, txs []*types.Transaction, uncles []*types.Header) {
 	// Accumulate any block and uncle rewards and commit the final state root
-	e.accumulateRewards(chain, state, header, uncles, e.cfg.GetConfig(header.Number))
+	e.accumulateRewards(chain, state, header)
 	header.Root = state.IntermediateRoot(chain.Config().IsEIP158(header.Number))
 	header.UncleHash = nilUncleHash
 }
@@ -586,41 +585,17 @@ func (e *Engine) validatorsList(genesis *types.Header, config istanbul.Config) (
 }
 
 // AccumulateRewards credits the beneficiary of the given block with a reward.
-func (e *Engine) accumulateRewards(chain consensus.ChainHeaderReader, state *state.StateDB, header *types.Header, uncles []*types.Header, cfg istanbul.Config) {
-	blockReward := cfg.BlockReward
-	if blockReward == nil {
-		return // no reward
-	}
-	// Accumulate the rewards for a beneficiary
-	reward := big.Int(*blockReward)
-	if cfg.BeneficiaryMode == nil || *cfg.BeneficiaryMode == "" {
-		if cfg.MiningBeneficiary != nil {
-			state.AddBalance(*cfg.MiningBeneficiary, &reward) // implicit besu compatible mode
-		}
-		return
-	}
-	switch strings.ToLower(*cfg.BeneficiaryMode) {
-	case "fixed":
-		if cfg.MiningBeneficiary != nil {
-			state.AddBalance(*cfg.MiningBeneficiary, &reward)
-		}
-	case "list":
-		for _, b := range cfg.BeneficiaryList {
-			state.AddBalance(b, &reward)
-		}
-	case "validators":
-		genesis := chain.GetHeaderByNumber(0)
-		if err := e.VerifyHeader(chain, genesis, nil, nil); err != nil {
-			log.Error("BFT: invalid genesis block", "err", err)
-			return
-		}
-		list, err := e.validatorsList(genesis, cfg)
-		if err != nil {
-			log.Error("get validators list", "err", err)
-			return
-		}
-		for _, b := range list {
-			state.AddBalance(b, &reward)
+func (e *Engine) accumulateRewards(chain consensus.ChainHeaderReader, state *state.StateDB, header *types.Header) {
+
+	blockReward := chain.Config().GetBlockReward(header.Number)
+	if blockReward.Cmp(big.NewInt(0)) > 0 {
+		coinbase := header.Coinbase
+		if (coinbase == common.Address{}) {
+			coinbase = e.signer
 		}
+		rewardAccount, _ := chain.Config().GetRewardAccount(header.Number, coinbase)
+		log.Trace("QBFT: accumulate rewards to", "rewardAccount", rewardAccount, "blockReward", blockReward)
+
+		state.AddBalance(rewardAccount, &blockReward)
 	}
 }

consensus/istanbul/qbft/engine/engine_test.go

@@ -2,22 +2,14 @@ package qbftengine
 
 import (
 	"bytes"
-	"fmt"
 	"math/big"
 	"reflect"
 	"testing"
 
 	"github.com/ethereum/go-ethereum/common"
 	"github.com/ethereum/go-ethereum/common/hexutil"
-	"github.com/ethereum/go-ethereum/common/math"
-	"github.com/ethereum/go-ethereum/consensus/istanbul"
 	istanbulcommon "github.com/ethereum/go-ethereum/consensus/istanbul/common"
-	"github.com/ethereum/go-ethereum/core"
-	"github.com/ethereum/go-ethereum/core/rawdb"
-	"github.com/ethereum/go-ethereum/core/state"
 	"github.com/ethereum/go-ethereum/core/types"
-	"github.com/stretchr/testify/assert"
-	"github.com/stretchr/testify/require"
 )
 
 func TestPrepareExtra(t *testing.T) {
@@ -167,124 +159,3 @@ func TestWriteValidatorVote(t *testing.T) {
 		t.Errorf("extra data mismatch: have %v, want %v", istExtra, expectedIstExtra)
 	}
 }
-
-func TestAccumulateRewards(t *testing.T) {
-	addr := common.HexToAddress("0xed9d02e382b34818e88b88a309c7fe71e65f419d")
-	fixedMode := "fixed"
-	listMode := "list"
-	validatorsMode := "validators"
-	emptyMode := ""
-	dummyMode := "dummy"
-	m := []struct {
-		addr              common.Address
-		miningBeneficiary *common.Address
-		balance           *big.Int
-		blockReward       *math.HexOrDecimal256
-		mode              *string
-		list              []common.Address
-		expectedBalance   *big.Int
-	}{
-		{ // off
-			addr:              addr,
-			miningBeneficiary: nil,
-			balance:           big.NewInt(1),
-			blockReward:       math.NewHexOrDecimal256(1),
-			mode:              nil,
-			list:              nil,
-			expectedBalance:   big.NewInt(1),
-		},
-		{ // auto/default
-			addr:              addr,
-			miningBeneficiary: &addr,
-			balance:           big.NewInt(1),
-			blockReward:       math.NewHexOrDecimal256(1),
-			mode:              nil,
-			list:              nil,
-			expectedBalance:   big.NewInt(2),
-		},
-		{ // failing
-			addr:              addr,
-			miningBeneficiary: nil,
-			balance:           big.NewInt(1),
-			blockReward:       math.NewHexOrDecimal256(1),
-			mode:              &fixedMode,
-			list:              nil,
-			expectedBalance:   big.NewInt(1),
-		},
-		{
-			addr:              addr,
-			miningBeneficiary: &addr,
-			balance:           big.NewInt(1),
-			blockReward:       math.NewHexOrDecimal256(1),
-			mode:              &fixedMode,
-			list:              nil,
-			expectedBalance:   big.NewInt(2),
-		},
-		{ // failing
-			addr:              addr,
-			miningBeneficiary: nil,
-			balance:           big.NewInt(1),
-			blockReward:       math.NewHexOrDecimal256(1),
-			mode:              &listMode,
-			list:              nil,
-			expectedBalance:   big.NewInt(1),
-		},
-		{
-			addr:              addr,
-			miningBeneficiary: nil,
-			balance:           big.NewInt(1),
-			blockReward:       math.NewHexOrDecimal256(1),
-			mode:              &listMode,
-			list:              []common.Address{addr},
-			expectedBalance:   big.NewInt(2),
-		},
-		{
-			addr:              addr,
-			miningBeneficiary: nil,
-			balance:           big.NewInt(1),
-			blockReward:       math.NewHexOrDecimal256(1),
-			mode:              &validatorsMode,
-			expectedBalance:   big.NewInt(1),
-		},
-		{
-			addr:              addr,
-			miningBeneficiary: nil,
-			balance:           big.NewInt(1),
-			blockReward:       math.NewHexOrDecimal256(1),
-			mode:              &emptyMode,
-			expectedBalance:   big.NewInt(1),
-		},
-		{
-			addr:              addr,
-			miningBeneficiary: nil,
-			balance:           big.NewInt(1),
-			blockReward:       math.NewHexOrDecimal256(1),
-			mode:              &dummyMode,
-			expectedBalance:   big.NewInt(1),
-		},
-	}
-	var e *Engine
-	chain := &core.BlockChain{}
-	db := state.NewDatabaseWithConfig(rawdb.NewMemoryDatabase(), nil)
-	state, err := state.New(common.Hash{}, db, nil)
-	require.NoError(t, err)
-
-	header := &types.Header{
-		Number: big.NewInt(1),
-	}
-	for idx, te := range m {
-		if te.mode == &validatorsMode {
-			continue // skip, it's not testable yet
-		}
-		state.SetBalance(te.addr, te.balance)
-		cfg := istanbul.Config{
-			BlockReward:       te.blockReward,
-			BeneficiaryMode:   te.mode,
-			BeneficiaryList:   te.list,
-			MiningBeneficiary: te.miningBeneficiary,
-		}
-		e.accumulateRewards(chain, state, header, nil, cfg)
-		balance := state.GetBalance(te.addr)
-		assert.Equal(t, te.expectedBalance, balance, fmt.Sprintf("index: %d", idx), te)
-	}
-}

core/state_transition.go

@@ -401,10 +401,16 @@ func (st *StateTransition) TransitionDb() (*ExecutionResult, error) {
 	if !isPrivate {
 		st.gas = leftoverGas
 	}
-	// End Quorum
+
+	// Quorum with gas enabled we can specify if it goes to coinbase(ie validators) or a fixed beneficiary
+	// Note the rewards here are only for transitions, any additional block rewards must go
+	rewardAccount, err := st.evm.ChainConfig().GetRewardAccount(st.evm.Context.BlockNumber, st.evm.Context.Coinbase)
+	if err != nil {
+		return nil, err
+	}
 
 	st.refundGas()
-	st.state.AddBalance(st.evm.Context.Coinbase, new(big.Int).Mul(new(big.Int).SetUint64(st.gasUsed()), st.gasPrice))
+	st.state.AddBalance(rewardAccount, new(big.Int).Mul(new(big.Int).SetUint64(st.gasUsed()), st.gasPrice))
 
 	if isPrivate {
 		return &ExecutionResult{

eth/ethconfig/config.go

@@ -296,8 +296,11 @@ func CreateConsensusEngine(stack *node.Node, chainConfig *params.ChainConfig, co
 		if chainConfig.QBFT.ValidatorContractAddress != (common.Address{}) {
 			config.Istanbul.ValidatorContract = chainConfig.QBFT.ValidatorContractAddress
 		}
-		config.Istanbul.Validators = chainConfig.QBFT.Validators
+		config.Istanbul.BlockReward = chainConfig.QBFT.BlockReward
+		config.Istanbul.BeneficiaryMode = chainConfig.QBFT.BeneficiaryMode
+		config.Istanbul.MiningBeneficiary = chainConfig.QBFT.MiningBeneficiary
 		config.Istanbul.ValidatorSelectionMode = chainConfig.QBFT.ValidatorSelectionMode
+		config.Istanbul.Validators = chainConfig.QBFT.Validators
 
 		return istanbulBackend.New(&config.Istanbul, stack.GetNodeKey(), db)
 	}

params/config.go

@@ -431,7 +431,6 @@ type QBFTConfig struct {
 	*BFTConfig
 	BlockReward            *math.HexOrDecimal256 `json:"blockReward,omitempty"`            // Reward from start, works only on QBFT consensus protocol
 	BeneficiaryMode        *string               `json:"beneficiaryMode,omitempty"`        // Mode for setting the beneficiary, either: list, besu, validators (beneficiary list is the list of validators)
-	BeneficiaryList        []common.Address      `json:"beneficiaryList,omitempty"`        // List of wallet addresses that have benefit at every new block (list mode)
 	MiningBeneficiary      *common.Address       `json:"miningBeneficiary,omitempty"`      // Wallet address that benefits at every new block (besu mode)
 	ValidatorSelectionMode *string               `json:"validatorselectionmode,omitempty"` // Select model for validators
 	Validators             []common.Address      `json:"validators"`                       // Validators list
@@ -469,7 +468,6 @@ type Transition struct {
 	TransactionSizeLimit         uint64                `json:"transactionSizeLimit,omitempty"`         // Modify TransactionSizeLimit
 	BlockReward                  *math.HexOrDecimal256 `json:"blockReward,omitempty"`                  // validation rewards
 	BeneficiaryMode              *string               `json:"beneficiaryMode,omitempty"`              // Mode for setting the beneficiary, either: list, besu, validators (beneficiary list is the list of validators)
-	BeneficiaryList              []common.Address      `json:"beneficiaryList,omitempty"`              // List of wallet addresses that have benefit at every new block (list mode)
 	MiningBeneficiary            *common.Address       `json:"miningBeneficiary,omitempty"`            // Wallet address that benefits at every new block (besu mode)
 }
 
@@ -639,6 +637,57 @@ func (c *ChainConfig) GetMaxCodeSize(num *big.Int) int {
 	return maxCodeSize
 }
 
+func (c *ChainConfig) GetRewardAccount(num *big.Int, coinbase common.Address) (common.Address, error) {
+	beneficiaryMode := "validator"
+	miningBeneficiary := common.Address{}
+
+	if c.QBFT.MiningBeneficiary != nil {
+		miningBeneficiary = *c.QBFT.MiningBeneficiary
+		beneficiaryMode = "fixed"
+	}
+
+	if c.QBFT.BeneficiaryMode != nil {
+		beneficiaryMode = *c.QBFT.BeneficiaryMode
+	}
+
+	c.GetTransitionValue(num, func(transition Transition) {
+		if transition.BeneficiaryMode != nil && (*transition.BeneficiaryMode == "validators" || *transition.BeneficiaryMode == "validator") {
+			beneficiaryMode = "validator"
+		}
+		if transition.MiningBeneficiary != nil && (transition.BeneficiaryMode == nil || *transition.BeneficiaryMode == "fixed") {
+			miningBeneficiary = *transition.MiningBeneficiary
+			beneficiaryMode = "fixed"
+		}
+	})
+
+	switch strings.ToLower(beneficiaryMode) {
+	case "fixed":
+		log.Trace("fixed beneficiary mode", "miningBeneficiary", miningBeneficiary)
+		return miningBeneficiary, nil
+	case "validator":
+		log.Trace("validator beneficiary mode", "coinbase", coinbase)
+		return coinbase, nil
+	}
+
+	return common.Address{}, errors.New("BeneficiaryMode must be coinbase|fixed")
+}
+
+func (c *ChainConfig) GetBlockReward(num *big.Int) big.Int {
+	blockReward := *math.NewHexOrDecimal256(0)
+
+	if c.QBFT.BlockReward != nil {
+		blockReward = *c.QBFT.BlockReward
+	}
+
+	c.GetTransitionValue(num, func(transition Transition) {
+		if transition.BlockReward != nil {
+			blockReward = *transition.BlockReward
+		}
+	})
+
+	return big.Int(blockReward)
+}
+
 // Quorum
 // gets value at or after a transition
 func (c *ChainConfig) GetTransitionValue(num *big.Int, callback func(transition Transition)) {

params/config_test.go

@@ -332,10 +332,10 @@ func TestCheckTransitionsData(t *testing.T) {
 	var ibftTransitionsConfig, qbftTransitionsConfig, invalidTransition, invalidBlockOrder []Transition
 	var emptyBlockPeriodSeconds uint64 = 10
 
-	tranI0 := Transition{big.NewInt(0), IBFT, 30000, 5, nil, 10, 50, common.Address{}, nil, "", nil, nil, nil, nil, 0, nil, 0, nil, nil, nil, nil}
-	tranQ5 := Transition{big.NewInt(5), QBFT, 30000, 5, &emptyBlockPeriodSeconds, 10, 50, common.Address{}, nil, "", nil, nil, nil, nil, 0, nil, 0, nil, nil, nil, nil}
-	tranI10 := Transition{big.NewInt(10), IBFT, 30000, 5, nil, 10, 50, common.Address{}, nil, "", nil, nil, nil, nil, 0, nil, 0, nil, nil, nil, nil}
-	tranQ8 := Transition{big.NewInt(8), QBFT, 30000, 5, &emptyBlockPeriodSeconds, 10, 50, common.Address{}, nil, "", nil, nil, nil, nil, 0, nil, 0, nil, nil, nil, nil}
+	tranI0 := Transition{big.NewInt(0), IBFT, 30000, 5, nil, 10, 50, common.Address{}, nil, "", nil, nil, nil, nil, 0, nil, 0, nil, nil, nil}
+	tranQ5 := Transition{big.NewInt(5), QBFT, 30000, 5, &emptyBlockPeriodSeconds, 10, 50, common.Address{}, nil, "", nil, nil, nil, nil, 0, nil, 0, nil, nil, nil}
+	tranI10 := Transition{big.NewInt(10), IBFT, 30000, 5, nil, 10, 50, common.Address{}, nil, "", nil, nil, nil, nil, 0, nil, 0, nil, nil, nil}
+	tranQ8 := Transition{big.NewInt(8), QBFT, 30000, 5, &emptyBlockPeriodSeconds, 10, 50, common.Address{}, nil, "", nil, nil, nil, nil, 0, nil, 0, nil, nil, nil}
 
 	ibftTransitionsConfig = append(ibftTransitionsConfig, tranI0, tranI10)
 	qbftTransitionsConfig = append(qbftTransitionsConfig, tranQ5, tranQ8)
@@ -395,7 +395,7 @@ func TestCheckTransitionsData(t *testing.T) {
 			wantErr: ErrBlockOrder,
 		},
 		{
-			stored:  &ChainConfig{Transitions: []Transition{{nil, IBFT, 30000, 5, &emptyBlockPeriodSeconds, 10, 50, common.Address{}, nil, "", nil, nil, nil, nil, 0, nil, 0, nil, nil, nil, nil}}},
+			stored:  &ChainConfig{Transitions: []Transition{{nil, IBFT, 30000, 5, &emptyBlockPeriodSeconds, 10, 50, common.Address{}, nil, "", nil, nil, nil, nil, 0, nil, 0, nil, nil, nil}}},
 			wantErr: ErrBlockNumberMissing,
 		},
 		{