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

refactor(stdlib): rewrite contractAddressExt, newAddress and Address.asSlice functions to Tact #1766

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

i582
Copy link
Contributor

@i582 i582 commented Feb 10, 2025

Screenshot 2025-02-13 at 16 00 12

Issue

Closes #1763.
Closes #1764.
Closes #1765.
Towards #1736.
Towards #573.

Checklist

  • I have run all the tests locally and no test failure was reported
  • I have run the linter, formatter and spellchecker
  • I did not do unrelated and/or undiscussed refactorings

@i582 i582 changed the title refactor(stdlib: rewrite contractAddressExt, newAddress and Address.asSlice functions to Tact refactor(stdlib): rewrite contractAddressExt, newAddress and Address.asSlice functions to Tact Feb 10, 2025
@anton-trunov anton-trunov added this to the v1.6.0 milestone Feb 10, 2025
@i582 i582 marked this pull request as ready for review February 10, 2025 14:46
@i582 i582 requested a review from a team as a code owner February 10, 2025 14:46
@i582 i582 requested a review from Shvandre February 10, 2025 14:46
@anton-trunov anton-trunov self-assigned this Feb 13, 2025
@i582
Copy link
Contributor Author

i582 commented Feb 13, 2025

Some changes from #1804, let's wait for #1804 to be merged and merge master into this PR

@i582 i582 requested a review from Shvandre February 13, 2025 10:50
@i582 i582 requested a review from Shvandre February 13, 2025 11:58
@i582 i582 requested a review from anton-trunov February 13, 2025 14:57
@anton-trunov
Copy link
Member

merge conflicts

@i582
Copy link
Contributor Author

i582 commented Feb 13, 2025

merge conflicts

Fixed

@@ -163,6 +163,11 @@ asm extends fun refs(self: Builder): Int { BREFS }

asm extends fun bits(self: Builder): Int { BBITS }

asm extends fun hash(self: Builder): Int {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need correctness tests for this new method: the hash we compute here should have a certain relationship with .hash() for Cells / Slices

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one will calculate data hash, not cell hash. Exactly like SHA256U for slices.

I think we should either rename this method to something like dataHash or make it compute cell hash to match the logic of .hash for cells and slices.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@novusnota
Copy link
Member

Added docs, please take a look!

Renamed calculateAddressHash() to contractHash() for clarity and conformance to existing functions. It's not the Address yet only the account_id part of the address, hence the simplification of the name.

Also renamed Slice.toAddressUnsafe to Slice.asAddressUnsafe because:

  • there is map.asCell() method that does similar type casting in spirit, i.e. NOP
  • there is a mirror function Address.asSlice(), also a NOP
  • issue 1736 originally discussed the .asAddressUnsafe() name

In general, I think at some point we might carefully split all the asSomething() casts and toSomething() conversions, where the former are no-ops and simply instruct the compiler to start viewing the underlying type differently, while the latter actually require some operations and transformations to be performed.

P.S.: We can revert those renames if you wish, but I believe they are on point :)

@novusnota
Copy link
Member

novusnota commented Feb 14, 2025

I've renamed calculateAddressHash() to contractHash(), see the rationale here.

The current implementation of contractHash() produces the following Fift assembly. Notice lots of SWAPs and such:

$global_contractHash PROCINLINE:<{
      NEWC
  131380 PUSHINT
  SWAP
  24 STU
  s2 PUSH
      CDEPTH
  SWAP
  16 STU
  OVER
      CDEPTH
  SWAP
  16 STU
  s0 s2 XCHG
      HASHCU
  ROT
  256 STU
  SWAP
      HASHCU
  SWAP
  256 STU
      ONE HASHEXT_SHA256
}>

If we rewrite it as a Tact asm function, we can easily get rid of lots of SWAPs and similar instructions by placing the data optimally, thus optimizing the resulting gas consumption. Furthermore, we would not need to introduce a separate Builder.hash() function at all, since it would be inlined.

That's why instead of writing docs for Builder.hash() function, I've efficiently rewrote the contractHash() function to Tact assembly:

asm fun contractHash(code: Cell, data: Cell): Int {
    // According to the https://docs.tact-lang.org/book/cells#cells-representation,
    // the layout for the Builder to hash goes as follows:
    // 1) refs_descriptor:bits8 | bits_descriptor:bits8 | data:bitsN
    //
    //  refs_descriptor: ref_count + ((exotic? & 1) * 8) + (mask * 32)
    //                   2 refs (code + data), non-exotic, zero-mask
    //
    //  bits_descriptor: floor(bit_count / 8) + ceil(bit_count, 8)
    //                   floor (5 bits / 8) + ceil(5 bits / 8) = 0 + 1 = 1
    //
    //  data: [0b00110] + [0b100] = [0b00110100] = 0x34 (data + augmented bits)
    //        0b00110 - data (split_depth, special, code, data, Library)
    //        0b100 - augmented bits (Leading 1 + zeroes to make section multiple of eight)
    //
    //  That is: (2 << 16) | (1 << 8) | 0x34 = 131380 for all three.
    //
    // 2) and 3) depth_descriptors: CDEPTH of `code` and CDEPTH of `data`
    // 4) and 5) ref hashes: HASHCU of `code` and HASHCU of `data`

    // Group 1: Computations and arrangements
    s0 PUSH HASHCU // `data` hash
    s2 PUSH HASHCU // `code` hash
    SWAP2
    CDEPTH         // `data` depth
    SWAP
    CDEPTH         // `code` depth
    131380 INT     // (2 << 16) | (1 << 8) | 0x34

    // Group 2: Composition of the Builder
    NEWC
    24 STU  // store refs_descriptor | bits_descriptor | data
    16 STU  // store depth_descriptor for `code`
    16 STU  // store depth_descriptor for `data`
    256 STU // store `code` hash
    256 STU // store `data` hash

    // Group 3: SHA256 hash of the resulting Builder
    ONE HASHEXT_SHA256 // a.k.a. Builder.hash()
}

In this form, it consumes 90 less gas than its non-asm counterpart, while producing identical results.

@anton-trunov @Shvandre @i582 @Gusarich If you agree with this vision, let's remove the Builder.hash() and replace the current contractHash() with my asm implementation :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants