docs: ADR-60 Prepare & Process Proposal #12838

tac0turtle · 2022-08-07T19:17:49Z

Description

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)

p0mvn

Nice work on this ADR! Although I know this is a work in progress, I just wanted to leave some thoughts I had while reading the draft

docs/architecture/adr-060-baseapp++.md

Co-authored-by: Matt Kocubinski <mkocubinski@gmail.com>

sergio-mena

We will provide a final link to the v0.37.x spec when available so you can replace the links in this document.

sergio-mena · 2022-08-12T18:52:14Z

docs/architecture/adr-060-abci++-phase-i.md

+accept, reject, or completely ignore some or all of these transactions. This is
+an important consideration to make as the application can essentially define and
+control its own mempool allowing it to define sophisticated transaction priority
+and filtering mechanisms, by completely ignoring the `TxRecords` Tendermint


Minor: It is the input parameter that you would be ignoring, which is of type bytes, not TxRecords

sergio-mena · 2022-08-12T18:54:21Z

docs/architecture/adr-060-abci++-phase-i.md

+
+It is important to note that in this phase of ABCI++ integration, the application
+is NOT responsible for locking semantics -- Tendermint will still be responsible
+for that. In the future, however, the application will be responsible for locking,


Yes, although this was no longer true in v0.36.x, it's (again) accurate for v0.34.x, and therefore for v0.37.x.
We have no plans to play the the locks before releasing v0.37.x.

sergio-mena · 2022-08-12T19:31:05Z

docs/architecture/adr-060-abci++-phase-i.md

+
+Prior to describing the implementation of the two new methods, it is important to
+note that the existing ABCI methods, `CheckTx`, `DeliverTx`, etc, still exist and
+serve the same functions as they do now.


Suggested change

serve the same functions as they do now.

serve the same functions as they do now. Note that the functions of `CheckTx` are extended (see next section).

I find it somewhat confusing to state that "CheckTx serves the same functions as now". IIUC, CheckTx will serve more functions in the new design.
In contrast, the paragraph in lines 56-59 is accurate. Consider adding a sentence similar to the one suggested.

CheckTx does serve the same function -- tx ante validation, with the added responsibility of mempool insertion/eviction which I mentioned in the ADR.

docs/architecture/adr-060-abci++-phase-i.md

sergio-mena · 2022-08-12T19:35:38Z

docs/architecture/adr-060-abci++-phase-i.md

