feat: Optimistic Execution by facundomedica · Pull Request #16581 · cosmos/cosmos-sdk

facundomedica · 2023-06-15T19:51:14Z

Description

RFC: #16499
Closes: #XXXX

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)

facundomedica · 2023-06-30T10:20:05Z

I think we need to be especially careful when OE is aborted, because finalizeBlockState could be left in a bad state.

InitChain happens, finalizeBlockState is not nil
During FinalizeBlock we check if finalizeBlockState == nil, otherwise we re-use it (because we want to be able to access any state written during InitChain)
If we abort, we need to be able to reset to the state that we had right after InitChain.

Maybe to keep it simple we can execute OE only if height > initialHeight. That way we can just clear up the state 100% of the times

github-actions · 2023-07-20T12:58:35Z

 	// processed the first block, as we want to avoid overwriting the finalizeState
 	// after state changes during InitChain.
 	if req.Height > app.initialHeight {
+		// abort any running OE
+		if app.oeEnabled && app.oeInfo != nil && app.oeInfo.Running() {
+			app.oeInfo.Abort()
+			_, _ = app.oeInfo.WaitResult() // ignore the result


Change potentially affects state.

Call sequence:

(*github.com/cosmos/cosmos-sdk/baseapp.BaseApp).ProcessProposal (baseapp/abci.go:467)

+		oe.logger.Error("OE aborted due to hash mismatch", "oe_hash", hex.EncodeToString(oe.request.Hash), "req_hash", hex.EncodeToString(reqHash), "oe_height", oe.request.Height, "req_height", oe.request.Height)
+		oe.cancelFunc()
+		return true
+	} else if oe.abortRate > 0 && rand.Intn(100) < oe.abortRate {


facundomedica · 2023-08-27T11:54:05Z

I tried cleaning up the concurrency model with little success, now I'm passing a context to internalFinalizeBlock which makes it unaware of the whole OE thing, it uses this context just to know if it needs to return early.

Using channels is a cleaner way to do this, but the problem is that the function we are passing as our FinalizeBlockFn has access to baseapp and to its properties, so we have partial control over what's going on with the state. This ended up in getting wrong app hashes from time to time.

A great improvement would be to have FinalizeBlock to use only whatever is passed to it and with little to no access to baseapp. This way we could run the entire block completely concurrently by passing only cached copies of the contexts and values; and by the time we abort we just discard all of these.
On the other hand, this could complicated this a bunch given that we'd need to make sure no other process modifies the base contexts and values being passed. Also I don't know if it's possible at all without a major refactor on baseapp

+	go func() {
+		start := time.Now()
+		resp, err := oe.finalizeBlockFunc(ctx, oe.request)
+		oe.mtx.Lock()
+		executionTime := time.Since(start)
+		oe.logger.Debug("OE finished", "duration", executionTime.String(), "height", req.Height, "hash", hex.EncodeToString(req.Hash))
+		oe.response, oe.err = resp, err
+		close(oe.stopCh)
+		oe.mtx.Unlock()
+	}()


+	oe.initialized = true
+
+	go func() {
+		start := time.Now()


Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

facundomedica · 2023-09-09T05:38:56Z

I've added a check so we don't start optimistic execution when ProcessProposal returns a "reject"

julienrbrt

mostly doc nits.

julienrbrt · 2023-09-15T16:26:56Z

 		voteExtHandler.SetHandlers(bApp)
 	}
-	baseAppOptions = append(baseAppOptions, voteExtOp)
+	baseAppOptions = append(baseAppOptions, voteExtOp, baseapp.SetOptimisticExecution())


Is it purposely done so that simapp only uses optimistic execution?
Or should we add it here:

cosmos-sdk/server/util.go

Line 529 in e2637b8

baseapp.SetQueryGasLimit(cast.ToUint64(appOpts.Get(FlagQueryGasLimit))),

?

So I wouldn't like to add a flag for this, because that could give node operators the chance of not running optimistic execution if the developers chose to do so (they can still modify the code but that takes some extra effort).
Could we add a flag on simapp only? wdyt?

Yeah, a flag wouldn't make sense.
I was taking about adding it in our default baseapp options.
But I think I remember conversation about having it disabled by default first 🤔, so it makes sense to add it only in simapp.

julienrbrt

utACK!

tac0turtle · 2023-10-23T10:36:33Z

@Mergifyio backport release/v0.50.x

mergify · 2023-10-23T10:36:37Z

backport release/v0.50.x

✅ Backports have been created

Details

#18205 feat: Optimistic Execution (backport #16581) has been created for branch release/v0.50.x but encountered conflicts

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> (cherry picked from commit 8df065b) # Conflicts: # CHANGELOG.md # baseapp/abci_test.go

Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Co-authored-by: Facundo <facundomedica@gmail.com>

alexanderbez · 2023-11-11T22:48:29Z

Great work @facundomedica 👏

facundomedica added 2 commits June 15, 2023 21:50

feat: Optimistic Execution

a57c937

fix panic recovery

023256e

github-actions Bot added the C:x/gov label Jun 15, 2023

remove test changes

14b80c4

github-actions Bot removed the C:x/gov label Jun 15, 2023

github-advanced-security AI found potential problems Jun 15, 2023

View reviewed changes

Comment thread baseapp/abci.go Fixed

facundomedica added 3 commits June 16, 2023 20:44

Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into facu/oe

47b8a1c

fix test

deaf6b7

make comet panic instead of sdk

f30e4a7

github-advanced-security AI found potential problems Jun 16, 2023

View reviewed changes

Comment thread baseapp/abci.go Fixed

facundomedica added 3 commits June 20, 2023 16:34

add abort channel

573d107

fix abort

17b5ca4

clean up phase1

d371c16

github-advanced-security AI found potential problems Jun 20, 2023

View reviewed changes

Comment thread baseapp/optimistic_execution.go Fixed

merge

c9dbc9a

facundomedica added 4 commits June 30, 2023 12:58

testing testing

20f0325

Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into facu/oe

6aec99a

merge main

e920201

progress

b855c1a

github-actions Bot reviewed Jul 20, 2023

View reviewed changes

github-advanced-security AI found potential problems Jul 20, 2023

View reviewed changes

Comment thread baseapp/optimistic_execution.go Fixed

github-advanced-security AI found potential problems Jul 20, 2023

View reviewed changes

Comment thread baseapp/abci.go Fixed

Comment thread baseapp/optimistic_execution.go Fixed

Comment thread baseapp/optimistic_execution.go Fixed

facundomedica and others added 4 commits July 21, 2023 14:16

fix

265e32d

Merge branch 'main' of https://github.com/cosmos/cosmos-sdk into facu/oe

2830366

progress

c835fa7

Merge branch 'main' into facu/oe

b26cfe8

github-advanced-security AI found potential problems Jul 27, 2023

View reviewed changes

Comment thread baseapp/oe/optimistic_execution.go Fixed

github-advanced-security AI found potential problems Jul 27, 2023

View reviewed changes

Comment thread baseapp/oe/optimistic_execution.go Fixed

Comment thread baseapp/oe/optimistic_execution.go Fixed

lint

06cb990

github-advanced-security AI found potential problems Aug 27, 2023

View reviewed changes

alexanderbez approved these changes Aug 28, 2023

View reviewed changes

Comment thread baseapp/abci.go Outdated

facundomedica and others added 2 commits August 29, 2023 12:18

Update baseapp/abci.go

4f90f04

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

only start optimistic execution if processProposal resp is accepted

2b574d5

Merge branch 'main' into facu/oe

f91b715

julienrbrt reviewed Sep 15, 2023

View reviewed changes

facundomedica added 5 commits September 18, 2023 09:58

godoc + tests

1c4743a

add file

8cda1f1

cl++

0065196

cl++

9d6c8b1

lint

8bdd23d

julienrbrt approved these changes Sep 18, 2023

View reviewed changes

facundomedica added this pull request to the merge queue Sep 18, 2023

Merged via the queue into main with commit 8df065b Sep 18, 2023

facundomedica deleted the facu/oe branch September 18, 2023 14:12

mergify Bot pushed a commit that referenced this pull request Oct 23, 2023

feat: Optimistic Execution (#16581)

b52dba1

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> (cherry picked from commit 8df065b) # Conflicts: # CHANGELOG.md # baseapp/abci_test.go

mergify Bot mentioned this pull request Oct 23, 2023

feat: Optimistic Execution (backport #16581) #18205

Merged

This comment was marked as spam.

Sign in to view

julienrbrt pushed a commit that referenced this pull request Oct 23, 2023

feat: Optimistic Execution (backport #16581) (#18205)

42dbfc4

Co-authored-by: Facundo Medica <14063057+facundomedica@users.noreply.github.com> Co-authored-by: Facundo <facundomedica@gmail.com>

This comment was marked as spam.

Sign in to view

This comment was marked as duplicate.

Sign in to view

byte-bandit mentioned this pull request Jan 11, 2024

Optimistic Execution VolumeFi/paloma#1162

Closed

faddat mentioned this pull request Nov 8, 2024

faddat/many times #22476

Closed

12 tasks

Conversation

facundomedica commented Jun 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Author Checklist

Reviewers Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

facundomedica commented Jun 30, 2023

Uh oh!

github-actions Bot Jul 20, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check failure

facundomedica commented Aug 27, 2023

Uh oh!

Check notice

Check warning

Uh oh!

facundomedica commented Sep 9, 2023

Uh oh!

julienrbrt left a comment

Choose a reason for hiding this comment

Uh oh!

julienrbrt Sep 15, 2023

Choose a reason for hiding this comment

Uh oh!

facundomedica Sep 18, 2023

Choose a reason for hiding this comment

Uh oh!

julienrbrt Sep 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

julienrbrt left a comment

Choose a reason for hiding this comment

Uh oh!

tac0turtle commented Oct 23, 2023

Uh oh!

mergify Bot commented Oct 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✅ Backports have been created

Uh oh!

This comment was marked as spam.

Uh oh!

This comment was marked as spam.

This comment was marked as duplicate.

Uh oh!

alexanderbez commented Nov 11, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

facundomedica commented Jun 15, 2023 •

edited

Loading

julienrbrt Sep 18, 2023 •

edited

Loading

mergify Bot commented Oct 23, 2023 •

edited

Loading