modular transaction hashing

[fedekunze]

Context

As an Ethermint user who submits a transaction via JSON-RPC, the transaction hash that I expect to receive should match the one as if I sent the transaction on Ethereum. Transaction hashing on Tendermint relies on sha256.Sum, while Ethereum transactions use an RLP hash of the data they contain (see here).

Problem

Transaction hashing is currently hardcoded on Tendermint (uses ). I propose to allow transactions to define a custom hashing function

type Tx []byte

// Hash computes the TMHASH hash of the wire encoded transaction.
func (tx Tx) Hash() []byte {
	return tmhash.Sum(tx)
}

cc: @ValarDragon @khoslaventures

[cmwaters]

Are you proposing that just the tx's have a custom hashing function or other hashes as well? Where do you imagine the hashing function gets defined?

[tac0turtle]

This has been brought up in a few places, some conversation has happened here #2186.

Registering the hash function can be done when on the node, it could be a simple api, super limiting in what it does. By default, we use what is there now and the sdk can expose a function to register different hashing functions.

[fedekunze]

@marbar3778 if you could leave a list of actionable items I can implement this issue

[cmwaters]

We are kind of in the process of meddling with the Node constructor / API's so it's not as clear how this could be implemented. If you're just going with the Tx Hashes you would need to add the hasher to the node constructor and then pass that to the mempool constructor for it to use - but I would hold out on that until we get more clarity about how we want the node struct to look/behave

I don't know the layout of Ethermint land but in your case, can't you just wrap the Tendermint RPC and return the correct hash.

[tac0turtle]

@marbar3778 if you could leave a list of actionable items I can implement this issue

We need buy in from the team, this is something that a few people have asked for, and I'm a fan of.

[fedekunze]

ccing: @tessr

[tessr]

Anything that expands Tendermint's non-ABCI surface area needs careful justification, especially at this phase of the release cycle. Can you start by writing a proposal ADR that examines the tradeoffs and explains the benefits?

[tac0turtle]

@fedekunze lets pair on the ADR, this is something we should support if we want Tendermint to be competitive. Other frameworks already support this (https://substrate.dev/docs/en/knowledgebase/advanced/cryptography#hashing-algorithms)

[ValarDragon]

I also think this is something that needs integration.

I'm in support of making tx hashing more application defined, this will be quite useful for a variety of other features as well. (You need this for having the txhash not include witness data that is intended to not make it into the final proposed block)

The only place that makes sense to me with the current abstractions would be to place this customizability on Node as well. (This is due to Tendermint being restricted to view all txs as an opaque set of bytes) Its definitely too much overhead imo for this to go to ABCI

[fedekunze]

will work on the ADR this week

[alexanderbez]

So there are a few things we need to complete here:

1. Remove the hard-coded SHA256 hashing
2. Introduce a Hasher that supports a fixed-set of supported hashers (e.g. SHA256 and BLAKE2b)
3. Possibly support custom hashers that implement Hasher

I was originally opposed to (3) for now, but realized in order to support Ethereum, we need it because otherwise we'd have to implement RLP (unless Ethereum has a ported hasher-as-a-library hasher that we can use).

[JayT106]

will work on the ADR this week
@fedekunze any updates for the ADR?

[fedekunze]

@marbar3778 @cmwaters @alexanderbez we want to resume this issue, what's the preferred approach in your view? I saw that #6773 was closed

[creachadair]

In the context of ABCI++, I think this is closely connected to the problem of how to name transactions so that PrepareProposal can add/modify/remove them. I think we should probably not try to solve them independently of each other. See #8033 for more discussion.