+// casting can be used in the actual mempool implementation.
+type MempoolTx interface {
+	// Size returns the size of the transaction in bytes.
+	Size(codec.Codec) int


Can you explain the role of the method's parameter?

The godoc describes it. It will be used for reaping and eviction.

I think @sergio-mena meant why we need a codec here. It's maybe worth putting that in the godoc too.

docs/architecture/adr-060-abci++-phase-i.md

sergio-mena · 2022-08-12T20:24:13Z

docs/architecture/adr-060-abci++-phase-i.md

+`PrepareProposal`, since `ProcessProposal` can fail and we do not want to lose
+transactions, we need to finally remove the transaction from the app-side mempool
+during `DeliverTx` since during this phase, the transactions are being included
+in the proposed block.


Probably none of my business, but have you considered something like a TTL to make sure every transaction is eventually removed from the app-side mempool (e.g., to prevent corner-case TXs from filling up the mempool over time)?

I'm not sure there is a direct need for a TTL as we already have a similar notion on txs directly and the mempool will be capped (by tx size).

docs/architecture/adr-060-abci++-phase-i.md

Co-authored-by: Sergio Mena <sergio@informal.systems>

tac0turtle

LGTM, only thing missing it seems is the added unmodified stuff tendermint needs. Id prefer to avoid this but seems like we cant

docs/architecture/adr-060-abci++-phase-i.md

peterbourgon · 2022-08-20T02:28:38Z

docs/architecture/adr-060-abci++-phase-i.md

+Previous discussions<sup>1</sup> have come to the agreement that Tendermint will
+perform a request to the application, via `RequestPrepareProposal`, with a certain
+amount of transactions reaped from Tendermint's local mempool. The exact amount
+of transactions reaped will be determined by a local operator configuration.
+This is referred to as the "one-shot approach" seen in discussions.


If the mempool has more transactions than requested, what determines which ones are reaped?

edit: oh, sorry, is this line 119?

It's however the mempool wants to define this behavior -- e.g. the lower priority ones.

So it's a Tendermint config option?

No, the application defines the mempool entirely. We will provide a basic one out of the box, whereas applications can define their own if the default one does not suite their needs (e.g. dYdX).

peterbourgon · 2022-08-20T02:41:52Z

docs/architecture/adr-060-abci++-phase-i.md

+// PrepareTxRecord defines a wrapper around a MempoolTx that is returned from
+// PrepareProposal which includes an Action to inform Tendermint what to do with
+// the transaction.
+type PrepareTxRecord[T MempoolTx] struct {
+	Tx T
+	Action abci.TxAction
+}
+
+type Mempool[T MempoolTx] interface {
+	// Insert attempts to insert a MempoolTx into the app-side mempool returning
+	// an error upon failure.
+	Insert(sdk.Context, T) error
+	// ReapMaxBytes returns the next set of available transactions from the app-side
+	// mempool, up to maxBytes or until the mempool is empty. The application can
+	// decide to return transactions from its own mempool or from the incoming
+	// TxRecords or some combination of both. The notion of 'available' or 'next'
+	// is defined by the application's mempool implementation.
+	ReapMaxBytes(ctx sdk.Context, txRecords abci.TxRecords, maxBytes int) ([]PrepareTxRecord[T], error)
+	// NumTxs returns the number of transactions currently in the mempool.
+	NumTxs() int
+	// Remove attempts to remove a transaction from the mempool, returning an error
+	// upon failure.
+	Remove(sdk.Context, T) error
+}


MempoolTx is a regular old interface, right? If so, why bring generics into the picture, versus e.g.

type PrepareTxRecord struct { Tx MempoolTx Action abci.TxAction } type Mempool interface { Insert(sdk.Context, MempoolTx) error ReapMaxBytes(ctx sdk.Context, txRecords abci.TxRecords, maxBytes int) ([]PrepareTxRecord, error) NumTxs() int Remove(sdk.Context, MempoolTx) error }

?

(This won't work directly, of course -- the Remove method implicitly requires that MempoolTx values are comparable, which isn't true at the moment.)

I think I mentioned in a comment above from @williambanfield -- there really isn't a need for generics per se.

amaury1093 · 2022-08-18T15:50:25Z

docs/architecture/adr-060-abci++-phase-i.md

+
+The `PrepareProposal` ABCI method is concerned with a block proposer requesting
+the application to evaluate a series of transactions to be included in the next
+block, defined as a slice of `TxRecord` objects. The application can either


Suggested change

block, defined as a slice of `TxRecord` objects. The application can either

block, defined as a slice of [`TxRecord`](todo) objects. The application can either

Can we add a link to what a TxRecord is?

amaury1093 · 2022-08-18T15:53:48Z

docs/architecture/adr-060-abci++-phase-i.md

+// casting can be used in the actual mempool implementation.
+type MempoolTx interface {
+	// Size returns the size of the transaction in bytes.
+	Size(codec.Codec) int


I think @sergio-mena meant why we need a codec here. It's maybe worth putting that in the godoc too.

* add layout for abci++ inclusion * prepare proposal * fix doc * updates Co-authored-by: Aleksandr Bezobchuk <aleks.bezobchuk@gmail.com> Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> Co-authored-by: Matt Kocubinski <mkocubinski@gmail.com> Co-authored-by: Sergio Mena <sergio@informal.systems>

add layout for abci++ inclusion

1b8f4a5

github-actions bot added the Type: ADR label Aug 7, 2022

tac0turtle and others added 7 commits August 7, 2022 21:31

prepare proposal

1700589

fix doc

4e31c48

updates

59dc204

updates

9647fcb

updates

9303039

updates

ae8e037

updates

d43e901

p0mvn reviewed Aug 11, 2022

View reviewed changes

kocubinski reviewed Aug 11, 2022

View reviewed changes

docs/architecture/adr-060-baseapp++.md Outdated Show resolved Hide resolved

alexanderbez and others added 14 commits August 11, 2022 11:39

updates

a542585

updates

ca22cac

updates

aa513ca

updates

790949c

updates

a045bb3

updates

17c9ec2

Update docs/architecture/adr-060-baseapp++.md

f6b08d2

Co-authored-by: Matt Kocubinski <mkocubinski@gmail.com>

updates

653fd5a

updates

c4025b8

updates

3a89e3e

updates

25201d8

updates

a7440a0

updates

90b9555

updates

6d756f3

alexanderbez marked this pull request as ready for review August 11, 2022 17:00

alexanderbez requested a review from a team as a code owner August 11, 2022 17:00

alexanderbez requested review from zmanian, ValarDragon and p0mvn August 11, 2022 17:01

sergio-mena reviewed Aug 12, 2022

View reviewed changes

tac0turtle and others added 3 commits August 14, 2022 15:17

Update docs/architecture/adr-060-abci++-phase-i.md

391e1f3

Co-authored-by: Sergio Mena <sergio@informal.systems>

Update docs/architecture/adr-060-abci++-phase-i.md

f964ba3

Co-authored-by: Sergio Mena <sergio@informal.systems>

Update docs/architecture/adr-060-abci++-phase-i.md

411c13b

Co-authored-by: Sergio Mena <sergio@informal.systems>

tac0turtle commented Aug 14, 2022

View reviewed changes

alexanderbez added 7 commits August 16, 2022 11:14

updates

b496639

updates

0c83134

updates

0b40b5b

updates

2460cc9

updates

ade7834

updates

6a44e05

updates

4169c09

prettymuchbryce reviewed Aug 17, 2022

View reviewed changes

docs/architecture/adr-060-abci++-phase-i.md Outdated Show resolved Hide resolved

alexanderbez added 2 commits August 17, 2022 13:33

updates

7f92d8c

updates

c40cfe6

prettymuchbryce reviewed Aug 18, 2022

View reviewed changes

docs/architecture/adr-060-abci++-phase-i.md Show resolved Hide resolved

alexanderbez added 2 commits August 18, 2022 09:51

updates

df2724a

updates

ce7068c

prettymuchbryce reviewed Aug 19, 2022

View reviewed changes

docs/architecture/adr-060-abci++-phase-i.md Outdated Show resolved Hide resolved

prettymuchbryce approved these changes Aug 19, 2022

View reviewed changes

peterbourgon reviewed Aug 20, 2022

View reviewed changes

updates

6238fce

amaury1093 reviewed Aug 29, 2022

View reviewed changes

alexanderbez approved these changes Aug 29, 2022

View reviewed changes

Merge branch 'main' into marko/abci++_adr

0facd59

tac0turtle enabled auto-merge (squash) August 30, 2022 06:49

tac0turtle merged commit 6aaf83c into main Aug 30, 2022

tac0turtle deleted the marko/abci++_adr branch August 30, 2022 06:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

docs: ADR-60 Prepare & Process Proposal #12838

docs: ADR-60 Prepare & Process Proposal #12838

tac0turtle commented Aug 7, 2022

p0mvn left a comment

sergio-mena left a comment

sergio-mena Aug 12, 2022

sergio-mena Aug 12, 2022

sergio-mena Aug 12, 2022

sergio-mena Aug 12, 2022

alexanderbez Aug 16, 2022

sergio-mena Aug 12, 2022

alexanderbez Aug 16, 2022 •

edited

Loading

amaury1093 Aug 18, 2022

sergio-mena Aug 12, 2022

alexanderbez Aug 16, 2022

tac0turtle left a comment

peterbourgon Aug 20, 2022 •

edited

Loading

alexanderbez Aug 22, 2022

peterbourgon Aug 22, 2022

alexanderbez Aug 23, 2022

peterbourgon Aug 20, 2022

peterbourgon Aug 20, 2022 •

edited

Loading

alexanderbez Aug 22, 2022

amaury1093 Aug 18, 2022

amaury1093 Aug 18, 2022

	serve the same functions as they do now.
	serve the same functions as they do now. Note that the functions of `CheckTx` are extended (see next section).

	block, defined as a slice of `TxRecord` objects. The application can either
	block, defined as a slice of [`TxRecord`](todo) objects. The application can either

docs: ADR-60 Prepare & Process Proposal #12838

docs: ADR-60 Prepare & Process Proposal #12838

Conversation

tac0turtle commented Aug 7, 2022

Description

Author Checklist

Reviewers Checklist

p0mvn left a comment

Choose a reason for hiding this comment

sergio-mena 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

alexanderbez Aug 16, 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

tac0turtle left a comment

Choose a reason for hiding this comment

peterbourgon Aug 20, 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

peterbourgon Aug 20, 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

alexanderbez Aug 16, 2022 •

edited

Loading

peterbourgon Aug 20, 2022 •

edited

Loading

peterbourgon Aug 20, 2022 •

edited

Loading