feat: ABCI 1.0 Phase I Completion #382

alexanderbez · 2022-11-15T15:07:04Z

Changelog

Downstream changes from the core SDK repo that contains the necessary components for ABCI 1.0 phase I completion. This includes but is not limited to:
- PrepareProposal support
- ProcessProposal support
- Default nonce-based Mempool
- Default PrepareProposal handler
- Default ProcessProposal handler

alexanderbez · 2022-11-16T02:22:25Z

So this PR is essentially blocked on getting the repo migrated to use cosmos/gogoproto instead of the deprecated github.com/gogo/protobuf, due to Tendermint also migrating (v0.37.x).

I've started this work here: #383

Currently blocked on getting it across the finish line...

p0mvn

Great work!

I'm going to be reviewing in stages since the PR is large.

So far, reviewed everything on a high level and focused on the mempool in-detail. Still need to review mempool tests.

types/mempool/nonce.go

p0mvn · 2022-12-08T15:53:30Z

types/mempool/mempool.go

-	// txs, or some combination of both.
-	Select(ctx types.Context, txs [][]byte, maxBytes int) ([]Tx, error)
+	// Select returns an Iterator over the app-side mempool. If txs are specified,
+	// then they shall be incorporated into the Iterator. The Iterator must


In our implementation, the transactions are not incorporated but just ignored. Is that expected?

Correct, Osmosis (and simapp) just ignore them and use the FIFO-ordered txs that are sent from Tendermint.

types/mempool/nonce.go

alexanderbez · 2022-12-08T17:51:37Z

Sorry @p0mvn didn't mean to waste your time on reviewing the (default) mempool implementation. The implementation was actually changed to be by random sender, instead of lowest-nonce. So this should give a reasonably more fair mempool. However, there are still DoS vectors. Regardless, this is just the default implementation and not something Osmosis will even use anyway.

p0mvn

LGTM.

Per earlier review strategy suggestions: reviewed abci and baseapp in-detail. Skimmed mempool.

Have some questions about tests but non-blocking since I expect the majority of testing is done at the integration level.

x/auth/tx/service_test.go

baseapp/baseapp_test.go

p0mvn · 2022-12-12T16:52:17Z

baseapp/baseapp_test.go

