adr: modular hasher by JayT106 · Pull Request #6773 · tendermint/tendermint

JayT106 · 2021-07-28T02:42:35Z

docs/architecture/adr-069-hasher.md

tac0turtle

would love to get a review from @ValarDragon and possibly @robert-zaremba if they would look at using something similar in the sdk.

JayT106 · 2021-07-28T14:39:59Z

would love to get a review from @ValarDragon and possibly @robert-zaremba if they would look at using something similar in the sdk.

Thanks for the comments @marbar3778, definitely it needs more reviews and discussion, like which hash algorithm should be included and implemented in the ADR, the setup for the SDK and other projects.

@fedekunze, is there anything you think we should draft more in the ADR?

github-actions · 2021-08-08T00:07:26Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

tessr · 2021-08-09T09:48:50Z

Echoing @marbar3778's comments above - would love reviews from @ValarDragon, @robert-zaremba, and @fedekunze on this one!

robert-zaremba · 2021-08-16T23:41:18Z

In SDK: we were discussing alternative (to sha256) hashing algorithms in SDK in various contexts: fast state sync (snapshot), ADR-40 (SMT), ADR-28...
The consensus was;

we don't want to have different hardcoded hashing algorithms
we want to have configurable hasher - but implementing it was a low priority.

So the idea would be to have an app config option for providing a hasher. That hasher can be provided as a dependency to modules / keepers etc... I would rather avoid things like a global config (see GetConfig). Implementing it should be much easier once Dependency Injection is implemented (implementation is lead by Aaron with the app wiring working group).

robert-zaremba

Cusomizing hasher is an interesting feature. Few things to clarify:

probably we will need to have both SDK and tendermint to use the same hasher
I don't think we need to create a new interface - the standard hash.Hash interface should be good enough.
Need to specify where the container is stored

docs/architecture/adr-070-hasher.md

robert-zaremba · 2021-08-16T23:52:32Z

docs/architecture/adr-070-hasher.md

+2. Initialize the `HasherContainer` in the node constructor. Might require passing the hasher to the reactors.
+3. Replace the hard-coded tmhash function call using `HasherContainer.GetHasher(SHA256)`.
+4. Mock custom hasher and test it.
+5. Add new objects into the genesis file, register `Hasher` base on settings, and implement the custom transaction hash.


So, you are proposing to have a global instance of HashContainer? In SDK we can use the Dependency Injection framework we are working on.

Thanks for the suggestion. Let me go check again.

Rewrote the proposal after checking the other projects and SDK, I think to put the Container into tmhash as a global instance, do init() in the package, and then provide the Set functions to allow SDK user to inject their hasher.

robert-zaremba · 2021-08-16T23:53:56Z

docs/architecture/adr-070-hasher.md

+        "hash": {
+            "types": "sha256",
+            "custom_tx_hash_enable": "true"
+        }


does it have to be a genesis param? It should be enough if it's an app param - the node will not sync if it's using different setting than the rest of the network.

however, external tools will need to know the hashing algorithm as well (eg to monitor TX confirmation), so having it in the genesis makes it more accessible. WDYT?

does it have to be a genesis param? It should be enough if it's an app param - the node will not sync if it's using different setting than the rest of the network.

Is it okay to put it into the app param? In my mind, it's a genesis-bound param.

however, external tools will need to know the hashing algorithm as well (eg to monitor TX confirmation), so having it in the genesis makes it more accessible. WDYT?

Didn't think about the tooling level. But I agree, if the monitor/explorers are able to utilize it, it might make the tooling easier to integrate with the TM projects?

I don't think this should be a consensus parameter. This would imply that it could change on-chain which would imply we need to build logic to handle the case of the hasher changing. We can add it to the genesis file though and expose what hasher is used via the RPC and app's gRPC.

would be good to introduce genesis params #6814

BTW: by app param I meant an app option - not a x/param thing.

@alexanderbez @marbar3778 I moved the hasher out of the consensus parameter, but still in the genesis.json. Any better place we can add the setting before the genesis params got implemented.

Wondertan · 2021-08-17T10:17:29Z

A small note from a stranger passing by. It might be worth considering using Multihashes for this case, as they are:

