From 0dd1c032db89fb343ad15d8d0c32ae50d0cccc57 Mon Sep 17 00:00:00 2001 From: Antony Denyer Date: Sun, 30 Oct 2022 15:33:49 +0000 Subject: [PATCH] fix: qbft issues with block rewards and beneficary - multiple issues were found with coinbase rewards when running qbft - fix up issues with MiningBeneficiary --- cmd/utils/flags.go | 1 - consensus/istanbul/config.go | 18 +-- consensus/istanbul/qbft/engine/engine.go | 49 ++----- consensus/istanbul/qbft/engine/engine_test.go | 129 ------------------ core/state_transition.go | 10 +- eth/ethconfig/config.go | 5 +- params/config.go | 53 ++++++- params/config_test.go | 10 +- 8 files changed, 87 insertions(+), 188 deletions(-) diff --git a/cmd/utils/flags.go b/cmd/utils/flags.go index 4c1bfb3835..99480dde17 100644 --- a/cmd/utils/flags.go +++ b/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 diff --git a/consensus/istanbul/config.go b/consensus/istanbul/config.go index 17e7809a74..d7b0e46999 100644 --- a/consensus/istanbul/config.go +++ b/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 { diff --git a/consensus/istanbul/qbft/engine/engine.go b/consensus/istanbul/qbft/engine/engine.go index 27674fec3a..a7135db01c 100644 --- a/consensus/istanbul/qbft/engine/engine.go +++ b/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) } } diff --git a/consensus/istanbul/qbft/engine/engine_test.go b/consensus/istanbul/qbft/engine/engine_test.go index 6ee803c4a2..458c021a09 100644 --- a/consensus/istanbul/qbft/engine/engine_test.go +++ b/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) - } -} diff --git a/core/state_transition.go b/core/state_transition.go index a80eb04f1a..d6401f012e 100644 --- a/core/state_transition.go +++ b/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{ diff --git a/eth/ethconfig/config.go b/eth/ethconfig/config.go index 119e04e962..9fc47a02ff 100644 --- a/eth/ethconfig/config.go +++ b/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) } diff --git a/params/config.go b/params/config.go index 3cb0c856c6..25ee51a989 100644 --- a/params/config.go +++ b/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 != nil && c.QBFT.MiningBeneficiary != nil { + miningBeneficiary = *c.QBFT.MiningBeneficiary + beneficiaryMode = "fixed" + } + + if c.QBFT != nil && 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)) { diff --git a/params/config_test.go b/params/config_test.go index 4e927cc71e..4037bc7a30 100644 --- a/params/config_test.go +++ b/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, }, {