Merge pull request #415 from agoric-labs/jimlarson/sim-unit

fix: avoid duplicates in event history

CHANGELOG-Agoric.md

@@ -42,10 +42,12 @@ Ref: https://keepachangelog.com/en/1.0.0/
 * (auth) [#407](https://github.com/agoric-labs/cosmos-sdk/pull/407) Configurable fee collector module account in DeductFeeDecorator.
 * (server) [#409](https://github.com/agoric-labs/cosmos-sdk/pull/409) Flag to select ABCI client type.
 * (deps) [#412](https://github.com/agoric-labs/cosmos-sdk/pull/412) Bump iavl to v0.19.7
+* (baseapp) [#415](https://github.com/agoric-labs/cosmos-sdk/pull/415) Unit tests and documentation for event history.
 
 ### Bug Fixes
 
 * (baseapp) [#413](https://github.com/agoric-labs/cosmos-sdk/pull/413) Ignore events from simulated transactions
+* (baseapp) [#415](https://github.com/agoric-labs/cosmos-sdk/pull/415) Avoid duplicates in event history.
 
 ### API Breaking
 

baseapp/abci.go

@@ -203,9 +203,11 @@ func (app *BaseApp) EndBlock(req abci.RequestEndBlock) (res abci.ResponseEndBloc
 	}
 
 	if app.endBlocker != nil {
-		// Propagate the event history.
-		em := sdk.NewEventManagerWithHistory(app.deliverState.eventHistory)
-		res = app.endBlocker(app.deliverState.ctx.WithEventManager(em), req)
+		// [AGORIC] Propagate the event history.
+		enhancedEm := sdk.NewEventManagerWithHistory(app.deliverState.eventHistory)
+		enhancedCtx := app.deliverState.ctx.WithEventManager(enhancedEm)
+
+		res = app.endBlocker(enhancedCtx, req)
 		res.Events = sdk.MarkEventsToIndex(res.Events, app.indexEvents)
 	}
 
@@ -266,6 +268,17 @@ func (app *BaseApp) CheckTx(req abci.RequestCheckTx) abci.ResponseCheckTx {
 // Regardless of tx execution outcome, the ResponseDeliverTx will contain relevant
 // gas execution context.
 func (app *BaseApp) DeliverTx(req abci.RequestDeliverTx) (res abci.ResponseDeliverTx) {
+	// [AGORIC] Remember event history for successful deliveries.
+	// deliverTxWithoutEventHistory is the upstream cosmos-sdk DeliverTx.
+	res = app.deliverTxWithoutEventHistory(req)
+	// When successful, remember event history.
+	if res.Code == sdkerrors.SuccessABCICode {
+		app.deliverState.eventHistory = append(app.deliverState.eventHistory, res.Events...)
+	}
+	return res
+}
+
+func (app *BaseApp) deliverTxWithoutEventHistory(req abci.RequestDeliverTx) (res abci.ResponseDeliverTx) {
 	gInfo := sdk.GasInfo{}
 	resultStr := "successful"
 
@@ -290,14 +303,12 @@ func (app *BaseApp) DeliverTx(req abci.RequestDeliverTx) (res abci.ResponseDeliv
 		return sdkerrors.ResponseDeliverTxWithEvents(err, gInfo.GasWanted, gInfo.GasUsed, sdk.MarkEventsToIndex(anteEvents, app.indexEvents), app.trace)
 	}
 
-	events := sdk.MarkEventsToIndex(result.Events, app.indexEvents)
-	app.deliverState.eventHistory = append(app.deliverState.eventHistory, events...)
 	return abci.ResponseDeliverTx{
 		GasWanted: int64(gInfo.GasWanted), // TODO: Should type accept unsigned ints?
 		GasUsed:   int64(gInfo.GasUsed),   // TODO: Should type accept unsigned ints?
 		Log:       result.Log,
 		Data:      result.Data,
-		Events:    events,
+		Events:    sdk.MarkEventsToIndex(result.Events, app.indexEvents),
 	}
 }
 

baseapp/baseapp.go

@@ -1,7 +1,6 @@
 package baseapp
 
 import (
-	"context"
 	"errors"
 	"fmt"
 	"strings"
@@ -29,6 +28,7 @@ const (
 	runTxModeDeliver                   // Deliver a transaction
 )
 
+// [AGORIC] Context keys for including TX hash and msg index.
 const (
 	TxHashContextKey   = sdk.ContextKey("tx-hash")
 	TxMsgIdxContextKey = sdk.ContextKey("tx-msg-idx")
@@ -435,7 +435,7 @@ func (app *BaseApp) setCheckState(header tmproto.Header) {
 	app.checkState = &state{
 		ms:           ms,
 		ctx:          sdk.NewContext(ms, header, true, app.logger).WithMinGasPrices(app.minGasPrices),
-		eventHistory: []abci.Event{},
+		eventHistory: []abci.Event{}, // [AGORIC]: start with an empty history.
 	}
 }
 
@@ -448,7 +448,7 @@ func (app *BaseApp) setDeliverState(header tmproto.Header) {
 	app.deliverState = &state{
 		ms:           ms,
 		ctx:          sdk.NewContext(ms, header, false, app.logger),
-		eventHistory: []abci.Event{},
+		eventHistory: []abci.Event{}, // [AGORIC]: start with an empty history.
 	}
 }
 
@@ -607,11 +607,13 @@ func (app *BaseApp) getContextForTx(mode runTxMode, txBytes []byte) sdk.Context
 // cacheTxContext returns a new context based off of the provided context with
 // a branched multi-store.
 func (app *BaseApp) cacheTxContext(ctx sdk.Context, txBytes []byte) (sdk.Context, sdk.CacheMultiStore) {
+	// [AGORIC] Add Tx hash to the context if absent.
 	txHash, ok := ctx.Context().Value(TxHashContextKey).(string)
 	if !ok {
 		txHash = fmt.Sprintf("%X", tmhash.Sum(txBytes))
-		ctx = ctx.WithContext(context.WithValue(ctx.Context(), TxHashContextKey, txHash))
+		ctx = ctx.WithValue(TxHashContextKey, txHash)
 	}
+
 	ms := ctx.MultiStore()
 	// TODO: https://github.com/cosmos/cosmos-sdk/issues/2824
 	msCache := ms.CacheMultiStore()
@@ -767,9 +769,6 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re
 		if len(anteEvents) > 0 && (mode == runTxModeDeliver || mode == runTxModeSimulate) {
 			// append the events in the order of occurrence
 			result.Events = append(anteEvents, result.Events...)
-			if app.deliverState != nil && mode == runTxModeDeliver {
-				app.deliverState.eventHistory = append(app.deliverState.eventHistory, result.Events...)
-			}
 		}
 	}
 
@@ -784,7 +783,6 @@ func (app *BaseApp) runTx(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, re
 func (app *BaseApp) runMsgs(ctx sdk.Context, msgs []sdk.Msg, mode runTxMode) (*sdk.Result, error) {
 	msgLogs := make(sdk.ABCIMessageLogs, 0, len(msgs))
 	events := sdk.EmptyEvents()
-	historicalEvents := ctx.EventManager().GetABCIEventHistory()
 	var msgResponses []*codectypes.Any
 
 	// NOTE: GasWanted is determined by the AnteHandler and GasUsed by the GasMeter.
@@ -800,8 +798,9 @@ func (app *BaseApp) runMsgs(ctx sdk.Context, msgs []sdk.Msg, mode runTxMode) (*s
 			err          error
 		)
 
-		msgCtx := ctx.WithEventManager(sdk.NewEventManagerWithHistory(historicalEvents))
-		msgCtx = msgCtx.WithContext(context.WithValue(msgCtx.Context(), TxMsgIdxContextKey, i))
+		// [AGORIC] Propagate the message index in the context.
+		msgCtx := ctx.WithValue(TxMsgIdxContextKey, i)
+
 		if handler := app.msgServiceRouter.Handler(msg); handler != nil {
 			// ADR 031 request type routing
 			msgResult, err = handler(msgCtx, msg)
@@ -838,7 +837,6 @@ func (app *BaseApp) runMsgs(ctx sdk.Context, msgs []sdk.Msg, mode runTxMode) (*s
 		// Note: Each message result's data must be length-prefixed in order to
 		// separate each result.
 		events = events.AppendEvents(msgEvents)
-		historicalEvents = append(historicalEvents, msgEvents.ToABCIEvents()...)
 
 		// Each individual sdk.Result that went through the MsgServiceRouter
 		// (which should represent 99% of the Msgs now, since everyone should

baseapp/baseapp_test.go

@@ -1128,6 +1128,75 @@ func TestSimulateTx(t *testing.T) {
 	}
 }
 
+func TestEventHistory(t *testing.T) {
+	// generates events in the ante
+	anteKey := []byte("ante-key")
+	anteOpt := func(bapp *BaseApp) { bapp.SetAnteHandler(anteHandlerTxTest(t, capKey1, anteKey)) }
+
+	deliverKey := []byte("deliver-key")
+	routerOpt := func(bapp *BaseApp) {
+		r := sdk.NewRoute(routeMsgCounter, handlerMsgCounter(t, capKey1, deliverKey))
+		bapp.Router().AddRoute(r)
+	}
+
+	// expand setupBaseApp() here so we can override the EndBlocker before app is sealed
+	app := newBaseApp(t.Name(), anteOpt, routerOpt)
+	require.Equal(t, t.Name(), app.Name())
+
+	app.MountStores(capKey1, capKey2)
+	app.SetParamStore(&paramStore{db: dbm.NewMemDB()})
+
+	var history []abci.Event
+	app.SetEndBlocker(func(ctx sdk.Context, req abci.RequestEndBlock) abci.ResponseEndBlock {
+		history = ctx.EventManager().GetABCIEventHistory()
+		return abci.ResponseEndBlock{}
+	})
+
+	// stores are mounted (seals the app)
+	err := app.LoadLatestVersion()
+	require.Nil(t, err)
+
+	app.InitChain(abci.RequestInitChain{})
+
+	// Create same codec used in txDecoder
+	cdc := codec.NewLegacyAmino()
+	registerTestCodec(cdc)
+
+	header := tmproto.Header{Height: int64(1)}
+	app.BeginBlock(abci.RequestBeginBlock{Header: header})
+
+	require.Zero(t, len(app.deliverState.eventHistory))
+
+	counter := int64(0)
+	tx := newTxCounter(counter, counter)
+	txBytes, err := cdc.Marshal(tx)
+	require.NoError(t, err)
+
+	// simulation returns events, but none in deliverState history
+	_, simResult, err := app.Simulate(txBytes)
+	require.NoError(t, err)
+	require.NotNil(t, simResult)
+	require.Zero(t, len(app.deliverState.eventHistory))
+	expectedEvents := simResult.Events
+	require.NotZero(t, len(expectedEvents))
+
+	// delivery results should be reflected in deliverState history
+	deliverResult := app.DeliverTx(abci.RequestDeliverTx{Tx: txBytes})
+	require.True(t, deliverResult.IsOK(), fmt.Sprintf("%v", deliverResult))
+	require.NotZero(t, len(app.deliverState.eventHistory))
+	// simResult events are not indexed, but should otherwise be the same as deliverResult events
+	for i, _ := range simResult.Events {
+		simResult.Events[i].Attributes[0].Index = true
+	}
+	require.Equal(t, expectedEvents, deliverResult.Events)
+	require.Equal(t, expectedEvents, app.deliverState.eventHistory)
+
+	// the deliverState event history should be passed to the end blocker in the context, then cleared
+	app.EndBlock(abci.RequestEndBlock{})
+	require.Zero(t, len(app.deliverState.eventHistory))
+	require.Equal(t, expectedEvents, history)
+}
+
 func TestRunInvalidTransaction(t *testing.T) {
 	anteOpt := func(bapp *BaseApp) {
 		bapp.SetAnteHandler(func(ctx sdk.Context, tx sdk.Tx, simulate bool) (newCtx sdk.Context, err error) {

baseapp/state.go

@@ -6,8 +6,12 @@ import (
 )
 
 type state struct {
-	ms           sdk.CacheMultiStore
-	ctx          sdk.Context
+	ms  sdk.CacheMultiStore
+	ctx sdk.Context
+	// eventHistory accumulates events returned by DeliverTx throughout a block.
+	// [AGORIC] The accumulated events are passed to the EndBlocker in its context's
+	// EventManager ABCI event history, and the state's eventHistory is then cleared.
+	// Not used for modes or states other than delivery.
 	eventHistory []abci.Event
 }
 

types/events.go

@@ -24,10 +24,15 @@ import (
 // EventManager implements a simple wrapper around a slice of Event objects that
 // can be emitted from.
 type EventManager struct {
-	events  Events
+	events Events
+	// history holds the events from all transactions delivered in a block
+	// [AGORIC] Used to communicate the history through the context.
 	history []abci.Event
 }
 
+// NewEventManagerWithHistory returns a new event manager with empty events,
+// but seeded with the provided history of earlier events in the block.
+// [AGORIC] This should be used to create the EventManager for use in EndBlockers.
 func NewEventManagerWithHistory(history []abci.Event) *EventManager {
 	return &EventManager{
 		events:  EmptyEvents(),
@@ -39,8 +44,8 @@ func NewEventManager() *EventManager {
 	return NewEventManagerWithHistory([]abci.Event{})
 }
 
-// GetEventHistory returns a deep copy of the ABCI events that have been
-// committed to thus far.
+// GetABCIEventHistory returns a deep copy of the ABCI events history.
+// [AGORIC] This should only be called in EndBlock processing.
 func (em *EventManager) GetABCIEventHistory() []abci.Event {
 	history := make([]abci.Event, len(em.history))
 	for i, event := range em.history {

types/module/module.go

@@ -494,11 +494,14 @@ func (m *Manager) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) abci.R
 // child context with an event manager to aggregate events emitted from all
 // modules.
 func (m *Manager) EndBlock(ctx sdk.Context, req abci.RequestEndBlock) abci.ResponseEndBlock {
-	em := ctx.EventManager()
-	if em == nil {
-		em = sdk.NewEventManager()
+	// [AGORIC] Reset the EventManager, preserving any existing event history.
+	eventHistory := []abci.Event{}
+	if oldEm := ctx.EventManager(); oldEm != nil {
+		eventHistory = oldEm.GetABCIEventHistory()
 	}
-	ctx = ctx.WithEventManager(sdk.NewEventManagerWithHistory(em.GetABCIEventHistory()))
+	em := sdk.NewEventManagerWithHistory(eventHistory)
+
+	ctx = ctx.WithEventManager(em)
 	validatorUpdates := []abci.ValidatorUpdate{}
 
 	for _, moduleName := range m.OrderEndBlockers {