abci++: simplify prepare proposal API

[cmwaters]

Summary

This issue brings light to a discussion around simplifying the API of prepare proposal. What is proposed are cosmetic changes that are believed to not impact the application in any meaningful way but may make implementation simpler and more intuitive.

Problem Definition

The new ResponsePrepareProposal object returns an array of TxRecord which are the txs (a.k.a the raw bytes) accompanied with an enum, Action, that can be either ADDED, UNMODIFIED, REMOVED. UNMODIFIED signals that a tx has been directly returned from the request object, ADDED means it's new and REMOVED tells Tendermint to remove whatever tx from the mempool if it's there.

message TxRecord {
  TxAction action = 1;
  bytes    tx     = 2;

  // TxAction contains App-provided information on what to do with a transaction that is part of a raw proposal
  enum TxAction {
    UNKNOWN    = 0;  // Unknown action
    UNMODIFIED = 1;  // The Application did not modify this transaction.
    ADDED      = 2;  // The Application added this transaction.
    REMOVED    = 3;  // The Application wants this transaction removed from the proposal and the mempool.
  }
}

Previously, it was decided that ADDED would immediately add the tx to the mempool before being propagated in the block. Later it was decided this wasn't necessary (as the tx is already in the block). Thus, currently, ADDED and UNMODIFIED are semantically the same.

This is additional complexity as it requires the application to know if a tx was proposed in the Request object and then to mark them accordingly. In the case of the SDK which is looking to use an app-side mempool, the application wants to simply return the transactions they already have queued up.

The REMOVED enum adds functionality in that the proposer can quickly garbage transactions that have become invalid from this round of selected transactions. However, there's no guarantee that this will be the block committed - thus the proposer may remove a transaction that may still be valuable for future rounds. More important, this is only for the proposer. The rest of the nodes will still have and gossip the removed transactions. Applications will still need to rely on ReCheckTx for global level garbage collection.

Proposal

There are currently three proposals:

1. Leave everything as it is

As we're relatively close to release we can choose to leave it as is and address it in a later release. The consequence of this means applications will have to do work twice to comply with the APIs

2. Merge the enum

One proposal is to merge these two actions into a single INCLUDED enum. This simply tells Tendermint to include these transactions in the proposed block:

enum TxAction {
    UNKNOWN    = 0;  // Unknown action
    INCLUDED   = 1;  // The Application includes the tx in the proposed block.
    REMOVED    = 2;  // The Application wants this transaction removed from the proposal and the mempool.
}

3. Return the raw transactions
This is the simplest API. The application returns directly the bytes it wants to be proposed in the block. There is no enum. Transactions can't be removed from the proposers mempool but must wait until after the block has been committed with ReCheckTx.

message ResponsePrepareProposal {
  repeated bytes txs = 1;
}

[ValarDragon]

I'm pretty in favor of choosing the API we think is best. My personal ordering is 3 > 2 > 1, 3 being most preferrable. (Keeping tendermint as just thinking of prepareproposal output as [][]byte)

[milosevic]

It seems that UNMODIFIED semantics is not present in any of the options. How important for tendermint is to know if tx is not modified by the application? I also don't understand what is the expected semantics regarding tx propagation with these proposals: I guess the idea is that this is not needed as tx is gossiped through the proposed block, right? What I am trying to understand is how tx lifecycle looks like if tx comes from the application. For example, what if tx is included by the application but then proposed block is not decided? Do we assume txs to mostly come from mempool and then application can potentially only modify it or we could imagine application also having other way to learn/create tx and then propose it.

Regarding semantics of REMOVED with respect to the proposal 3, what exactly is the problem we are trying to address here? As it affects only local mempool copy, this is clearly not enough to deal with garbage collection problem, and I am not sure if it makes sense having this kind of best effort approaches where we do something that helps, but not significantly. If that's the case I would opt for simplicity, which is option 3.

[alexanderbez]

I'm pretty in favor of choosing the API we think is best. My personal ordering is 3 > 2 > 1, 3 being most preferrable. (Keeping tendermint as just thinking of prepareproposal output as [][]byte)

Fully agree with @ValarDragon here.

In fact, the current SDK ADR is actually incorrect in that it currently would set each included/accepted tx as ADDED rather than UNMODIFIED, which IMO, is extremely confusing.

I would simplify the semantics and go with option 3. I think it makes not only Tendermint simpler, but any application reference implementation simpler too.

[williambanfield]

It seems that UNMODIFIED semantics is not present in any of the options. How important for tendermint is to know if tx is not modified by the application?

At the moment there is no distinction made between UNMODIFIED vs ADDED. Tendermint treats these exactly the same way by including them in the proposed block. Previous designs of the ABCI++ API considered allowing the user to mark a transaction as a modified version of a different transaction or set of transactions. This is incredibly fraught for the reason outlined in https://github.com/tendermint/tendermint/blob/main/docs/rfc/rfc-015-abci%2B%2B-tx-mutation.md and so has been left out for the time being. ADDED, in previous versions of the design, placed the transaction into the Tendermint mempool. This was fairly complicated in practice because of the mempool API and was not being asked for by any user, so this functionality was also dropped. As a result, UNMODIFIED and ADDED ended up being effectively identical.

[williambanfield]

I've created a draft proposal to demonstrate what option 3 would look like in terms of code and complexity. It is effectively all deletions. The only element missing from the draft is use of RecheckTx in the test application to remove executed transactions that are modified versions of transactions from the mempool. This should be easy to implement and is not user facing so was left out of the quick draft.

#9283

[alexanderbez]

At the moment there is no distinction made between UNMODIFIED vs ADDED. Tendermint treats these exactly the same way by including them in the proposed block.

What I'm still confused about is what actions on Tendermint's mempool does each Action ENUM have?

Note, there is almost no need or reason to have ADDED -- by the time PrepareProposal is called, the application already would have the txs in its own mempool when Tendermint called CheckTx. This has caused me a lot of confusion.

[prettymuchbryce]

Option 3 seems to be the simplest approach.

Regarding Option 2, I don't see much value from signaling via REMOVED Tendermint should remove a transaction from its own local mempool because 1. The proposal may not be accepted in which case the removed transaction may be needed in the future. 2. Removing this transaction is only for this node's local mempool, not the entire network.

[evan-forbes]

also agree that option 3 seems like the best approach from my point of view.

The added enum seems to make assumptions about the application that are not needed. Specifically the REMOVED enum that removes the tx from mempool and the proposal. We have cases where we want to remove txs from the proposal but keep them in the mempool.

[williambanfield]

We have cases where we want to remove txs from the proposal but keep them in the mempool.

To clarify, the mechanism with the enum listed here does allow one to remove a Tx from the proposal without removing it from the mempool. To remove from the proposal but keep in the mempool, the application would simply return a list of transactions that does not contain the transaction(s) it does not wish to include.

[evan-forbes]

ahh I see now, that makes sense! thx

[cmwaters]

The decision by the team is to implement proposal 3.