[CORE-538] Apply dYdX patches to Cosmos SDK 0.50.1 fork #30

lcwik · 2023-12-04T19:48:27Z

Description

Apply dYdX patches to Cosmos SDK 0.50.1 fork

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

* Fix lint/gocritic error * Disable some workflow jobs

github-actions · 2023-12-04T19:48:47Z

@lcwik your pull request is missing a changelog!

linear · 2023-12-04T20:46:51Z

CORE-538 Upgrade to Cosmos 0.50.0 (when available) and CometBFT 0.38.0

This change pushes the mutex that was in the local client to the top level of the ABCI methods and uses the unsynchronized local client. A future change is intended to reduce the critical sections of the various ABCI methods. We also replace cometbft usage with dYdX fork.

teddyding · 2023-12-06T17:05:41Z

baseapp/baseapp.go

@@ -184,6 +186,9 @@ type BaseApp struct {
 	// including the goroutine handling.This is experimental and must be enabled
 	// by developers.
 	optimisticExec *oe.OptimisticExecution
+
+	// Used to synchronize the application when using an unsynchronized ABCI++ client.
+	mtx sync.Mutex


Question on DeliverTx:

I see that on v0.47 fork we take the app.mtx lock

cosmos-sdk/baseapp/abci.go

Lines 418 to 429 in bdf96fd

func (app *BaseApp) DeliverTx(req abci.RequestDeliverTx) (res abci.ResponseDeliverTx) {

return app.DeliverTxShouldLock(req, true)

}

// DeliverTxShouldLock enables control of whether the lock should be acquired. The golang mutex

// is not reentrant so we enable conditional locking to prevent deadlock since cosmos-sdk/x/genutil.InitGenesis

// invokes DeliverTx from the ABCI++ InitGenesis method which already holds the lock.

func (app *BaseApp) DeliverTxShouldLock(req abci.RequestDeliverTx, needsLock bool) (res abci.ResponseDeliverTx) {

if needsLock {

app.mtx.Lock()

defer app.mtx.Unlock()

}

in DeliverTx. How has this changed in v0.50? This is the corresponding deliverTx function - is it intended to not take the lock?

Cosmos was re-entrant when delivering txs during block commit which is why we needed the ability to selectively take the lock. CometBFT pushed down block commit logic of individually calling DeliverTx into FinalizeBlock which means that we could elevate the need for acquiring the lock to FinalizeBlock removing it from deliverTx which also meant that we didn't care any more that Cosmos was re-entrant for this method or not.

Thanks for the explanation. I see from here tha BeginBlock, DeliverTx, and EndBlock are coalesced into FinalizeBlock

teddyding · 2023-12-06T17:10:16Z

baseapp/abci.go

@@ -901,6 +937,9 @@ func (app *BaseApp) checkHalt(height int64, time time.Time) error {
 // against that height and gracefully halt if it matches the latest committed
 // height.
 func (app *BaseApp) Commit() (*abci.ResponseCommit, error) {
+	app.mtx.Lock()
+	defer app.mtx.Unlock()
+
 	header := app.finalizeBlockState.ctx.BlockHeader()
 	retainHeight := app.GetBlockRetentionHeight(header.Height)



Question: do we still need the commiter logic from here? Or is that replaced by the prepareCheckStater below?

It is replaced by prepareCheckStater which @prettymuchbryce pushed upstream as the replacement for the committer logic.

teddyding · 2023-12-06T17:18:15Z

baseapp/baseapp.go

+
+	return gInfo, result, anteEvents, err
+}
+
 // runTx processes a transaction within a given execution mode, encoded transaction
 // bytes, and the decoded transaction itself. All state transitions occur through
 // a cached Context depending on the mode provided. State only gets persisted


Question for runTx() below: do we still need these checks?

Yes, added it back and updated the [STAB-19] Reduce the size of the critical section commit to contain it to keep the history clean.

teddyding · 2023-12-06T17:30:17Z

To confirm, do we still need to port over these changes from the v0.47 fork?

teddyding · 2023-12-06T17:31:46Z

For my understanding, how do we plan to test the upgrade from v0.47 fork to v0.50 fork? Involves a lot of changes all over the places, and it seems easy to miss something to port over.

lcwik · 2023-12-06T17:43:08Z

To confirm, do we still need to port over these changes from the v0.47 fork?

Just added these now to this PR.

lcwik · 2023-12-06T17:46:19Z

For my understanding, how do we plan to test the upgrade from v0.47 fork to v0.50 fork? Involves a lot of changes all over the places, and it seems easy to miss something to port over.

This is TBD, my thoughts around this are:

ensure existing unit tests pass
deploy to dev to ensure it continues to run
perform upgrade testing (either using the upgrade test framework if it exists or performing upgrades in a persistent environment (dev/staging/testnet))

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`

ctx.Done() support was added in 0.50 with cosmos#15041 server start up time was removed in 0.50 with cosmos#15041

lcwik · 2023-12-06T18:52:00Z

I was able to add the last of the remaining commits.

I flattened these two commits together (5402055, b95c66d).

And the CLOB-930 commit was able to be simplified a bunch because of changes that were released in 0.50 (cosmos#15041).

prettymuchbryce and others added 2 commits December 4, 2023 10:56

Update README with fork instructions

05df267

Enable CI workflows and fix failures (#4)

9510f1c

* Fix lint/gocritic error * Disable some workflow jobs

lcwik force-pushed the lukecore538 branch 2 times, most recently from 5f8013b to 21e77b6 Compare December 4, 2023 20:27

[STAB-4] Update version of CometBFT fork to use.

c0b38c4

lcwik force-pushed the lukecore538 branch from 21e77b6 to a6451fd Compare December 4, 2023 20:33

lcwik changed the title ~~Apply dYdX patches to Cosmos SDK 0.50.1 fork~~ [CORE-538] Apply dYdX patches to Cosmos SDK 0.50.1 fork Dec 4, 2023

lcwik force-pushed the lukecore538 branch from a6451fd to dccb1f1 Compare December 4, 2023 22:26

teddyding reviewed Dec 6, 2023

View reviewed changes

lcwik and others added 3 commits December 6, 2023 09:49

Update README.md with current default branch

c338245

change default commision rate and default commission max rate to 100%

9c28897

lcwik force-pushed the lukecore538 branch from dccb1f1 to 9c28897 Compare December 6, 2023 17:58

jiajames and others added 3 commits December 6, 2023 10:31

update LICENSE

02d061c

Remove in memory sink from fanout and add latency sampling methods

acaae16

[CLOB-930] Add support for domain sockets for gRPC server and gRPC.

89d0d11

ctx.Done() support was added in 0.50 with cosmos#15041 server start up time was removed in 0.50 with cosmos#15041

teddyding approved these changes Dec 6, 2023

View reviewed changes

lcwik merged commit 2768f92 into dydx-fork-v0.50.1 Dec 6, 2023
38 of 40 checks passed

lcwik deleted the lukecore538 branch December 6, 2023 19:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CORE-538] Apply dYdX patches to Cosmos SDK 0.50.1 fork #30

[CORE-538] Apply dYdX patches to Cosmos SDK 0.50.1 fork #30

lcwik commented Dec 4, 2023 •

edited

Loading

github-actions bot commented Dec 4, 2023

linear bot commented Dec 4, 2023

teddyding Dec 6, 2023 •

edited

Loading

lcwik Dec 6, 2023

teddyding Dec 6, 2023

teddyding Dec 6, 2023 •

edited

Loading

lcwik Dec 6, 2023

teddyding Dec 6, 2023

lcwik Dec 6, 2023

teddyding commented Dec 6, 2023

teddyding commented Dec 6, 2023

lcwik commented Dec 6, 2023 •

edited

Loading

lcwik commented Dec 6, 2023

lcwik commented Dec 6, 2023

	func (app *BaseApp) DeliverTx(req abci.RequestDeliverTx) (res abci.ResponseDeliverTx) {
	return app.DeliverTxShouldLock(req, true)
	}

	// DeliverTxShouldLock enables control of whether the lock should be acquired. The golang mutex
	// is not reentrant so we enable conditional locking to prevent deadlock since cosmos-sdk/x/genutil.InitGenesis
	// invokes DeliverTx from the ABCI++ InitGenesis method which already holds the lock.
	func (app *BaseApp) DeliverTxShouldLock(req abci.RequestDeliverTx, needsLock bool) (res abci.ResponseDeliverTx) {
	if needsLock {
	app.mtx.Lock()
	defer app.mtx.Unlock()
	}

[CORE-538] Apply dYdX patches to Cosmos SDK 0.50.1 fork #30

[CORE-538] Apply dYdX patches to Cosmos SDK 0.50.1 fork #30

Conversation

lcwik commented Dec 4, 2023 • edited Loading

Description

Author Checklist

Reviewers Checklist

github-actions bot commented Dec 4, 2023

linear bot commented Dec 4, 2023

teddyding Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

lcwik Dec 6, 2023

Choose a reason for hiding this comment

teddyding Dec 6, 2023

Choose a reason for hiding this comment

teddyding Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

lcwik Dec 6, 2023

Choose a reason for hiding this comment

teddyding Dec 6, 2023

Choose a reason for hiding this comment

lcwik Dec 6, 2023

Choose a reason for hiding this comment

teddyding commented Dec 6, 2023

teddyding commented Dec 6, 2023

lcwik commented Dec 6, 2023 • edited Loading

lcwik commented Dec 6, 2023

lcwik commented Dec 6, 2023

lcwik commented Dec 4, 2023 •

edited

Loading

teddyding Dec 6, 2023 •

edited

Loading

teddyding Dec 6, 2023 •

edited

Loading

lcwik commented Dec 6, 2023 •

edited

Loading