feat: migrate to starknet-types-core Felt type

[tcoratger]

Should close #554.

[Oppen]

Did you run the benchmarks to check for possible regressions? Reports on those would be valuable for the types and lambdaworks teams.

[tcoratger]

Did you run the benchmarks to check for possible regressions? Reports on those would be valuable for the types and lambdaworks teams.

I haven't had time to do it yet, especially because the integration is not completely finished. There are two or three fixes left to do. But as soon as I have everything, I will run the suite of benchmarks between before and after to give feedback.

[xJonathanLEI]

Branch staled. Lemme try to squash and rebase this.

[tcoratger]

Branch staled. Lemme try to squash and rebase this.

Yes last modifications were done before your last two merges. Let me know if you need some help.

[xJonathanLEI]

Squashed and rebased. CI green now.

[xJonathanLEI]

I prefer to have higher level crates use Felt from starknet-core re-export instead of just importing the crate themselves, as they did with FieldElement before. Refactoring now.

[xJonathanLEI]

Reviewed the diffs and they look good. I'm just gonna run some size tests and benchmark on my side before merging.

[xJonathanLEI]

Ran benchmark (cargo bench --all) on my device and this is what I got:

class_hash              time:   [17.404 ms 17.454 ms 17.515 ms]
                        change: [-2.8274% -2.5207% -2.1880%] (p = 0.00 < 0.05)
                        Performance has improved.

ecdsa_get_public_key    time:   [106.33 µs 106.54 µs 106.81 µs]
                        change: [+18.274% +18.598% +18.929%] (p = 0.00 < 0.05)
                        Performance has regressed.

ecdsa_recover           time:   [422.06 µs 425.29 µs 428.33 µs]
                        change: [+8.0781% +8.5721% +9.1202%] (p = 0.00 < 0.05)
                        Performance has regressed.

ecdsa_sign              time:   [159.65 µs 160.38 µs 161.06 µs]
                        change: [+17.471% +17.846% +18.197%] (p = 0.00 < 0.05)
                        Performance has regressed.

ecdsa_verify            time:   [437.45 µs 438.23 µs 439.16 µs]
                        change: [+16.118% +16.390% +16.700%] (p = 0.00 < 0.05)
                        Performance has regressed.

pedersen_hash           time:   [21.652 µs 21.805 µs 21.941 µs]
                        change: [-13.778% -12.943% -12.265%] (p = 0.00 < 0.05)
                        Performance has improved.

poseidon_hash           time:   [8.1969 µs 8.2193 µs 8.2431 µs]
                        change: [+5.6092% +5.9596% +6.2956%] (p = 0.00 < 0.05)
                        Performance has regressed.

poseidon_hash_single    time:   [8.3008 µs 8.3360 µs 8.3808 µs]
                        change: [+5.1017% +6.5091% +7.8048%] (p = 0.00 < 0.05)
                        Performance has regressed.

poseidon_hash_many      time:   [18.198 µs 18.220 µs 18.244 µs]
                        change: [+20.005% +20.337% +20.627%] (p = 0.00 < 0.05)
                        Performance has regressed.

rfc6979_generate_k      time:   [1.4392 µs 1.4523 µs 1.4649 µs]
                        change: [-9.9581% -9.3454% -8.7124%] (p = 0.00 < 0.05)
                        Performance has improved.

It looks like while performance has been improved for k generation and Pedersen hashing, ECDSA and Poseidon performance has actually suffered.

Notably, the class_hash benchmark we have is for legacy Cairo 0 classes, which uses Pedersen instead of Poseidon, so it's seeing an improvement. I guess if we add another one for Cairo 1 it would be the opposite.

[xJonathanLEI]

@tcoratger Any idea on ECDSA and Poseidon perf here? Am a bit reluctant to merge this due to the perf impact.

[xJonathanLEI]

(Edit: see this comment for an update)

I just went ahead and tested the starknet-wasm example. When running the built web page from the browser this is what I got:

Somehow the resulting web app cannot arrive at the correct public key based on the private key. I just tested the master branch and it does work normally.

This is definitely a blocker for this PR.

[xJonathanLEI]

Just upgraded to 0.1.2 for starknet-types-core but it doesn't look like it's helping much:

class_hash              time:   [17.077 ms 17.167 ms 17.257 ms]
                        change: [-6.3100% -5.6820% -5.0437%] (p = 0.00 < 0.05)
                        Performance has improved.

ecdsa_get_public_key    time:   [111.18 µs 111.43 µs 111.82 µs]
                        change: [+24.967% +25.560% +26.286%] (p = 0.00 < 0.05)
                        Performance has regressed.

ecdsa_recover           time:   [433.97 µs 434.99 µs 436.24 µs]
                        change: [+12.239% +12.968% +13.732%] (p = 0.00 < 0.05)
                        Performance has regressed.

