Skip to content

Proto: remove incorrect functions from Reveal_hash

Andrea Cerone requested to merge andrea@dac-fix-reveal-hash-module into master

Context

Fixes: #4201 (closed)

Currently we include all the functions from Environment.S.Hash in Sc_rollup.Reveal_hash.t, and we borrow the implementation from the underlying V0 module. However, many of these functions (to_bytes, of_bytes_opt, to_string, of_string_opt, etc...) are incorrect as they do not prepend (remove) the tag byte when computing a byte sequence/string (hash).

Only one function that is used in the codebase, Reveal_hash.size was found to be incorrect and to lead to incorrect behaviour when serializing payloads of large size.

We currently have hashes of size 31 + 1 byte (the extra byte for the tag, but we use the function size borrowed by the underlying hash module (which returns 31). This means that the dac encoding will not compute the correct number of hashes in a page: floor(4096/31) = 131, which will lead to hash pages having at most size 131 * 32 = 4192 bytes

This MR explicitly removes the inclusion of V0 functions in the Reveal_hash module, and instead specifies a minimal signature of safe-to-use functions whose implementation is borrowed from the V0 hash module. The implementation of the size function is also changed to fix the bug discovered.

In the future, we may want to define a multihash interface to deal with Reveal_hash.

Manually testing the MR

Relying on CI passing.

Checklist

  • Document the interface of any function added or modified (see the coding guidelines)
  • Document any change to the user interface, including configuration parameters (see node configuration)
  • Provide automatic testing (see the testing guide).
  • For new features and bug fixes, add an item in the appropriate changelog (docs/protocols/alpha.rst for the protocol and the environment, CHANGES.rst at the root of the repository for everything else).
  • Select suitable reviewers using the Reviewers field below.
  • Select as Assignee the next person who should take action on that MR
Edited by Andrea Cerone

Merge request reports