feat: ABCI 1.0 baseapp integration #13453

kocubinski · 2022-10-05T17:42:16Z

Description

Summary of changes

mempool/tx: Remove mempool.Tx interface and notion of transaction size. Byte counting logic pulled up to baseapp.
mempool: Update mempool.Select to use iterator semantics instead of enumerating the mempool all at once.
mempool: Add tests with production transactions exercising multi signer logic.
baseapp: Implementation of PrepareProposal and ProcessProposal (ABCI 1.0) with default app-side mempool.

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)

…k into kocubinski/mempool-impl

…empool-impl

* feat(mempool): tests and benchmark for mempool * added base for select benchmark * added simple select mempool test * added select mempool test * insert 100 and 1000 * t * t * t * move mempool to its own package and fix a small nil pointer error * Minor bug fixes for the sender pool; * minor fixes to consume * test * t * t * t * added gen tx with order * t

…k into kocubinski/mempool-impl

alexanderbez

Looks great! I have no major changes to recommend. There were a few minor nit comments.

Only outstanding item seems to be around the semantics of DefaultProcessProposal and how to handle invalid txs.

baseapp/abci.go

baseapp/baseapp.go

x/distribution/types/codec.go

baseapp/baseapp.go

alexanderbez · 2022-11-07T16:54:22Z

baseapp/baseapp.go