Self-described - actual hash gets prepended with information describing the hash itself. Hence, it is possible to have multiple hashes types in runtime. Hence, if sha2(or other) get compromised, the running chain can just move to a new hashing function, while keeping support for the existing history.
Standardised and implemented for various, most common languages
Have the ref implementation in Go and follows all the best practices.

docs/architecture/adr-070-hasher.md

JayT106 · 2021-08-18T15:18:10Z

Multihashes

@Wondertan
Thanks for the suggestion, I've used this for a small Ipfs project a while ago. What about the benchmark compare with just using the hash algorithm itself?

Will it be too corpulent to include it into Tendermint? @alexanderbez @marbar3778

Wondertan · 2021-08-26T17:08:35Z

What about the benchmark compare with just using the hash algorithm itself?

There shouldn't be any difference in performance for any hash algorithm in use. For sha256 multihashes they use sha-simd and similar for other algos they aim to use the most performant implementation. The only overhead is prepended bytes to self-describe the hash type in use.

cmwaters

So the way I see it, the chief aspect to be decided is where on the scale of hashing customization Tendermint wants to position itself. There are a few dimensions to this.

The first of which is does Tendermint offer a set of possible options for developers to choose from or allow any hasher algorithm. For me this depends on how widespread these hashing algorithms are. If 95% of projects use 3 types of hashers then I'd tend towards supporting those three only. If there's hundreds of different hashes and they all offer unique benefits then it's probably okay to allow injection of custom algorithms.

The second dimension looks at how many different components within Tendermint shall we allow for configurable hashes. Here it looks like a developer could have a different tx.Hash() to how evidence or the commit get's hashed. For me personally (at least in the short term), I think a single hasher should be used across all types that are public i.e. when a developer sets a hash it's the only one used for that network. If more demand for increasing hashing customisation comes in the future we can review it then.

docs/architecture/adr-070-hasher.md

github-actions · 2021-09-13T00:08:13Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions · 2021-09-24T00:07:40Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions · 2021-10-05T00:08:30Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

docs/architecture/adr-070-hasher.md

ValarDragon · 2021-10-16T06:12:50Z

docs/architecture/adr-070-hasher.md

