Merge pull request #1709 from ian0371/bugfix-reward

Fix reward miscalculation under RoundRobin policy
klaytn · Nov 21, 2022 · febce7b · febce7b
2 parents b0659bd + 46e478c
commit febce7b
Show file tree

Hide file tree

Showing 3 changed files with 134 additions and 13 deletions.
diff --git a/consensus/istanbul/backend/engine.go b/consensus/istanbul/backend/engine.go
@@ -456,7 +456,7 @@ func (sb *backend) Finalize(chain consensus.ChainReader, header *types.Header, s
 	var err error
 
 	// If sb.chain is nil, it means backend is not initialized yet.
-	if sb.chain != nil && sb.governance.Params().Policy() == uint64(istanbul.WeightedRandom) {
+	if sb.chain != nil && !reward.IsRewardSimple(chain.Config()) {
 		// TODO-Klaytn Let's redesign below logic and remove dependency between block reward and istanbul consensus.
 
 		lastHeader := chain.CurrentHeader()

diff --git a/reward/reward_distributor.go b/reward/reward_distributor.go
@@ -157,6 +157,12 @@ func GetTotalTxFee(header *types.Header, config *params.ChainConfig) *big.Int {
 	return totalFee
 }
 
+// config.Istanbul must have been set
+func IsRewardSimple(config *params.ChainConfig) bool {
+	policy := config.Istanbul.ProposerPolicy
+	return policy != uint64(istanbul.WeightedRandom)
+}
+
 // GetBlockReward returns the actual reward amounts paid in this block
 // Used in klay_getReward RPC API
 func GetBlockReward(header *types.Header, config *params.ChainConfig) (*RewardSpec, error) {
@@ -167,8 +173,7 @@ func GetBlockReward(header *types.Header, config *params.ChainConfig) (*RewardSp
 		return nil, errors.New("no IstanbulConfig")
 	}
 
-	policy := config.Istanbul.ProposerPolicy
-	if policy == uint64(istanbul.RoundRobin) || policy == uint64(istanbul.Sticky) {
+	if IsRewardSimple(config) {
 		spec, err = CalcDeferredRewardSimple(header, config)
 		if err != nil {
 			return nil, err
@@ -178,16 +183,16 @@ func GetBlockReward(header *types.Header, config *params.ChainConfig) (*RewardSp
 		if err != nil {
 			return nil, err
 		}
+	}
 
-		// Compensate the difference between CalcDeferredReward() and actual payment.
-		// If not DeferredTxFee, CalcDeferredReward() assumes 0 total_fee, but
-		// some non-zero fee is paid to the proposer.
-		if !config.Governance.Reward.DeferredTxFee {
-			blockFee := GetTotalTxFee(header, config)
-			spec.Proposer = spec.Proposer.Add(spec.Proposer, spec.TotalFee)
-			spec.TotalFee = spec.TotalFee.Add(spec.TotalFee, blockFee)
-			incrementRewardsMap(spec.Rewards, header.Rewardbase, blockFee)
-		}
+	// Compensate the difference between CalcDeferredReward() and actual payment.
+	// If not DeferredTxFee, CalcDeferredReward() assumes 0 total_fee, but
+	// some non-zero fee already has been paid to the proposer.
+	if !config.Governance.Reward.DeferredTxFee {
+		blockFee := GetTotalTxFee(header, config)
+		spec.Proposer = spec.Proposer.Add(spec.Proposer, spec.TotalFee)
+		spec.TotalFee = spec.TotalFee.Add(spec.TotalFee, blockFee)
+		incrementRewardsMap(spec.Rewards, header.Rewardbase, blockFee)
 	}
 
 	return spec, nil
@@ -205,6 +210,29 @@ func CalcDeferredRewardSimple(header *types.Header, config *params.ChainConfig)
 	}
 
 	minted := rc.mintingAmount
+
+	// If not DeferredTxFee, fees are already added to the proposer during TX execution.
+	// Therefore, there are no fees to distribute here at the end of block processing.
+	// However, before Kore, there was a bug that distributed tx fee regardless
+	// of `deferredTxFee` flag. See https://github.com/klaytn/klaytn/issues/1692.
+	// To maintain backward compatibility, we only fix the buggy logic after Kore
+	// and leave the buggy logic before Kore.
+	// However, the fees must be compensated to calculate actual rewards paid.
+
+	// bug-fixed logic after Kore
+	if !rc.deferredTxFee && rc.rules.IsKore {
+		proposer := new(big.Int).Set(minted)
+		logger.Debug("CalcDeferredRewardSimple after Kore when deferredTxFee=false returns",
+			"proposer", proposer)
+		return &RewardSpec{
+			Minted:   minted,
+			TotalFee: big.NewInt(0),
+			BurntFee: big.NewInt(0),
+			Proposer: proposer,
+			Rewards:  map[common.Address]*big.Int{header.Rewardbase: proposer},
+		}, nil
+	}
+
 	totalFee := rc.totalFee
 	rewardFee := new(big.Int).Set(totalFee)
 	burntFee := big.NewInt(0)
@@ -217,6 +245,12 @@ func CalcDeferredRewardSimple(header *types.Header, config *params.ChainConfig)
 
 	proposer := big.NewInt(0).Add(minted, rewardFee)
 
+	logger.Debug("CalcDeferredRewardSimple returns",
+		"proposer", proposer.Uint64(),
+		"totalFee", totalFee.Uint64(),
+		"burntFee", burntFee.Uint64(),
+	)
+
 	return &RewardSpec{
 		Minted:   minted,
 		TotalFee: totalFee,

diff --git a/reward/reward_distributor_test.go b/reward/reward_distributor_test.go
@@ -294,7 +294,7 @@ func TestRewardDistributor_GetBlockReward(t *testing.T) {
 	}{
 		{
 			policy:        istanbul.RoundRobin,
-			deferredTxFee: false,
+			deferredTxFee: true,
 			expected: &RewardSpec{
 				Minted:   minted,
 				TotalFee: new(big.Int).SetUint64(1000),
@@ -305,6 +305,19 @@ func TestRewardDistributor_GetBlockReward(t *testing.T) {
 				},
 			},
 		},
+		{
+			policy:        istanbul.RoundRobin,
+			deferredTxFee: false,
+			expected: &RewardSpec{
+				Minted:   minted,
+				TotalFee: new(big.Int).SetUint64(1000),
+				BurntFee: new(big.Int).SetUint64(0),
+				Proposer: new(big.Int).SetUint64(9.6e18 + 1000),
+				Rewards: map[common.Address]*big.Int{
+					proposerAddr: new(big.Int).SetUint64(9.6e18 + 1000),
+				},
+			},
+		},
 		{
 			policy:        istanbul.WeightedRandom,
 			deferredTxFee: true,
@@ -411,6 +424,80 @@ func TestRewardDistributor_CalcDeferredRewardSimple(t *testing.T) {
 	}
 }
 
+// Before Kore, there was a bug that distributed txFee at the end of
+// block processing regardless of `deferredTxFee` flag.
+// See https://github.com/klaytn/klaytn/issues/1692.
+// To maintain backward compatibility, we only fix the buggy logic after Kore
+// and leave the buggy logic before Kore.
+func TestRewardDistributor_CalcDeferredRewardSimple_nodeferred(t *testing.T) {
+	header := &types.Header{
+		Number:     big.NewInt(1),
+		GasUsed:    1000,
+		BaseFee:    big.NewInt(1),
+		Rewardbase: proposerAddr,
+	}
+
+	testcases := []struct {
+		isMagma  bool
+		isKore   bool
+		expected *RewardSpec
+	}{
+		{ // totalFee should have been 0, but returned due to bug
+			isMagma: false,
+			isKore:  false,
+			expected: &RewardSpec{
+				Minted:   minted,
+				TotalFee: new(big.Int).SetUint64(1000),
+				BurntFee: new(big.Int).SetUint64(0),
+				Proposer: new(big.Int).SetUint64(9.6e18 + 1000),
+				Rewards: map[common.Address]*big.Int{
+					proposerAddr: new(big.Int).SetUint64(9.6e18 + 1000),
+				},
+			},
+		},
+		{ // totalFee should have been 0, but returned due to bug
+			isMagma: true,
+			isKore:  false,
+			expected: &RewardSpec{
+				Minted:   minted,
+				TotalFee: new(big.Int).SetUint64(1000),
+				BurntFee: new(big.Int).SetUint64(500),
+				Proposer: new(big.Int).SetUint64(9.6e18 + 500),
+				Rewards: map[common.Address]*big.Int{
+					proposerAddr: new(big.Int).SetUint64(9.6e18 + 500),
+				},
+			},
+		},
+		{ // totalFee is now 0 because bug is fixed after Kore
+			isMagma: true,
+			isKore:  true,
+			expected: &RewardSpec{
+				Minted:   minted,
+				TotalFee: new(big.Int).SetUint64(0),
+				BurntFee: new(big.Int).SetUint64(0),
+				Proposer: new(big.Int).SetUint64(9.6e18),
+				Rewards: map[common.Address]*big.Int{
+					proposerAddr: new(big.Int).SetUint64(9.6e18),
+				},
+			},
+		},
+	}
+
+	for i, tc := range testcases {
+		config := noDeferred((getTestConfig()))
+		if !tc.isMagma {
+			config = noMagma(config)
+		}
+		if !tc.isKore {
+			config = noKore(config)
+		}
+
+		spec, err := CalcDeferredRewardSimple(header, config)
+		assert.Nil(t, err, "testcases[%d] failed", i)
+		assert.Equal(t, tc.expected, spec, "testcases[%d] failed", i)
+	}
+}
+
 func TestRewardDistributor_CalcDeferredReward(t *testing.T) {
 	oldStakingManager := GetStakingManager()
 	defer SetTestStakingManager(oldStakingManager)