[STAB-19] Reduce the size of the critical section #16

lcwik · 2023-04-28T20:21:35Z

This change makes a copy of runTx and does the following:

moves transaction descoding and tx validation before lock acquisition
moves creation of the response after releasing the lock
removes support for the post handler (unused by dYdX) and extracts out the only code that is executed in runMsgs allowing us to avoid the creation of the runMsgCtx and its associated MultiStore
removes consumeBlockGas since it is only executed during deliverTx

Note that this change allowed for checkTx to scale up to ~600 place orders/second before running out of CPU on an m5d.large:

linear · 2023-04-28T20:21:37Z

STAB-19

jonfung-dydx · 2023-05-01T20:47:12Z

baseapp/baseapp.go

+			anteEvents = events.ToABCIEvents()
+		}
+
+		if mode == runTxModeCheck {


Suggestion: Given that this function only runs in CheckTx or RecheckTx, assert this condition at the beginning of the function and drop this if branch

We still need the call since recheckTx shouldn't insert the transaction into the mempool again but I will add the check to the top to ensure that this is used only for the appropriate modes.

This change makes a copy of `runTx` and does the following: * moves transaction descoding and tx validation before lock acquisition * moves creation of the response after releasing the lock * removes support for the post handler (unused by dYdX) and extracts out the only code that is executed in `runMsgs` allowing us to avoid the creation of the `runMsgCtx` and its associated `MultiStore` * removes `consumeBlockGas` since it is only executed during `deliverTx`

ttl33 · 2023-05-11T19:44:57Z

baseapp/baseapp.go

+// Note, gas execution info is always returned. A reference to a Result is
+// returned if the tx does not run out of gas and if all the messages are valid
+// and execute successfully. An error is returned otherwise.
+func (app *BaseApp) runCheckTxConcurrently(mode runTxMode, txBytes []byte) (gInfo sdk.GasInfo, result *sdk.Result, anteEvents []abci.Event, priority int64, err error) {


In general, I think there are duplicate code between this new method and the runTx method.

Is there room to refactor out common components? Or is the goal here to keep the runTx unmodified to keep the rebasing easier in the future? Or something else?

How do we plan to keep this method in sync w future updates to runTx?

In general, I think there are duplicate code between this new method and the runTx method.

Is there room to refactor out common components? Or is the goal here to keep the runTx unmodified to keep the rebasing easier in the future? Or something else?

The goal was to keep the rebasing simple and also to cut the function down to what is needed for dYdX v4.

How do we plan to keep this method in sync w future updates to runTx?

Our e2e tests that execute through the ABCI++ flow should cover most of our needs but an audit of the differences in runTx versions during upgrades would help until we have more e2e tests.

Makes sense.

audit of the differences in runTx

Do you think it's worth listing a checklist in our readme?

We also prob want to add an item to audit our app/ante impl, which we have been tracking as one offs using tickets like this, but I think it's better to create a checklist somewhere more visible

ttl33 · 2023-05-11T19:45:50Z

baseapp/baseapp.go

+	// NOTE: GasWanted should be returned by the AnteHandler. GasUsed is
+	// determined by the GasMeter. We need access to the context to get the gas
+	// meter, so we initialize upfront.
+	var gasWanted uint64


IIUC, gas is only considered when we are in deliverTx mode. Is this the correct understanding?

If so, do we need to do any gas related work here (i.e. setting gasWanted and recovery func below)?

IIUC, gas is only considered when we are in deliverTx mode. Is this the correct understanding?

We check that the transaction has enough gas during CheckTx and consume the gas during DeliverTx. We fail the CheckTx if it wouldn't have enough gas.

If so, do we need to do any gas related work here (i.e. setting gasWanted and recovery func below)?

Yes, we have to return any gas computations from CheckTx back to the caller.

ttl33 · 2023-05-11T19:48:32Z

baseapp/baseapp.go

+// bytes, and the decoded transaction itself. All state transitions occur through
+// a cached Context depending on the mode provided.
+//
+// Note, gas execution info is always returned. A reference to a Result is


gas execution info is always returned ... tx does not run out of gas

Is this true? i think gas doesn't matter unless we run DeliverTx mode and I think we only run this w checkTx or recheckTx. This comment doesn't seem super relevant

Yes since the ABCI CheckTx response returns gas wanted/gas used on it.

ttl33 · 2023-05-11T19:49:38Z

baseapp/baseapp.go

+		panic("runCheckTxConcurrently can only be invoked for CheckTx and RecheckTx.")
+	}
+
+	// Strip out the post handler


nit: can we add more comments why this is not supported?

ttl33 · 2023-05-11T19:54:23Z

baseapp/baseapp.go

+		if mode == runTxModeCheck {
+			err = app.mempool.Insert(ctx, tx)
+			if err != nil {
+				result = nil


do we also want to set anteEvents or priority to the default error value? if not, why?

This is because in the runTx version of this method they return the current value of these fields and only overwrite result with nil.

ttl33 · 2023-05-11T19:55:29Z

baseapp/baseapp.go

+		return gInfo, result, anteEvents, priority, sdkerrors.Wrap(err, "failed to marshal tx data")
+	}
+
+	msgLogs := make(sdk.ABCIMessageLogs, 0, len(msgs))


is this var always empty? if so, should we just set Log to an empty string?

It was to keep compatibility with the ABCIMessageLogs.String function incase if it was empty it would choose to return something other then an empty string (e.g. empty json list formatted as string).

can we perhaps add a comment?

ttl33 · 2023-05-11T19:56:53Z

baseapp/baseapp.go

+	result = &sdk.Result{
+		Data:         data,
+		Log:          strings.TrimSpace(msgLogs.String()),
+		Events:       sdk.EmptyEvents().ToABCIEvents(),


nit: can we add comments why we set the events to empty?

ttl33 · 2023-05-11T19:57:21Z

baseapp/baseapp.go

+		MsgResponses: msgResponses,
+	}
+
+	// We don't support the post handler specifically to avoid creating a branched MultiStore and since dYdX


nit: should we move this logic to line 625?

I originally had the comment here because this is where the PostHandler logic was in runTx

ttl33 · 2023-05-11T19:58:27Z

baseapp/baseapp.go

@@ -609,6 +609,141 @@ func (app *BaseApp) cacheTxContext(ctx sdk.Context, txBytes []byte) (sdk.Context
 	return ctx.WithMultiStore(msCache), msCache
 }

+// runCheckTxConcurrently processes a transaction with either the checkTx or recheckTx modes, encoded transaction


Do we have any tests for this function? Can we create a tech-debt ticket to track if you are working on higher priority item?

I specifically added the concurrent matches and cancels tests to cover most of the concurrent scenarios here, also updated the simulator to use this function over runTx, and we have some coverage from the CosmosSDKs tests themselves. I feel as though this would be sufficient but if you think there are certain scenarios you would want covered in addition to the above I would open up a ticket around this.

I see, I agree that this feels sufficient for now. Thanks for clarifying!

lcwik force-pushed the lukestab19 branch 2 times, most recently from 658fe28 to e30fcbc Compare May 1, 2023 20:16

lcwik marked this pull request as ready for review May 1, 2023 20:31

jonfung-dydx reviewed May 1, 2023

View reviewed changes

jonfung-dydx approved these changes May 1, 2023

View reviewed changes

lcwik force-pushed the lukestab19 branch from e30fcbc to ff03ddf Compare May 1, 2023 21:01

lcwik force-pushed the lukestab19 branch from ff03ddf to f6b9a2b Compare May 1, 2023 21:13

lcwik merged commit 6477dcf into dydx-fork-v0.47.1 May 1, 2023

lcwik deleted the lukestab19 branch May 1, 2023 21:58

ttl33 reviewed May 11, 2023

View reviewed changes

lcwik added a commit that referenced this pull request Aug 22, 2023

Address #16 PR comments

4009993

lcwik added a commit that referenced this pull request Aug 23, 2023

Address #16 PR comments

21517ec

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[STAB-19] Reduce the size of the critical section #16

[STAB-19] Reduce the size of the critical section #16

lcwik commented Apr 28, 2023 •

edited

Loading

linear bot commented Apr 28, 2023

jonfung-dydx May 1, 2023

lcwik May 1, 2023 •

edited

Loading

ttl33 May 11, 2023

lcwik May 11, 2023

ttl33 May 12, 2023

ttl33 May 11, 2023

lcwik May 11, 2023

ttl33 May 11, 2023

lcwik May 11, 2023

ttl33 May 11, 2023

lcwik May 11, 2023

ttl33 May 11, 2023

lcwik May 11, 2023

ttl33 May 11, 2023

lcwik May 11, 2023

ttl33 May 12, 2023

ttl33 May 11, 2023

lcwik May 11, 2023

ttl33 May 11, 2023

lcwik May 11, 2023

ttl33 May 11, 2023

lcwik May 11, 2023

ttl33 May 12, 2023

[STAB-19] Reduce the size of the critical section #16

[STAB-19] Reduce the size of the critical section #16

Conversation

lcwik commented Apr 28, 2023 • edited Loading

linear bot commented Apr 28, 2023

Choose a reason for hiding this comment

lcwik May 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lcwik commented Apr 28, 2023 •

edited

Loading

lcwik May 1, 2023 •

edited

Loading