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

remove the crc-32 library from dependency #1565

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

Conversation

Danil42Russia
Copy link
Contributor

@Danil42Russia Danil42Russia commented Jan 24, 2025

Issue

Closes #1437.

The table was generated in advance, via:

function main() {
  const POLYNOMIAL = -306674912;
  for (let i = 0; i < 256; i++) {
    let r = i;
    for (let bit = 8; bit > 0; --bit) {
      r = ((r & 1) ? ((r >>> 1) ^ POLYNOMIAL) : (r >>> 1));
    }

    console.log(("0x" + (r >>> 0).toString(16)).padStart(8, "0"))
  }
}

If you need to write tests, I'll add them

Checklist

  • I have updated CHANGELOG.md
  • 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

@Danil42Russia Danil42Russia requested a review from a team as a code owner January 24, 2025 18:36
@@ -0,0 +1,58 @@
const TABLE = new Int32Array([
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't add generated code here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why?

  1. it is faster than creating a table every time;
  2. it is done by analogy with crc16, which has the same table.

Copy link
Contributor

@verytactical verytactical Jan 25, 2025

Choose a reason for hiding this comment

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

Which of the two options sounds more correct? :)

  • execute 2048 operations every time compiler starts
  • have a table of questionable provenance that is impossible to review, that also takes 65536 operations every time TS checks types, and also increases size of js bundle

Copy link
Contributor Author

@Danil42Russia Danil42Russia Jan 25, 2025

Choose a reason for hiding this comment

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

So typescript and type checking is called only once, when building a project. And if you don't calculate the table, you will have to do it every time you start the compiler.

Regarding the “weird” table:

  1. This table is available in the LLVM project: https://github.com/llvm/llvm-project/blob/6383a12e3b4339fa4743bb97da4d51dea6d2e2ea/llvm/lib/Support/CRC.cpp#L30
  2. There are many references in Microsoft repository https://github.com/search?q=org%3Amicrosoft%20%220xD9D65ADC%22&type=code

If you look at the code of the js-crc32 library, it doesn't get any better, it doesn't even have comments https://github.com/SheetJS/js-crc32/blob/master/crc32c.js#L85

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To make sure the table is correct, you can copy from anywhere else, diff will be zero.
I can also add its generator to the project.

Copy link
Contributor

Choose a reason for hiding this comment

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

I realize there are quite a few mentions of this table online, because it's used for something like last half of a century, but I don't think checking numbers by hand is a sound approach to development.

type checking is called only once

There's also tsserver.

Copy link
Contributor

@verytactical verytactical left a comment

Choose a reason for hiding this comment

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

The original issue started specifically with

Write tests with a contract that uses crc32 function that pass strings with unicode characters.

Until there are tests for unicode characters, or rather any crc32 tests at all, we can't even know that hash is computed the way it did before, and can't do any of these changes.

@verytactical
Copy link
Contributor

Imagine worst-case scenario: we have something like message(crc32()) in tact contract, gets evaluated at compile-time as constant expression, all the contract tests pass because bindings use precisely the same value, then incorrect contract is deployed.

]);

export function crc32(data: string | Buffer): number {
if (!(data instanceof Buffer)) {
Copy link
Contributor

@verytactical verytactical Jan 25, 2025

Choose a reason for hiding this comment

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

If src/optimizer/interpreter.ts is the only place we call it from, we never pass Buffer here.

Buffer is Node's non-standard API for binary data, while there is platform-agnostic Blob. When we build browser version of @tact-lang/compiler, we have to include a polyfill for Buffer.

While there are other places that use Buffer, we're working hard to remove them. I'd consider removing Buffer support here too.

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

Successfully merging this pull request may close these issues.

Remove crc-32 dependency
2 participants