Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rework EffectHash #2808

Closed
3 tasks done
redshiftzero opened this issue Jul 10, 2023 · 2 comments · Fixed by #2943
Closed
3 tasks done

Rework EffectHash #2808

redshiftzero opened this issue Jul 10, 2023 · 2 comments · Fixed by #2943
Assignees

Comments

@redshiftzero
Copy link
Member

redshiftzero commented Jul 10, 2023

This is a tracking ticket for modifying the EffectHash:

@redshiftzero redshiftzero moved this to Next in Testnets Jul 10, 2023
@conorsch
Copy link
Contributor

Raising priority on this ticket, moving to top of Next column. We still need more discussion on design. The motivation for this work is to reduce the likelihood that a developer will overlook these interfaces and thereby introduce bugs; see for example https://github.com/penumbra-zone/penumbra/pull/2820/files.

@cronokirby
Copy link
Contributor

Some ideas after more discussion with @redshiftzero:

  • For handling padding, we could use a similar api to the GroupHasher utility we've just introduced for the setup ceremony code.
  • For handling ordering of items we need to include in the effect hash (like the recent PR in fix ics20 withdrawal: include withdrawals in effect hash and tx builder #2820), one solution would be to hash all of the items, and then sort the hashes, before including those in the hash state. This would provide a canonical total ordering for any item that can be hashed, and thus avoid the need to worry about having a consistent ordering elsewhere in the code. Not sure if that's totally solving ordering, but seems to move the issue away from the effect hash side of things.
  • For handling code reuse with proto serialization, the tricky part is when a structure mixes both "atomic" fields (string, ints, etc.) and composite substructures that need to be hashed. The issue is that we can't then just hash the serialization of the struct, because the sub-structures need to be hashed first. One way around this, that requires touching many protos, would be to make it so that each structures is always either a collection of sub-structures, or a collection of atomic fields. This would mean that the only two codepaths would be hashing all sub-structures before combining those hashes, or serializing the entire structure as a protobuf and hashing that.

@redshiftzero redshiftzero moved this from Next (Steal from here) to Testnet 59: Enceladus in Testnets Aug 11, 2023
@redshiftzero redshiftzero self-assigned this Aug 16, 2023
hdevalence added a commit that referenced this issue Nov 15, 2023
I'd missed that this change wasn't already included in #2808. I thought that
we'd moved to the simpler alternative of making the `EffectHash` just be the
hash of all of the actions in the plan, and expecting the transaction builder
to build the actions in the order they appear in the plan.

Not doing this totally blows up the complexity of the changes we're trying to
achieve now.  However, making this change means that we can simply build the
actions in the order they appear in the plan and get correct results.
hdevalence added a commit that referenced this issue Nov 17, 2023
I'd missed that this change wasn't already included in #2808. I thought that
we'd moved to the simpler alternative of making the `EffectHash` just be the
hash of all of the actions in the plan, and expecting the transaction builder
to build the actions in the order they appear in the plan.

Not doing this totally blows up the complexity of the changes we're trying to
achieve now.  However, making this change means that we can simply build the
actions in the order they appear in the plan and get correct results.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Testnet 59: Enceladus
Development

Successfully merging a pull request may close this issue.

3 participants