+```
+
+### Chain parameter settings
+For the project built on top of Tendermint can use `Hashers.RegisterHasher` to change the main hasher it would like to use. For Tendermint itself, it requires a chain parameter to indicate which hasher should be used in the service when the beginning. Therefore, we need to add a `hasher` param into the `genesis.json`.


wdym chain parameter here. It is not, and should not become an "on-chain parameter", changeable on the fly like consensus params.

i believe this was inline with #6814. As you said its meant to avoid on the fly changing stuff that shouldnt be

I will rephrase it to genesis parameter. Sorry for the ambiguity.

Co-authored-by: Callum Waters <cmwaters19@gmail.com>

github-actions · 2021-10-31T00:09:05Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

github-actions · 2021-11-11T00:08:37Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

fedekunze · 2021-11-14T12:30:49Z

what's the status of this ADR @marbar3778 @cmwaters?

cmwaters · 2021-11-15T11:30:55Z

what's the status of this ADR @marbar3778 @cmwaters?

This was ice boxed as we felt it wasn't really a priority. Personally, I think that custom hashing should only be used for transactions which is the only place where I can see it being a concern for applications. We can bring up this topic at the next dev call.

JayT106 requested review from alexanderbez, cmwaters, ebuchman, tessr, tychoish and williambanfield as code owners July 28, 2021 02:42

tac0turtle reviewed Jul 28, 2021

View reviewed changes

docs/architecture/adr-069-hasher.md Outdated Show resolved Hide resolved

tac0turtle reviewed Jul 28, 2021

View reviewed changes

JayT106 changed the title ~~doc: ADR-069 hasher (draft)~~ adr: ADR-070 hasher (draft) Jul 28, 2021

JayT106 changed the title ~~adr: ADR-070 hasher (draft)~~ adr: hasher (draft) Jul 28, 2021

github-actions bot added the stale for use by stalebot label Aug 8, 2021

cmwaters removed the stale for use by stalebot label Aug 9, 2021

tac0turtle mentioned this pull request Aug 11, 2021

proposal: Genesis Params #6814

Closed

4 tasks

robert-zaremba reviewed Aug 16, 2021

View reviewed changes

liamsi reviewed Aug 17, 2021

View reviewed changes

docs/architecture/adr-070-hasher.md Outdated Show resolved Hide resolved

JayT106 requested a review from creachadair as a code owner August 25, 2021 19:45

tac0turtle changed the title ~~adr: hasher (draft)~~ adr: hasher Aug 30, 2021

tac0turtle changed the title ~~adr: hasher~~ adr: modular hasher Aug 30, 2021

cmwaters reviewed Sep 2, 2021

View reviewed changes

docs/architecture/adr-070-hasher.md Outdated Show resolved Hide resolved

docs/architecture/adr-070-hasher.md Show resolved Hide resolved

docs/architecture/adr-070-hasher.md Outdated Show resolved Hide resolved

github-actions bot added the stale for use by stalebot label Sep 13, 2021

cmwaters removed the stale for use by stalebot label Sep 13, 2021

github-actions bot added the stale for use by stalebot label Oct 5, 2021

robert-zaremba reviewed Oct 5, 2021

View reviewed changes

docs/architecture/adr-070-hasher.md Outdated Show resolved Hide resolved

robert-zaremba reviewed Oct 5, 2021

View reviewed changes

docs/architecture/adr-070-hasher.md Outdated Show resolved Hide resolved

cmwaters removed the stale for use by stalebot label Oct 6, 2021

ValarDragon reviewed Oct 16, 2021

View reviewed changes

JayT106 and others added 11 commits October 20, 2021 17:42

draft

e8cedba

fmt

c484b55

update adr number

1d7b1ec

revision ADR070 hasher

0da0739

add blake3 option

c178521

format

7b12db6

Update docs/architecture/adr-070-hasher.md

3ba2311

Co-authored-by: Callum Waters <cmwaters19@gmail.com>

Update docs/architecture/adr-070-hasher.md

920fa0d

Co-authored-by: Callum Waters <cmwaters19@gmail.com>

add seal function and rename main to default

dce7614

fmt

62046d8

wording

b0c15c4

JayT106 force-pushed the adr-069 branch from 0c3ef0b to b0c15c4 Compare October 20, 2021 21:43

github-actions bot added the stale for use by stalebot label Oct 31, 2021

creachadair removed the stale for use by stalebot label Oct 31, 2021

github-actions bot added the stale for use by stalebot label Nov 11, 2021

Merge branch 'master' into adr-069

7c5f00b

fedekunze removed the stale for use by stalebot label Nov 14, 2021

JayT106 closed this Nov 23, 2021

fedekunze mentioned this pull request Mar 1, 2022

modular transaction hashing #6539

Closed

cason mentioned this pull request Jun 19, 2024

docs(ADR): #113 Modular transaction hashing cometbft/cometbft#2241

Merged

Conversation

JayT106 commented Jul 28, 2021

Uh oh!

Uh oh!

tac0turtle left a comment

Choose a reason for hiding this comment

Uh oh!

JayT106 commented Jul 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Aug 8, 2021

Uh oh!

tessr commented Aug 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

robert-zaremba commented Aug 16, 2021

Uh oh!

robert-zaremba left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JayT106 Aug 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JayT106 Aug 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wondertan commented Aug 17, 2021

Uh oh!

Uh oh!

JayT106 commented Aug 18, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Wondertan commented Aug 26, 2021

Uh oh!

cmwaters left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Sep 13, 2021

Uh oh!

github-actions bot commented Sep 24, 2021

Uh oh!

github-actions bot commented Oct 5, 2021

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JayT106 Oct 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Oct 31, 2021

Uh oh!

JayT106 commented Jul 28, 2021 •

edited

Loading

tessr commented Aug 9, 2021 •

edited

Loading

JayT106 Aug 17, 2021 •

edited

Loading

JayT106 Aug 25, 2021 •

edited

Loading

JayT106 commented Aug 18, 2021 •

edited

Loading

JayT106 Oct 20, 2021 •

edited

Loading