-				return err
+			_, _, _, _, err = app.runTx(runTxProcessProposal, txBytes)
+			if err != nil {
+				err = app.mempool.Remove(tx)


Ahh right, so when a honest process receives a proposal and executes it, any txs that are deemed invalid should still be included in the proposal. I think this is what @sergio-mena is alluding to. In other words, we should not reject or remove the tx from the mempool.

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

baseapp/abci.go

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

alexanderbez

Giving an ACK here. But we do need to undo the changes to x/distribution/types/codec.go

baseapp/abci.go

baseapp/baseapp.go

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

prettymuchbryce · 2022-11-08T23:02:10Z

baseapp/baseapp.go

+//
+// If any transaction fails to pass either condition, the proposal is rejected.
+func (app *BaseApp) DefaultProcessProposal() sdk.ProcessProposalHandler {
+	return func(ctx sdk.Context, req abci.RequestProcessProposal) abci.ResponseProcessProposal {


Food for thought: As an initial implementation for ABCI 1.0 why not just always return abci.ResponseProcessProposal_ACCEPT?

Some benefits:

It gives applications the exact same behavior that exists in the SDK today, therefore reduces potential for any liveness risks or bugs.

It's simpler and easier for you to test.

It's more performant.

My understanding is that the introduction of ProcessProposal was not intended specifically to solve some existing consensus issue on Cosmos SDK chains, rather it was introduced to give application developers flexibility (like us at dYdX) to reject proposals. So even if your default implementation always returns abci.ResponseProcessProposal_ACCEPT, you are still fulfilling the promise of ProcessProposal by allowing application developers to optionally implement their own logic via SetProcessProposal.

If in the future if there emerges a strong case to return REJECT in DefaultProcessProposal (i.e. if you want to validate that the transactions in a block do not exceed MaxGas, or something like that), then you could easily introduce such changes iteratively in a future version of the SDK.

Running every transaction through CheckTx seems a bit extreme to me and it's not clear that it solves any specific problem that exists in any existing chain today, however I could be ignorant or wrong about this. It also just actively makes upgrades more risky. For example, is it possible that some existing chain could have added some custom AnteHandler which doesn't necessarily expect to be invoked during ProcessProposal? Is this something that developers upgrading to v0.47 be aware of and audit in their codebases when upgrading?

In any case, dYdX will be using SetProcessProposal so the default behavior is of no concern to us. This is merely my own personal opinion on a more iterative upgrade path for the SDK that reduces some of the surface area while still retaining the most important feature (the ability for developers to customize ProcessProposal via SetProcessProposal).

Thanks @prettymuchbryce you make some good points. I've captured them in #13810 for further review and implementation. If we decide it's something we want @JeancarloBarrios or I can knock it out pretty quickly. At this moment to change direction would mean a thoughtful rewrite. Perhaps it's warranted but this PR was opened Oct 5 so we feel this iteration is good enough for an MVP. Let's continue the discussion! 🙏

That makes sense. Thanks for humoring me! 😄

prettymuchbryce · 2022-11-08T23:20:07Z

baseapp/abci.go

+
+		// NOTE: runTx was already run in CheckTx which calls mempool.Insert so ideally everything in the pool
+		// should be valid. But some mempool implementations may insert invalid txs, so we check again.
+		_, _, _, _, err = app.runTx(runTxPrepareProposal, bz)


I don't necessarily understand the point of forcing the runTx behavior on all app-side mempool implementations. If anything, this just makes the SetMempool API more restrictive. It also forces the performance burden on all custom app-side mempools of running all transactions through the Antehandlers a third time (CheckTx, DeliverTx, and now PrepareProposal). As stated in the comment here this shouldn't be necessary unless the application developer's custom app-side mempool implementation has a bug or is trying to do something unexpected. They can already break themselves in other ways, for example a developer could add some panic to their Select implementation which means no proposal would ever be sent, right?

Just a suggestion and not a dealbreaker, but I think I'd prefer to be able to opt out of this somehow.

AnteHandlers add no performance burden -- that's the entire point of them. Lightweight checks prior to tx execution.

We add validity checks here stated per the ADR. In addition, it's possible for an app to include/inject txs on Insert during CheckTx so this ensures any injected txs are still valid.

I disagree that they add no performance burden. Some of the checks read from state which could potentially mean disk reads if the state is not in cache. There is also signature verification which is not free.

A couple of examples of reading from state during Antehandlers:

Reading the Account in ConsumeTxSizeGasDecorator here.

Calling SendCoinsFromAccountToModule in DeductFees which reads module accounts from state here.

There is also signature verifications here which looks to take ~0.15ms per-op for secp256k1 if I am understanding the benchmarks correctly (Sig/secp256k1-8). This means while ABCI is globally locked during PrepareProposal, you're doing this operation for each transaction serially. So if the block has 100 transactions on average, you're spending ~15ms on average per-block blocking the entire application and verifying transaction signatures that have already been verified.

Let me know if I'm misunderstanding something!

I disagree that they add no performance burden. Some of the checks read from state which could potentially mean disk reads if the state is not in cache. There is also signature verification which is not free.

True, state is read from but not written to. In the grand scheme of things, the performance overhead is near negligible.

True, state is read from but not written to. In the grand scheme of things, the performance overhead is near negligible.

Just to be super clear, 15 milliseconds of additional global lock time per-block (as in my above signature example) is definitely not negligible for us, especially given this is totally unnecessary computation in our case. It's very likely we're a unique case given our throughput aspirations so therefore your assessment is probably true for most other chains.

agree with @prettymuchbryce, on profiles I have run on live networks ante handlers show up quite often and do add a non negligible amount of overhead. For many chains this isn't a problem but it's not something we should over look as we are trying to make the sdk more performant.

Again, we validate the txs (i.e. AnteHandler) since applications can insert txs on Insert during CheckTx and we need to ensure these are still valid when selecting during PrepareProposal.

Also, recall:

The application defines the AnteHandler chain

The application and thus it's AnteHandler is aware of tx execution context (i.e. PrepareProposal)

So if this doesn't suite your needs, your AnteHandler chain can easily do something like "If PrepareProposal, skip XYZ".

The application defines the AnteHandler chain
So if this doesn't suite your needs, your AnteHandler chain can easily do something like "If PrepareProposal, skip XYZ".

This is a really good point that I had forgotten. I think this would be reasonable solution.

ttl33 · 2022-11-09T19:07:28Z

baseapp/abci.go

+		} else if byteCount += txSize; byteCount <= req.MaxTxBytes {
+			txsBytes = append(txsBytes, bz)
+		} else {
+			break


Food for thought:

This else block seems to suggest that the txs queued up in the iterator won't be part of the returned response once the MaxTxBytes has been reached. Not processing all the queued up txs is not desirable in certain app scenarios.

For example, let's say Select returns txs [A1, A2, B1, B2] but PrepareProposal only processes [A1, A2] and leaves out [B1, B2], because the MaxTxBytes has been reached. Select returned the txs in the above order, because the app has an invariant that txs of type A must appear before txs of type B, but from the app's perspective, txs of type B are much more important to include in the block than type A. So in this case, the invariant is respected, but the app doesn't get to include txs of type B and is not alerted about this behavior.

Thinking out loud, to prevent this issue, the app can validate that the txs returned in Select is under the MaxTxBytes cap. However, the app doesn't get to learn if all the txs returned in Select were processed or not (it speculates that the txs were processed by PrepareProposal, but there's no explicit signal).

Thinking out loud, some suggestions:

Hint MaxTxBytes as part of Select

Add a way for the app to learn that not all txs returned in Select were processed by PrepareProposal and retry Select / PrepareProposal

Add a mode where "process all txs returned by Select or fail"

The reason we don't pass MaxBytes to Select is that we need to ensure that selected txs are valid.

We discussed this further offline. #13831 fixes the concern.

ttl33 · 2022-11-09T19:16:06Z

baseapp/baseapp.go

@@ -507,16 +550,25 @@ func validateBasicTxMsgs(msgs []sdk.Msg) error {
 // Returns the application's deliverState if app is in runTxModeDeliver,


nit: should this comment get updated?

ttl33 · 2022-11-09T19:17:54Z

baseapp/baseapp.go

+		return app.prepareProposalState
+	case runTxProcessProposal:
+		return app.processProposalState
+	default:


nit: should these cases explicitly switch on each of the mode values and have the default panic to catch any unhandled modes?

ttl33 · 2022-11-09T19:20:25Z

baseapp/baseapp.go

 		if err != nil {
 			return gInfo, nil, anteEvents, priority, err
 		}
+	} else if mode == runTxModeDeliver {


for my own understanding, why is it okay to not handle other types of runTxMode?

We remove during DeliverTx (stated in ADR).

ttl33 · 2022-11-09T19:26:07Z

types/mempool/mempool.go

-	// mempool, up to maxBytes or until the mempool is empty. The application can
-	// decide to return transactions from its own mempool, from the incoming
-	// txs, or some combination of both.
-	Select(txs [][]byte, maxBytes int64) ([]Tx, error)


Related to the above comment about MaxTxBytes, I see that the previous impl hints the max bytes. Why is this removed? Was it because: "with iterators, PrepareProposal can handle the cutoff point"?

Exactly right.

kocubinski and others added 30 commits September 13, 2022 10:03

size impl

d72d1a2

cleanup

01ed963

fix nill pointer when checking if key exists

35ff6d6

add init of hashes

bbd9405

notes

3cc3fb1

Merge branch 'kocubinski/mempool-impl' of github.com:cosmos/cosmos-sd…

9ffa994

…k into kocubinski/mempool-impl

Merge branch 'main' of github.com:cosmos/cosmos-sdk into kocubinski/m…

aa103ef

…empool-impl

btree ordering

77a9b37

MauricGit/skiplist impl

46c34ff

add huandu skiplist impl

9a4eeac

very rough draft of stateful ordering

09f4e6b

some notes

a6fb1e5

fill out remove method

c8931c9

some comments

c3746c7

mempool to own package

816dc70

stateful mempool test passes

85c5f1e

dfs impl in graph test passing

a10b047

comment

59b2195

add test case

aafb4ce

runtime edge detection is a bad idea

1798d68

Refactor mempool impl

5f2b9cc

Merge branch 'kocubinski/mempool-impl' of github.com:cosmos/cosmos-sd…

14afa8d

…k into kocubinski/mempool-impl

tx generation

ee97a25

shuffle return

bfda138

transaction generating

ee2ed7e

clean up tx order tests

afc04eb

generative tests are passing

a9a8fa3

add comment

9556eff

more iterations

87bf3b6

Apply suggestions from code review

1e36da8

alexanderbez reviewed Nov 7, 2022

View reviewed changes

JeancarloBarrios and others added 9 commits November 7, 2022 10:58

Update baseapp/baseapp.go

a3bc195

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

duplicate ProcessProposal request fields into context object

ccc00f1

Merge branch 'main' into kocubinski/mempool-all

5595988

Merge branch 'main' into kocubinski/mempool-all

26641b6

debug and fix bug surfaced in liveness tests

061e68e

Merge branch 'main' into kocubinski/mempool-all

e646e46

init prepare/process proposal state in .Init

e594b59

reject when checktx fails on process propossal

40a5b21

reject when checktx fails on process propossal

e50ef7d

alexanderbez reviewed Nov 8, 2022

View reviewed changes

baseapp/abci.go Outdated Show resolved Hide resolved

kocubinski and others added 4 commits November 8, 2022 11:40

Add 2 comments

5ce1d3a

Merge branch 'main' into kocubinski/mempool-all

97f02e9

Update baseapp/abci.go

c10e857

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

fix comment

323b9f7

alexanderbez approved these changes Nov 8, 2022

View reviewed changes

baseapp/abci.go Outdated Show resolved Hide resolved

baseapp/baseapp.go Outdated Show resolved Hide resolved

alexanderbez and others added 3 commits November 8, 2022 15:27

Update baseapp/abci.go

3575579

Merge branch 'main' into kocubinski/mempool-all

348f67e

Update baseapp/baseapp.go

4576305

Co-authored-by: Aleksandr Bezobchuk <[email protected]>

prettymuchbryce reviewed Nov 8, 2022

View reviewed changes

kocubinski mentioned this pull request Nov 9, 2022

Default Prepare/Process proposal to always ACCEPT #13810

Closed

Merge branch 'main' into kocubinski/mempool-all

61e1b60

alexanderbez approved these changes Nov 9, 2022

View reviewed changes

kocubinski merged commit 61effe8 into main Nov 9, 2022

kocubinski deleted the kocubinski/mempool-all branch November 9, 2022 15:50

ttl33 reviewed Nov 9, 2022

View reviewed changes

ttl33 mentioned this pull request Nov 10, 2022

Proof-of-Concept: Add alt options to PrepareProposal #13818

Closed

alexanderbez mentioned this pull request Nov 12, 2022

docs: ADR-060 Updates #13844

Merged

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023

feat: ABCI 1.0 baseapp integration (cosmos#13453)

8c343fb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ABCI 1.0 baseapp integration #13453

feat: ABCI 1.0 baseapp integration #13453

kocubinski commented Oct 5, 2022 •

edited

Loading

alexanderbez left a comment

alexanderbez Nov 7, 2022

alexanderbez left a comment

prettymuchbryce Nov 8, 2022 •

edited

Loading

kocubinski Nov 9, 2022 •

edited

Loading

prettymuchbryce Nov 9, 2022

prettymuchbryce Nov 8, 2022

alexanderbez Nov 9, 2022

prettymuchbryce Nov 9, 2022

alexanderbez Nov 9, 2022

prettymuchbryce Nov 9, 2022

tac0turtle Nov 9, 2022 •

edited

Loading

alexanderbez Nov 9, 2022

prettymuchbryce Nov 9, 2022

ttl33 Nov 9, 2022

alexanderbez Nov 9, 2022

ttl33 Nov 10, 2022

ttl33 Nov 9, 2022

ttl33 Nov 9, 2022

ttl33 Nov 9, 2022

alexanderbez Nov 9, 2022

ttl33 Nov 9, 2022

kocubinski Nov 9, 2022

		@@ -507,16 +550,25 @@ func validateBasicTxMsgs(msgs []sdk.Msg) error {
		// Returns the application's deliverState if app is in runTxModeDeliver,

feat: ABCI 1.0 baseapp integration #13453

feat: ABCI 1.0 baseapp integration #13453

Conversation

kocubinski commented Oct 5, 2022 • edited Loading

Description

Summary of changes

Author Checklist

Reviewers Checklist

alexanderbez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez left a comment

Choose a reason for hiding this comment

prettymuchbryce Nov 8, 2022 • edited Loading

Choose a reason for hiding this comment

kocubinski Nov 9, 2022 • 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

tac0turtle Nov 9, 2022 • 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

kocubinski commented Oct 5, 2022 •

edited

Loading

prettymuchbryce Nov 8, 2022 •

edited

Loading

kocubinski Nov 9, 2022 •

edited

Loading

tac0turtle Nov 9, 2022 •

edited

Loading