-	cases := map[string]struct {
-		setLoader    func(*BaseApp)
+		// write some data in substore
+		kv, _ := rs.GetStore(key).(storetypes.KVStore)


I'm not really following the test changes in this file. Could you please summarize why these are needed and what is the relationship to ABCI?

The changes to baseapp tests are essentially captured in this PR. The tests prior to this were messy, scattered all over the place and not well organized.

So in this PR, since I had to port the ABCI 1.0 baseapp tests, I figured I'd clean it up all in one go.

So TL;DR

Better organization

Tests for ABCI 1.0

p0mvn · 2022-12-12T19:04:34Z

baseapp/baseapp.go

+//
+// If any transaction fails to pass either condition, the proposal is rejected.
+// Note that step (2) is identical to the validation step performed in
+// DefaultPrepareProposal. It is very important that the same validation logic


This question might be related to Adam's point of custom PrepareProposal

Assuming that ProcessProposal and PrepareProposal have validations stronger than runtTx. Does it mean that a validator who modifies a mempool and relaxes their PrepareProposal validation, can now cause chain halt?

This is captured in the ABCI spec HERE under Requirement 3.

For an honest process, if PrepareProposal passes validity checks for a proposal, those same validity checks must pass during ProcessProposal. Otherwise, you face liveness issues. In practice, this should not be a concern:

If a node is running a custom mempool and thus a custom PrepareProposal and other processes reject that proposal during ProcessProposal, Tendermint will move on to the next proposer. So it should not be a concern unless a majority of validators are running a custom PrepareProposal, then you could face liveness faults.

p0mvn · 2022-12-12T19:16:31Z

baseapp/baseapp.go

+	if app.prepareProposal == nil {
+		app.SetPrepareProposal(app.DefaultPrepareProposal())
+	}
+
+	if app.processProposal == nil {
+		app.SetProcessProposal(app.DefaultProcessProposal())
+	}


Could you please remind me - in Osmosis, both of these are custom to perform a nonce validation check while relying on Tendermint's FIFO, is that correct?

baseapp/baseapp.go

czarcas7ic

Going to have to give this another pass later since it is a lot to take in. Have some clarifying questions here, thanks!

baseapp/abci.go

czarcas7ic · 2022-12-12T19:03:23Z

baseapp/baseapp.go

+// is used in both steps, and applications must ensure that this is the case in
+// non-default handlers.


What does it mean to use a "non-default handler"?

Handler's that are not DefaultPrepareProposal and DefaultProcessProposal.

czarcas7ic · 2022-12-12T19:05:59Z

baseapp/baseapp.go

+
+			bz, err := app.txEncoder(memTx)
+			if err != nil {
+				panic(err)


If a tx fails encoding in the mempool, shouldnt we remove it from the mempool as well?

This is the upstream repo's behavior. I think the idea was that this should never happen. An alternative would be to remove and continue.

Let me ask some other people...

czarcas7ic · 2022-12-12T19:15:56Z

baseapp/baseapp.go

+	return func(ctx sdk.Context, req abci.RequestProcessProposal) abci.ResponseProcessProposal {
+		for _, txBytes := range req.Txs {
+			_, err := app.txDecoder(txBytes)
+			if err != nil {
+				return abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_REJECT}
+			}
+
+			_, _, _, err = app.runTx(runTxProcessProposal, txBytes)
+			if err != nil {
+				return abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_REJECT}
+			}
+		}
+
+		return abci.ResponseProcessProposal{Status: abci.ResponseProcessProposal_ACCEPT}
+	}


Should processProposal also be checking the byteCount as we do in prepareProposal?

No, maxBytes is a local operator (proposer) configuration. Different nodes will have different preferences on MaxBytes.

Doesn't this mean a proposer could upload an arbitrarily large block, or are maxBytes orthogonal?

czarcas7ic · 2022-12-12T19:20:20Z

