chore: add comments to eip-1559 code (#6818)

* chore: add comments to eip-1559 code * Update x/txfees/keeper/mempool-1559/code.go Co-authored-by: Roman <[email protected]> * Update x/txfees/keeper/mempool-1559/code.go Co-authored-by: Roman <[email protected]> * chore: add logger, use const instead of var * chore: remove sleep in test --------- Co-authored-by: Roman <[email protected]>
osmosis-labs · Nov 3, 2023 · 83c9b13 · 83c9b13
1 parent 41db47a
commit 83c9b13
Show file tree

Hide file tree

Showing 4 changed files with 82 additions and 77 deletions.
diff --git a/x/txfees/keeper/mempool-1559/code.go b/x/txfees/keeper/mempool-1559/code.go
@@ -10,51 +10,69 @@ import (
 	osmomath "github.com/osmosis-labs/osmosis/osmomath"
 )
 
-// Sections to this right now:
-// - Maintain something thats gets parsed from chain tx execution
-// update eipState according to that.
-// - Every time blockheight % 1000 = 0, reset eipState to default. (B/c this isn't committed on-chain, still gives us some consistency guarantees)
-// - Evaluate CheckTx/RecheckTx against this.
-//
-// 1000 blocks = almost 2 hours, maybe we need a smaller time for resets?
-// Lets say 500 blocks = 1 hour
-//
-// PROBLEMS: Currently, a node will throw out any tx that gets under its gas bound here.
-// :OOO We can just do this on checkTx not recheck
-//
-// Variables we can control for:
-// - fees paid per unit gas
-// - gas wanted per block (Ethereum)
-// - gas used and gas wanted difference
-// TODO: Change this percentage update time to be faster
-
-// TODO: Read this from config, can even make default 0, so this is only turned on by nodes who change it!
-// ALt: do that with an enable/disable flag. That seems likes a better idea
-
-var DefaultBaseFee = sdk.MustNewDecFromStr("0.0025")
-var MinBaseFee = sdk.MustNewDecFromStr("0.0025")
-var MaxBaseFee = sdk.MustNewDecFromStr("10")
-var TargetGas = int64(60_000_000)
-var MaxBlockChangeRate = sdk.NewDec(1).Quo(sdk.NewDec(16))
-var ResetInterval = int64(1000)
-var BackupFile = "eip1559state.json"
-var RecheckFeeConstant = int64(4)
+/*
+   This is the logic for the Osmosis implementation for EIP-1559 fee market,
+	 the goal of this code is to prevent spam by charging more for transactions when the network is busy.
+
+	 This logic does two things:
+   - Maintaining data parsed from chain transaction execution and updating eipState accordingly.
+   - Resetting eipState to default every ResetInterval (1000) block height intervals to maintain consistency.
+
+   Additionally:
+   - Periodically evaluating CheckTx and RecheckTx for compliance with these parameters.
+
+   Note: The reset interval is set to 1000 blocks, which is approximately 2 hours. Consider adjusting for a smaller time interval (e.g., 500 blocks = 1 hour) if necessary.
+
+   Challenges:
+   - Transactions falling under their gas bounds are currently discarded by nodes. This behavior can be modified for CheckTx, rather than RecheckTx.
+
+   Global variables stored in memory:
+   - DefaultBaseFee: Default base fee, initialized to 0.0025.
+   - MinBaseFee: Minimum base fee, initialized to 0.0025.
+   - MaxBaseFee: Maximum base fee, initialized to 10.
+   - MaxBlockChangeRate: The maximum block change rate, initialized to 1/16.
+
+   Global constants:
+   - TargetGas: Gas wanted per block, initialized to 60,000,000.
+   - ResetInterval: The interval at which eipState is reset, initialized to 1000 blocks.
+   - BackupFile: File for backup, set to "eip1559state.json".
+   - RecheckFeeConstant: A constant value for rechecking fees, initialized to 4.
+*/
+
+var (
+	DefaultBaseFee     = sdk.MustNewDecFromStr("0.0025")
+	MinBaseFee         = sdk.MustNewDecFromStr("0.0025")
+	MaxBaseFee         = sdk.MustNewDecFromStr("10")
+	MaxBlockChangeRate = sdk.NewDec(1).Quo(sdk.NewDec(16))
+)
 
+const (
+	TargetGas          = int64(60_000_000)
+	ResetInterval      = int64(1000)
+	BackupFile         = "eip1559state.json"
+	RecheckFeeConstant = int64(4)
+)
+
+// EipState tracks the current base fee and totalGasWantedThisBlock
+// this structure is never written to state
 type EipState struct {
-	// Signal when we are starting a new block
-	// TODO: Or just use begin block
 	lastBlockHeight         int64
 	totalGasWantedThisBlock int64
 
 	CurBaseFee osmomath.Dec `json:"cur_base_fee"`
 }
 
+// CurEipState is a global variable used in the BeginBlock, EndBlock and
+// DeliverTx (fee decorator AnteHandler) functions, it's also using when determining
+// if a transaction has enough gas to successfully execute
 var CurEipState = EipState{
 	lastBlockHeight:         0,
 	totalGasWantedThisBlock: 0,
 	CurBaseFee:              sdk.NewDec(0),
 }
 
+// startBlock is executed at the start of each block and is responsible for reseting the state
+// of the CurBaseFee when the node reaches the reset interval
 func (e *EipState) startBlock(height int64) {
 	e.lastBlockHeight = height
 	e.totalGasWantedThisBlock = 0
@@ -65,22 +83,27 @@ func (e *EipState) startBlock(height int64) {
 		e.CurBaseFee = e.tryLoad()
 	}
 
+	// we reset the CurBaseFee every ResetInterval
 	if height%ResetInterval == 0 {
 		e.CurBaseFee = DefaultBaseFee.Clone()
 	}
 }
 
+// deliverTxCode runs on every transaction in the feedecorator ante handler and sums the gas of each transaction
 func (e *EipState) deliverTxCode(ctx sdk.Context, tx sdk.FeeTx) {
 	if ctx.BlockHeight() != e.lastBlockHeight {
-		fmt.Println("Something is off here? ctx.BlockHeight() != e.lastBlockHeight", ctx.BlockHeight(), e.lastBlockHeight)
+		ctx.Logger().Error("Something is off here? ctx.BlockHeight() != e.lastBlockHeight", ctx.BlockHeight(), e.lastBlockHeight)
 	}
 	e.totalGasWantedThisBlock += int64(tx.GetGas())
-	// fmt.Println("height, tx gas, blockGas", ctx.BlockHeight(), tx.GetGas(), e.totalGasWantedThisBlock)
 }
 
-// Equation is:
-// baseFeeMultiplier = 1 + (gasUsed - targetGas) / targetGas * maxChangeRate
-// newBaseFee = baseFee * baseFeeMultiplier
+// updateBaseFee updates of a base fee in Osmosis.
+// It employs the following equation to calculate the new base fee:
+//
+//	baseFeeMultiplier = 1 + (gasUsed - targetGas) / targetGas * maxChangeRate
+//	newBaseFee = baseFee * baseFeeMultiplier
+//
+// updateBaseFee runs at the end of every block
 func (e *EipState) updateBaseFee(height int64) {
 	if height != e.lastBlockHeight {
 		fmt.Println("Something is off here? height != e.lastBlockHeight", height, e.lastBlockHeight)
@@ -94,27 +117,33 @@ func (e *EipState) updateBaseFee(height int64) {
 	baseFeeMultiplier := sdk.NewDec(1).Add(baseFeeIncrement)
 	e.CurBaseFee.MulMut(baseFeeMultiplier)
 
-	// Make a min base fee
+	// Enforce the minimum base fee by resetting the CurBaseFee is it drops below the MinBaseFee
 	if e.CurBaseFee.LT(MinBaseFee) {
 		e.CurBaseFee = MinBaseFee.Clone()
 	}
 
-	// Make a max base fee
+	// Enforce the maximum base fee by resetting the CurBaseFee is it goes above the MaxBaseFee
 	if e.CurBaseFee.GT(MaxBaseFee) {
 		e.CurBaseFee = MaxBaseFee.Clone()
 	}
 
 	go e.tryPersist()
 }
 
+// GetCurBaseFee returns a clone of the CurBaseFee to avoid overwriting the initial value in
+// the EipState, we use this in the AnteHandler to Check transactions
 func (e *EipState) GetCurBaseFee() osmomath.Dec {
 	return e.CurBaseFee.Clone()
 }
 
+// GetCurRecheckBaseFee returns a clone of the CurBaseFee / RecheckFeeConstant to account for
+// rechecked transactions in the feedecorator ante handler
 func (e *EipState) GetCurRecheckBaseFee() osmomath.Dec {
 	return e.CurBaseFee.Clone().Quo(sdk.NewDec(RecheckFeeConstant))
 }
 
+// tryPersist persists the eip1559 state to disk in the form of a json file
+// we do this in case a node stops and it can continue functioning as normal
 func (e *EipState) tryPersist() {
 	bz, err := json.Marshal(e)
 	if err != nil {
@@ -129,6 +158,8 @@ func (e *EipState) tryPersist() {
 	}
 }
 
+// tryLoad reads eip1559 state from disk and initializes the CurEipState to
+// the previous state when a node is restarted
 func (e *EipState) tryLoad() osmomath.Dec {
 	bz, err := os.ReadFile(BackupFile)
 	if err != nil {

diff --git a/x/txfees/keeper/mempool-1559/code_test.go b/x/txfees/keeper/mempool-1559/code_test.go
@@ -2,16 +2,16 @@ package mempool1559
 
 import (
 	"testing"
-	"time"
 
 	sdk "github.com/cosmos/cosmos-sdk/types"
+	"github.com/tendermint/tendermint/libs/log"
 	tmproto "github.com/tendermint/tendermint/proto/tendermint/types"
 	"gotest.tools/assert"
 
 	"github.com/osmosis-labs/osmosis/osmoutils/noapptest"
 )
 
-// TestUpdateBaseFee simulates the update of a base fee in a blockchain system.
+// TestUpdateBaseFee simulates the update of a base fee in Osmosis.
 // It employs the following equation to calculate the new base fee:
 //
 //	baseFeeMultiplier = 1 + (gasUsed - targetGas) / targetGas * maxChangeRate
@@ -27,9 +27,10 @@ func TestUpdateBaseFee(t *testing.T) {
 		CurBaseFee:              DefaultBaseFee.Clone(),
 	}
 
+	// we iterate over 1000 blocks as the reset happens after 1000 blocks
 	for i := 1; i <= 1002; i++ {
 		// create a new block
-		ctx := sdk.NewContext(nil, tmproto.Header{Height: int64(i)}, false, nil)
+		ctx := sdk.NewContext(nil, tmproto.Header{Height: int64(i)}, false, log.NewNopLogger())
 
 		// start the new block
 		eip.startBlock(int64(i))
@@ -41,7 +42,7 @@ func TestUpdateBaseFee(t *testing.T) {
 				eip.deliverTxCode(ctx, tx.(sdk.FeeTx))
 			}
 		}
-		baseFeeBeforeUpdate := eip.CurBaseFee.Clone()
+		baseFeeBeforeUpdate := eip.GetCurBaseFee()
 
 		// update base fee
 		eip.updateBaseFee(int64(i))
@@ -52,14 +53,9 @@ func TestUpdateBaseFee(t *testing.T) {
 		// Assert that the actual result matches the expected result
 		assert.DeepEqual(t, expectedBaseFee, eip.CurBaseFee)
 	}
-
-	// We wait here to test the write as the write is async
-	time.Sleep(100 * time.Millisecond)
-	readCurBaseFee := eip.tryLoad()
-	assert.DeepEqual(t, readCurBaseFee, eip.CurBaseFee)
 }
 
-// calculateBaseFee is the same as in the test
+// calculateBaseFee is the same as in is defined on the eip1559 code
 func calculateBaseFee(totalGasWantedThisBlock int64, eipStateCurBaseFee sdk.Dec) (expectedBaseFee sdk.Dec) {
 	gasUsed := totalGasWantedThisBlock
 	gasDiff := gasUsed - TargetGas
@@ -79,7 +75,7 @@ func calculateBaseFee(totalGasWantedThisBlock int64, eipStateCurBaseFee sdk.Dec)
 	return expectedBaseFee
 }
 
-// GenTx generates a signed mock transaction.
+// GenTx generates a mock gas transaction.
 func GenTx(gas uint64) sdk.Tx {
 	gen := noapptest.MakeTestEncodingConfig().TxConfig
 	txBuilder := gen.NewTxBuilder()

diff --git a/x/txfees/keeper/mempool-1559/state_compatible_update_logic.go b/x/txfees/keeper/mempool-1559/state_compatible_update_logic.go
@@ -2,14 +2,20 @@ package mempool1559
 
 import sdk "github.com/cosmos/cosmos-sdk/types"
 
+// DeliverTxCode is run on every transaction and will collect
+// the gas for every transaction for use calculating gas
 func DeliverTxCode(ctx sdk.Context, tx sdk.FeeTx) {
 	CurEipState.deliverTxCode(ctx, tx)
 }
 
+// BeginBlockCode runs at the start of every block and it
+// reset the CurEipStates lastBlockHeight and totalGasWantedThisBlock
 func BeginBlockCode(ctx sdk.Context) {
 	CurEipState.startBlock(ctx.BlockHeight())
 }
 
+// EndBlockCode runs at the end of every block and it
+// updates the base fee based on the block attributes
 func EndBlockCode(ctx sdk.Context) {
 	CurEipState.updateBaseFee(ctx.BlockHeight())
 }
diff --git a/x/txfees/keeper/mempool-1559/todo.md b/x/txfees/keeper/mempool-1559/todo.md