Support a secondary index for transaction status

[cmwaters]

Feature Request

Summary

The block store stores all transactions but yet in order to know whether a transaction has been committed, one needs to enable the tx indexer which takes up a lot of disk space. There should be some minimal way of checking that the transaction has committed and being able to retrieve it. This completes the conventional flow of submitting a transaction and then polling the status to see that it has been confirmed.

Proposal

Extend the block store with a secondary index with the tx hash as the key and the and transactional meta data as the value. This should include the height, index and exec that it was committed so that it can be retrieved.

Introduce a new RPC endpoint TxStatus that returns the status of the transaction. To begin with this is just committed or not-committed. In the future, we may want to incorporate the mempool to understand whether it is pending or has been evicted.

Modify the Tx endpoint to use this secondary index in the event that the tx indexer is not enabled to be able to retrieve the committed transaction.

Actually mark broadcast_tx_commit as deprecated. We can plan to remove it in v3.

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged
• Contributor assigned/self-assigned

[rootulp]

1. Why don't we upstream this? This issue doesn't seem Celestia-specific.

2. Does this issue involve defining a new indexer config option?

3. This should include the height, index and exec that it was committed so that it can be retrieved.

   To clarify are the three fields:

   type MinimalIndexerValue struct {
   	blockHeight int64
   	index int64
   	isCommitted bool
   }

4. In the future, we may want to incorporate the mempool to understand whether it is pending or has been evicted.

   I assume that transactions get added to the new indexer when they get added to the mempool and updated after they have been included in a block. If the mempool logic isn't incorporated into the first implementation of this issue, does that mean all transactions in the indexer will have isCommitted = true?

[evan-forbes]

We could definitely upstream this, and I think it would be useful for other projects as well. Projects with large transactions would gain the most out of this I think.

one very simple way to implement this would be to add a separate configuration for the existing index, and then if that config is selected, then we simply remove the tx from the index. In the config.toml, instead of having indexer = "kv", we could have indexer = "kv-lite" (or whatever name)

then in this method

celestia-core/state/txindex/kv/kv.go

Lines 93 to 135 in ee6aef5

	func (txi *TxIndex) Index(result *abci.TxResult) error {
	b := txi.store.NewBatch()
	defer b.Close()
	hash := types.Tx(result.Tx).Hash()
	if !result.Result.IsOK() {
	oldResult, err := txi.Get(hash)
	if err != nil {
	return err
	}
	// if the new transaction failed and it's already indexed in an older block and was successful
	// we skip it as we want users to get the older successful transaction when they query.
	if oldResult != nil && oldResult.Result.Code == abci.CodeTypeOK {
	return nil
	}
	}
	// index tx by events
	err := txi.indexEvents(result, hash, b)
	if err != nil {
	return err
	}
	// index by height (always)
	err = b.Set(keyForHeight(result), hash)
	if err != nil {
	return err
	}
	rawBytes, err := proto.Marshal(result)
	if err != nil {
	return err
	}
	// index by hash (always)
	err = b.Set(hash, rawBytes)
	if err != nil {
	return err
	}
	return b.WriteSync()
	}

we would simply remove the tx bytes from the result before indexing if configured to do so. That would mean that we could use the exact same endpoint etc, the only different would be that the result when querying the tx would not include the actual tx itself unless the normal "kv" tx index was used.

later, we could also pass this indexer to the mempool, which could add, remove, or update txs from the index. We could also remove further fields if that would be advantageous, such as the logs or errors.

[rootulp]

Given #1178 is in review, does this issue no longer need discussion?

[ninabarbakadze]

I gave an update in the pr description but i could copy it over here as well.

This issue is now part of the BestTxs workstream as a temporary fix since there's a great demand for being able to easily query txs without having to enable kv indexer. I believe, @cmwaters and john discussed this during marseille offsite and until we come up with a better, more permanent design we'd proceed with this solution.

[rootulp]

I'm asking if we should remove the needs-discussion label from this issue if #1178 closes it?

If #1178 doesn't close this issue, what's remaining in this issue after #1178 merges?

[ninabarbakadze]

I'd say we could remove the needs-discussion label. #1178 implements the proposed solution in this issue. The only thing missing in my solution is the committed tag, because transactions are already committed when they get indexed in the blockstore.