baseapp/options.go

 func (app *BaseApp) SetCMS(cms store.CommitMultiStore) {
 	if app.sealed {
-		panic("SetEndBlocker() on sealed BaseApp")
+		panic("SetCMS() on sealed BaseApp")


nit: I would prefer the func name to be SetCommitMultiStore, unless CMS is a terms that many know.

That's an API breaking changes, which I'm not against.

baseapp/testutil/messages.go

czarcas7ic · 2022-12-12T19:31:17Z

types/mempool/mempool_test.go

+	tx.getSigs = func() ([]txsigning.SignatureV2, error) {
+		return nil, nil
+	}
+	require.Error(t, s.mempool.Insert(ctx, tx))
+	require.Error(t, s.mempool.Remove(tx))


What's happening here?

Ensuring that such a tx cannot be inserted. And when attempted to be removed, errors as well.

czarcas7ic · 2022-12-12T19:35:52Z

types/mempool/sender_nonce.go

+	if snm.maxTx < 0 {
+		return nil
+	}


Why is no error returned when trying to insert a tx into a mempool with a maxTx of less than zero?

cosmos#14014

czarcas7ic · 2022-12-12T19:37:32Z

types/mempool/sender_nonce.go

+	if len(sigs) == 0 {
+		return fmt.Errorf("tx must have at least one signer")
+	}


Why are we checking for this in the removal? If a tx somehow got into the mempool with no signers, we should want to be able to remove that, right?

We need to get the sender in order to properly remove the tx. If we cannot get the sender, we cannot remove.

alexanderbez added 7 commits November 15, 2022 10:04

updates

c4d7ad5

Merge branch 'feature/abci-1.0' into bez/abci-1.0-updates

dd86ef6

updates

5cd6df4

updates

1340b6d

updates

627ccd5

updates

d3b760a

updates

932cb53

github-actions bot added the C:x/auth label Nov 15, 2022

alexanderbez added 5 commits November 15, 2022 10:44

updates

f5b8837

updates

025dd4b

updates

c08eafd

updates

1d97c6f

updates

a24ea71

github-actions bot added the C:x/bank label Nov 15, 2022

Merge branch 'feature/abci-1.0' into bez/abci-1.0-updates

a71c881

github-actions bot removed the C:x/bank label Dec 1, 2022

alexanderbez added 3 commits December 5, 2022 10:41

updates

ff9232f

updates

c1d8b48

updates

2c47882

github-actions bot added C:CLI C:x/staking labels Dec 6, 2022

alexanderbez added 3 commits December 6, 2022 17:44

updates

9cae10d

updates

de45b41

updates

f78d7c4

github-actions bot removed C:x/staking C:CLI labels Dec 7, 2022

alexanderbez added 3 commits December 7, 2022 10:43

updates

17d845d

updates

4470b10

updates

f6e5156

alexanderbez marked this pull request as ready for review December 7, 2022 16:33

alexanderbez requested review from a team, czarcas7ic, p0mvn and ValarDragon December 7, 2022 16:33

p0mvn reviewed Dec 8, 2022

View reviewed changes

updates

74e3e07

p0mvn approved these changes Dec 12, 2022

View reviewed changes

czarcas7ic reviewed Dec 12, 2022

View reviewed changes

alexanderbez added 2 commits December 13, 2022 10:24

updates

36d6e93

updates

c967279

alexanderbez mentioned this pull request Dec 13, 2022

ABCI: Do not ProcessProposal when you're a proposer that proposed in PrepareProposal #397

Closed

alexanderbez requested a review from czarcas7ic December 13, 2022 16:21

alexanderbez merged commit c1143e3 into feature/abci-1.0 Dec 13, 2022

alexanderbez deleted the bez/abci-1.0-updates branch December 13, 2022 18:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: ABCI 1.0 Phase I Completion #382

feat: ABCI 1.0 Phase I Completion #382

alexanderbez commented Nov 15, 2022 •

edited

Loading

alexanderbez commented Nov 16, 2022

p0mvn left a comment

p0mvn Dec 8, 2022

alexanderbez Dec 13, 2022

alexanderbez commented Dec 8, 2022

p0mvn left a comment

p0mvn Dec 12, 2022

alexanderbez Dec 13, 2022 •

edited

Loading

p0mvn Dec 12, 2022

alexanderbez Dec 13, 2022 •

edited

Loading

p0mvn Dec 12, 2022

alexanderbez Dec 13, 2022

czarcas7ic left a comment

czarcas7ic Dec 12, 2022

alexanderbez Dec 13, 2022

czarcas7ic Dec 12, 2022

alexanderbez Dec 13, 2022

czarcas7ic Dec 12, 2022

alexanderbez Dec 13, 2022

czarcas7ic Dec 13, 2022

czarcas7ic Dec 12, 2022

alexanderbez Dec 13, 2022

czarcas7ic Dec 12, 2022 •

edited

Loading

alexanderbez Dec 13, 2022

czarcas7ic Dec 12, 2022

alexanderbez Dec 13, 2022

czarcas7ic Dec 12, 2022

alexanderbez Dec 13, 2022

		// is used in both steps, and applications must ensure that this is the case in
		// non-default handlers.

feat: ABCI 1.0 Phase I Completion #382

feat: ABCI 1.0 Phase I Completion #382

Conversation

alexanderbez commented Nov 15, 2022 • edited Loading

Changelog

alexanderbez commented Nov 16, 2022

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez commented Dec 8, 2022

p0mvn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexanderbez Dec 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

czarcas7ic left a 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

czarcas7ic Dec 12, 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

alexanderbez commented Nov 15, 2022 •

edited

Loading

alexanderbez Dec 13, 2022 •

edited

Loading

alexanderbez Dec 13, 2022 •

edited

Loading

czarcas7ic Dec 12, 2022 •

edited

Loading