refactor(feeaddress): improve validation separation and add docs (#6464)

## Summary
Addresses review comments from #6441:

- Move fee/gas validation from ProcessProposal to ante handler
(`ProtocolFeeTerminatorDecorator`)
  - Ante validates: fee == balance, gas == ProtocolFeeGasLimit
  - ProcessProposal only validates block structure (tx presence/absence)
- Add `pkg/feeaddress/README.md` with architecture, consensus safety,
threat model
- Add `IsProtocolFeeTxBytes` helper, remove `parseProtocolFeeTx` and
`validateProtocolFeeTx`
- Refactor tests: remove nested ifs, split into focused functions

## Test plan
- [x] `go build ./...` passes
- [x] `go test -run Protocol ./app/...` passes
- [x] `golangci-lint run ./app/... ./pkg/feeaddress/...` passes

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
Co-authored-by: Rootul P <rootulp@gmail.com>

Author: 3 people

app/ante/ante.go

@@ -9,14 +9,14 @@ import (
 	minfeekeeper "github.com/celestiaorg/celestia-app/v7/x/minfee/keeper"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	"github.com/cosmos/cosmos-sdk/x/auth/ante"
-	authtypes "github.com/cosmos/cosmos-sdk/x/auth/types"
+	bankkeeper "github.com/cosmos/cosmos-sdk/x/bank/keeper"
 	ibcante "github.com/cosmos/ibc-go/v8/modules/core/ante"
 	ibckeeper "github.com/cosmos/ibc-go/v8/modules/core/keeper"
 )
 
 func NewAnteHandler(
 	accountKeeper ante.AccountKeeper,
-	bankKeeper authtypes.BankKeeper,
+	bankKeeper bankkeeper.Keeper,
 	blobKeeper blob.Keeper,
 	feegrantKeeper ante.FeegrantKeeper,
 	signModeHandler *txsigning.HandlerMap,

app/ante/protocol_fee.go

@@ -1,7 +1,10 @@
 package ante
 
 import (
+	"fmt"
+
 	"cosmossdk.io/errors"
+	"github.com/celestiaorg/celestia-app/v7/pkg/appconsts"
 	"github.com/celestiaorg/celestia-app/v7/pkg/feeaddress"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
@@ -15,9 +18,10 @@ import (
 //
 // For MsgPayProtocolFee transactions, this decorator:
 // 1. Rejects user submissions (only valid when protocol-injected)
-// 2. Validates the fee format
-// 3. Transfers the fee from fee address to fee collector
-// 4. Returns without calling next() - skipping the rest of the ante chain
+// 2. Validates fee == fee address balance (ensures ALL funds are forwarded)
+// 3. Validates gas == ProtocolFeeGasLimit
+// 4. Transfers the fee from fee address to fee collector
+// 5. Returns without calling next() - skipping the rest of the ante chain
 //
 // For all other transactions, this decorator simply calls next().
 type ProtocolFeeTerminatorDecorator struct {
@@ -53,10 +57,20 @@ func (d ProtocolFeeTerminatorDecorator) AnteHandle(ctx sdk.Context, tx sdk.Tx, s
 	}
 	fee := feeTx.GetFee()
 
-	if err := feeaddress.ValidateProtocolFee(fee, nil); err != nil {
+	// Get current fee address balance - the fee MUST equal this balance exactly.
+	// This ensures ALL accumulated funds are forwarded to validators, preventing
+	// a malicious proposer from only forwarding a partial amount.
+	feeAddressBalance := d.bankKeeper.GetBalance(ctx, feeaddress.FeeAddress, appconsts.BondDenom)
+	if err := feeaddress.ValidateProtocolFee(fee, &feeAddressBalance); err != nil {
 		return ctx, errors.Wrap(sdkerrors.ErrInvalidRequest, err.Error())
 	}
 
+	// Validate gas limit matches expected constant
+	if feeTx.GetGas() != feeaddress.ProtocolFeeGasLimit {
+		return ctx, errors.Wrap(sdkerrors.ErrInvalidRequest,
+			fmt.Sprintf("gas limit %d does not match expected %d", feeTx.GetGas(), feeaddress.ProtocolFeeGasLimit))
+	}
+
 	err := d.bankKeeper.SendCoinsFromAccountToModule(ctx, feeaddress.FeeAddress, authtypes.FeeCollectorName, fee)
 	if err != nil {
 		return ctx, errors.Wrap(err, "failed to deduct fee from fee address")

app/ante/protocol_fee_test.go

@@ -35,6 +35,11 @@ func (m *protoFeeMockFeeTx) FeeGranter() []byte                    { return nil
 // protoFeeMockBankKeeper implements feeaddress.ProtocolFeeBankKeeper for testing.
 type protoFeeMockBankKeeper struct {
 	sentToModule map[string]sdk.Coins
+	balance      sdk.Coin // balance to return from GetBalance
+}
+
+func (m *protoFeeMockBankKeeper) GetBalance(_ context.Context, _ sdk.AccAddress, _ string) sdk.Coin {
+	return m.balance
 }
 
 func (m *protoFeeMockBankKeeper) SendCoinsFromAccountToModule(_ context.Context, _ sdk.AccAddress, recipientModule string, amt sdk.Coins) error {
@@ -56,7 +61,12 @@ func (m *protoFeeMockNonFeeTx) ValidateBasic() error                  { return n
 
 // protoFeeMockBankKeeperWithError implements feeaddress.ProtocolFeeBankKeeper and returns an error.
 type protoFeeMockBankKeeperWithError struct {
-	err error
+	err     error
+	balance sdk.Coin
+}
+
+func (m *protoFeeMockBankKeeperWithError) GetBalance(_ context.Context, _ sdk.AccAddress, _ string) sdk.Coin {
+	return m.balance
 }
 
 func (m *protoFeeMockBankKeeperWithError) SendCoinsFromAccountToModule(_ context.Context, _ sdk.AccAddress, _ string, _ sdk.Coins) error {
@@ -68,12 +78,13 @@ func protoFeeNextAnteHandler(ctx sdk.Context, _ sdk.Tx, _ bool) (sdk.Context, er
 }
 
 func TestProtocolFeeTerminatorRejectsUserSubmittedTx(t *testing.T) {
-	bankKeeper := &protoFeeMockBankKeeper{}
+	balance := sdk.NewCoin(appconsts.BondDenom, math.NewInt(1000))
+	bankKeeper := &protoFeeMockBankKeeper{balance: balance}
 	decorator := ante.NewProtocolFeeTerminatorDecorator(bankKeeper)
 
 	msg := feeaddress.NewMsgPayProtocolFee()
-	fee := sdk.NewCoins(sdk.NewCoin(appconsts.BondDenom, math.NewInt(1000)))
-	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: fee, gas: 50000}
+	fee := sdk.NewCoins(balance)
+	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: fee, gas: feeaddress.ProtocolFeeGasLimit}
 
 	// Create CheckTx context - this simulates a user submitting the tx
 	ctx := sdk.NewContext(nil, tmproto.Header{}, true, log.NewNopLogger()) // isCheckTx = true
@@ -85,12 +96,13 @@ func TestProtocolFeeTerminatorRejectsUserSubmittedTx(t *testing.T) {
 }
 
 func TestProtocolFeeTerminatorRejectsReCheckTx(t *testing.T) {
-	bankKeeper := &protoFeeMockBankKeeper{}
+	balance := sdk.NewCoin(appconsts.BondDenom, math.NewInt(1000))
+	bankKeeper := &protoFeeMockBankKeeper{balance: balance}
 	decorator := ante.NewProtocolFeeTerminatorDecorator(bankKeeper)
 
 	msg := feeaddress.NewMsgPayProtocolFee()
-	fee := sdk.NewCoins(sdk.NewCoin(appconsts.BondDenom, math.NewInt(1000)))
-	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: fee, gas: 50000}
+	fee := sdk.NewCoins(balance)
+	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: fee, gas: feeaddress.ProtocolFeeGasLimit}
 
 	// Create ReCheckTx context
 	ctx := sdk.NewContext(nil, tmproto.Header{}, true, log.NewNopLogger()).WithIsReCheckTx(true)
@@ -102,7 +114,8 @@ func TestProtocolFeeTerminatorRejectsReCheckTx(t *testing.T) {
 }
 
 func TestProtocolFeeTerminatorValidatesSingleDenom(t *testing.T) {
-	bankKeeper := &protoFeeMockBankKeeper{}
+	balance := sdk.NewCoin(appconsts.BondDenom, math.NewInt(1000))
+	bankKeeper := &protoFeeMockBankKeeper{balance: balance}
 	decorator := ante.NewProtocolFeeTerminatorDecorator(bankKeeper)
 
 	msg := feeaddress.NewMsgPayProtocolFee()
@@ -111,7 +124,7 @@ func TestProtocolFeeTerminatorValidatesSingleDenom(t *testing.T) {
 		sdk.NewCoin(appconsts.BondDenom, math.NewInt(1000)),
 		sdk.NewCoin("otherdenom", math.NewInt(500)),
 	)
-	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: fee, gas: 50000}
+	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: fee, gas: feeaddress.ProtocolFeeGasLimit}
 
 	// Create DeliverTx context (not CheckTx)
 	ctx := sdk.NewContext(nil, tmproto.Header{}, false, log.NewNopLogger())
@@ -123,13 +136,14 @@ func TestProtocolFeeTerminatorValidatesSingleDenom(t *testing.T) {
 }
 
 func TestProtocolFeeTerminatorRejectsWrongDenom(t *testing.T) {
-	bankKeeper := &protoFeeMockBankKeeper{}
+	balance := sdk.NewCoin(appconsts.BondDenom, math.NewInt(1000))
+	bankKeeper := &protoFeeMockBankKeeper{balance: balance}
 	decorator := ante.NewProtocolFeeTerminatorDecorator(bankKeeper)
 
 	msg := feeaddress.NewMsgPayProtocolFee()
 	// Wrong denom - should be rejected
 	fee := sdk.NewCoins(sdk.NewCoin("wrongdenom", math.NewInt(1000)))
-	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: fee, gas: 50000}
+	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: fee, gas: feeaddress.ProtocolFeeGasLimit}
 
 	// Create DeliverTx context (not CheckTx)
 	ctx := sdk.NewContext(nil, tmproto.Header{}, false, log.NewNopLogger())
@@ -141,12 +155,13 @@ func TestProtocolFeeTerminatorRejectsWrongDenom(t *testing.T) {
 }
 
 func TestProtocolFeeTerminatorSuccess(t *testing.T) {
-	bankKeeper := &protoFeeMockBankKeeper{}
+	balance := sdk.NewCoin(appconsts.BondDenom, math.NewInt(1000))
+	bankKeeper := &protoFeeMockBankKeeper{balance: balance}
 	decorator := ante.NewProtocolFeeTerminatorDecorator(bankKeeper)
 
 	msg := feeaddress.NewMsgPayProtocolFee()
-	fee := sdk.NewCoins(sdk.NewCoin(appconsts.BondDenom, math.NewInt(1000)))
-	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: fee, gas: 50000}
+	fee := sdk.NewCoins(balance)
+	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: fee, gas: feeaddress.ProtocolFeeGasLimit}
 
 	// Create DeliverTx context (not CheckTx)
 	ctx := sdk.NewContext(nil, tmproto.Header{}, false, log.NewNopLogger())
@@ -159,12 +174,13 @@ func TestProtocolFeeTerminatorSuccess(t *testing.T) {
 }
 
 func TestProtocolFeeTerminatorRejectsSimulation(t *testing.T) {
-	bankKeeper := &protoFeeMockBankKeeper{}
+	balance := sdk.NewCoin(appconsts.BondDenom, math.NewInt(1000))
+	bankKeeper := &protoFeeMockBankKeeper{balance: balance}
 	decorator := ante.NewProtocolFeeTerminatorDecorator(bankKeeper)
 
 	msg := feeaddress.NewMsgPayProtocolFee()
-	fee := sdk.NewCoins(sdk.NewCoin(appconsts.BondDenom, math.NewInt(1000)))
-	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: fee, gas: 50000}
+	fee := sdk.NewCoins(balance)
+	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: fee, gas: feeaddress.ProtocolFeeGasLimit}
 
 	// Create DeliverTx context but pass simulate=true
 	ctx := sdk.NewContext(nil, tmproto.Header{}, false, log.NewNopLogger())
@@ -176,7 +192,8 @@ func TestProtocolFeeTerminatorRejectsSimulation(t *testing.T) {
 }
 
 func TestProtocolFeeTerminatorRejectsNonFeeTx(t *testing.T) {
-	bankKeeper := &protoFeeMockBankKeeper{}
+	balance := sdk.NewCoin(appconsts.BondDenom, math.NewInt(1000))
+	bankKeeper := &protoFeeMockBankKeeper{balance: balance}
 	decorator := ante.NewProtocolFeeTerminatorDecorator(bankKeeper)
 
 	msg := feeaddress.NewMsgPayProtocolFee()
@@ -192,12 +209,13 @@ func TestProtocolFeeTerminatorRejectsNonFeeTx(t *testing.T) {
 }
 
 func TestProtocolFeeTerminatorBankTransferFailure(t *testing.T) {
-	bankKeeper := &protoFeeMockBankKeeperWithError{err: sdkerrors.ErrInsufficientFunds}
+	balance := sdk.NewCoin(appconsts.BondDenom, math.NewInt(1000))
+	bankKeeper := &protoFeeMockBankKeeperWithError{err: sdkerrors.ErrInsufficientFunds, balance: balance}
 	decorator := ante.NewProtocolFeeTerminatorDecorator(bankKeeper)
 
 	msg := feeaddress.NewMsgPayProtocolFee()
-	fee := sdk.NewCoins(sdk.NewCoin(appconsts.BondDenom, math.NewInt(1000)))
-	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: fee, gas: 50000}
+	fee := sdk.NewCoins(balance)
+	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: fee, gas: feeaddress.ProtocolFeeGasLimit}
 
 	// Create DeliverTx context (not CheckTx)
 	ctx := sdk.NewContext(nil, tmproto.Header{}, false, log.NewNopLogger())
@@ -209,12 +227,13 @@ func TestProtocolFeeTerminatorBankTransferFailure(t *testing.T) {
 }
 
 func TestProtocolFeeTerminatorZeroFeeRejected(t *testing.T) {
-	bankKeeper := &protoFeeMockBankKeeper{}
+	balance := sdk.NewCoin(appconsts.BondDenom, math.NewInt(1000))
+	bankKeeper := &protoFeeMockBankKeeper{balance: balance}
 	decorator := ante.NewProtocolFeeTerminatorDecorator(bankKeeper)
 
 	msg := feeaddress.NewMsgPayProtocolFee()
 	// Zero fee should be rejected
-	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: sdk.Coins{}, gas: 50000}
+	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: sdk.Coins{}, gas: feeaddress.ProtocolFeeGasLimit}
 
 	// Create DeliverTx context (not CheckTx)
 	ctx := sdk.NewContext(nil, tmproto.Header{}, false, log.NewNopLogger())
@@ -224,3 +243,42 @@ func TestProtocolFeeTerminatorZeroFeeRejected(t *testing.T) {
 	require.Error(t, err)
 	require.ErrorContains(t, err, "protocol fee tx requires exactly one fee coin")
 }
+
+func TestProtocolFeeTerminatorRejectsFeeNotEqualToBalance(t *testing.T) {
+	balance := sdk.NewCoin(appconsts.BondDenom, math.NewInt(1000))
+	bankKeeper := &protoFeeMockBankKeeper{balance: balance}
+	decorator := ante.NewProtocolFeeTerminatorDecorator(bankKeeper)
+
+	msg := feeaddress.NewMsgPayProtocolFee()
+	// Fee is less than balance - should be rejected
+	fee := sdk.NewCoins(sdk.NewCoin(appconsts.BondDenom, math.NewInt(500)))
+	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: fee, gas: feeaddress.ProtocolFeeGasLimit}
+
+	// Create DeliverTx context (not CheckTx)
+	ctx := sdk.NewContext(nil, tmproto.Header{}, false, log.NewNopLogger())
+
+	_, err := decorator.AnteHandle(ctx, tx, false, protoFeeNextAnteHandler)
+
+	require.Error(t, err)
+	require.ErrorContains(t, err, "does not equal expected fee")
+}
+
+func TestProtocolFeeTerminatorRejectsWrongGasLimit(t *testing.T) {
+	balance := sdk.NewCoin(appconsts.BondDenom, math.NewInt(1000))
+	bankKeeper := &protoFeeMockBankKeeper{balance: balance}
+	decorator := ante.NewProtocolFeeTerminatorDecorator(bankKeeper)
+
+	msg := feeaddress.NewMsgPayProtocolFee()
+	fee := sdk.NewCoins(balance)
+	// Wrong gas limit - should be rejected
+	tx := &protoFeeMockFeeTx{msgs: []sdk.Msg{msg}, fee: fee, gas: feeaddress.ProtocolFeeGasLimit * 2}
+
+	// Create DeliverTx context (not CheckTx)
+	ctx := sdk.NewContext(nil, tmproto.Header{}, false, log.NewNopLogger())
+
+	_, err := decorator.AnteHandle(ctx, tx, false, protoFeeNextAnteHandler)
+
+	require.Error(t, err)
+	require.ErrorContains(t, err, "gas limit")
+	require.ErrorContains(t, err, "does not match expected")
+}

app/prepare_proposal.go

@@ -115,11 +115,21 @@ func (app *App) injectProtocolFeeTx(ctx sdk.Context, txs [][]byte) ([][]byte, sd
 	return append([][]byte{protocolFeeTx}, txs...), feeBalance, nil
 }
 
-// verifyProtocolFeeTxSurvived performs defense-in-depth verification that the fee
-// forward tx was not filtered out. If the fee address had a balance and we created
-// a protocol fee tx, it MUST be the first tx in the output. The protocol fee tx should
-// never be filtered since it has no signature requirements and uses minimal gas.
-// If it's filtered, there's a bug in the ante handler chain.
+// verifyProtocolFeeTxSurvived performs defense-in-depth verification that the protocol
+// fee tx was not filtered out by the ante handler chain.
+//
+// INVARIANT: If fee address had a balance, the protocol fee tx MUST be the first tx in output.
+//
+// The protocol fee tx should NEVER be filtered because:
+//   - No signatures to fail verification
+//   - Minimal gas (40k, well under any limit)
+//   - Positive fees
+//
+// If this check fails, it indicates a BUG in the ante handler chain, not normal operation.
+//
+// IMPACT: Returns error → PrepareProposal fails → this validator cannot propose this block.
+// This is NOT a chain halt. Other validators can still propose valid blocks.
+// Explicit failure is preferred over silently producing blocks that ProcessProposal will reject.
 //
 // If feeBalance is zero, this is a no-op (no protocol fee tx was injected).
 func (app *App) verifyProtocolFeeTxSurvived(feeBalance sdk.Coin, filteredTxs [][]byte) error {
@@ -131,11 +141,7 @@ func (app *App) verifyProtocolFeeTxSurvived(feeBalance sdk.Coin, filteredTxs [][
 		return fmt.Errorf("protocol fee tx was filtered out (no txs remain); fee_balance=%s", feeBalance.String())
 	}
 
-	_, firstTxIsProtocolFee, err := app.parseProtocolFeeTx(filteredTxs[0])
-	if err != nil {
-		return fmt.Errorf("failed to parse protocol fee tx: %w; fee_balance=%s", err, feeBalance.String())
-	}
-	if !firstTxIsProtocolFee {
+	if !feeaddress.IsProtocolFeeTxBytes(app.encodingConfig.TxConfig.TxDecoder(), filteredTxs[0]) {
 		return fmt.Errorf("protocol fee tx was filtered out unexpectedly; fee_balance=%s", feeBalance.String())
 	}