ecdsa_sign              time:   [152.99 µs 153.17 µs 153.37 µs]
                        change: [+7.0266% +7.4902% +7.9647%] (p = 0.00 < 0.05)
                        Performance has regressed.

ecdsa_verify            time:   [436.97 µs 437.40 µs 437.88 µs]
                        change: [+11.610% +11.823% +12.001%] (p = 0.00 < 0.05)
                        Performance has regressed.

pedersen_hash           time:   [21.271 µs 21.296 µs 21.324 µs]
                        change: [-13.127% -12.872% -12.615%] (p = 0.00 < 0.05)
                        Performance has improved.

poseidon_hash           time:   [8.5768 µs 8.5887 µs 8.6017 µs]
                        change: [+5.1858% +5.3842% +5.5633%] (p = 0.00 < 0.05)
                        Performance has regressed.

poseidon_hash_single    time:   [8.6104 µs 8.6206 µs 8.6318 µs]
                        change: [+6.3293% +6.4803% +6.6188%] (p = 0.00 < 0.05)
                        Performance has regressed.

poseidon_hash_many      time:   [17.456 µs 17.562 µs 17.669 µs]
                        change: [+5.9767% +6.4526% +6.9839%] (p = 0.00 < 0.05)
                        Performance has regressed.

rfc6979_generate_k      time:   [1.4570 µs 1.4630 µs 1.4704 µs]
                        change: [-7.5800% -7.2414% -6.8791%] (p = 0.00 < 0.05)
                        Performance has improved.

This is still comparing to master, not the commit before the upgrade.

The starknet-wasm issue still exists by the way.

[xJonathanLEI]

I did some debugging and as it turns out, the starknet-wasm error is merely a formatting error. The issue is that while the WASM module requests a format!("{public_key:#064x}"), indicating that 0 must be left-padded to 64 characters, this is ignored by starknet-types-core, returning an unpadded hex string, and eventually causing the assertion error shown in the screenshot above.

This bug must be fixed in starknet-types-core before we can merge this.

[xJonathanLEI]

Rebased on master to keep the PR branch up to date.

[xJonathanLEI]

Tests failing due to Felt formatting issues. Specifically, LowerHex and UpperHex impls do not respect # or padding.

[xJonathanLEI]

Rebased on master and updated to use starknet-types-core v0.1.3 with the Felt formatting fix.

[xJonathanLEI]

Latest benchmark results:

cairo0_class_hash       time:   [9.1665 ms 9.1690 ms 9.1718 ms]
                        change: [-16.644% -16.576% -16.511%] (p = 0.00 < 0.05)
                        Performance has improved.

sierra_class_hash       time:   [6.6931 ms 6.6944 ms 6.6958 ms]
                        change: [+5.9420% +6.0509% +6.1559%] (p = 0.00 < 0.05)
                        Performance has regressed.

ecdsa_get_public_key    time:   [62.223 µs 62.231 µs 62.240 µs]
                        change: [+12.173% +12.464% +12.764%] (p = 0.00 < 0.05)
                        Performance has regressed.

ecdsa_recover           time:   [253.15 µs 253.47 µs 254.13 µs]
                        change: [+1.4131% +1.9446% +2.4244%] (p = 0.00 < 0.05)
                        Performance has regressed.

ecdsa_sign              time:   [95.633 µs 95.649 µs 95.668 µs]
                        change: [+9.9088% +10.033% +10.193%] (p = 0.00 < 0.05)
                        Performance has regressed.

ecdsa_verify            time:   [255.70 µs 255.77 µs 255.84 µs]
                        change: [+3.5085% +3.6447% +3.8199%] (p = 0.00 < 0.05)
                        Performance has regressed.

pedersen_hash           time:   [13.021 µs 13.023 µs 13.024 µs]
                        change: [-19.064% -18.895% -18.747%] (p = 0.00 < 0.05)
                        Performance has improved.

poseidon_hash           time:   [5.0139 µs 5.0148 µs 5.0155 µs]
                        change: [+5.5839% +5.7232% +5.8665%] (p = 0.00 < 0.05)
                        Performance has regressed.

poseidon_hash_single    time:   [5.0239 µs 5.0381 µs 5.0543 µs]
                        change: [+5.7098% +5.9643% +6.2036%] (p = 0.00 < 0.05)
                        Performance has regressed.

poseidon_hash_many      time:   [10.077 µs 10.087 µs 10.100 µs]
                        change: [+5.7924% +5.9339% +6.0957%] (p = 0.00 < 0.05)
                        Performance has regressed.

rfc6979_generate_k      time:   [4.5806 µs 4.5821 µs 4.5836 µs]
                        change: [-3.1612% -3.0148% -2.8486%] (p = 0.00 < 0.05)
                        Performance has improved.

Pedersen hashing is faster; EC operations and Poseidon are slower.

[xJonathanLEI]

LGTM