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

fix: transaction hash computation #1303

Merged
merged 5 commits into from
Aug 6, 2024
Merged

fix: transaction hash computation #1303

merged 5 commits into from
Aug 6, 2024

Conversation

ArielElp
Copy link
Collaborator

@ArielElp ArielElp commented Jun 27, 2024

Description of the Changes

Minor fixes to transaction hashes computation

PR Preview URL

After you push a commit to this PR, a preview is built and a URL to the root of the preview appears in the comment feed.

Paste here the specific URL(s) of the content that this PR addresses.

Check List

  • Changes have been done against main branch, and PR does not conflict
  • PR title follows the convention: <docs/feat/fix/chore>(optional scope): <description>, e.g: fix: minor typos in code

This change is Reviewable

@ArielElp ArielElp requested a review from odednaor June 27, 2024 14:43
@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1303/ .

7 similar comments
@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1303/ .

@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1303/ .

@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1303/ .

@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1303/ .

@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1303/ .

@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1303/ .

@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1303/ .

Copy link
Collaborator

@odednaor odednaor left a comment

Choose a reason for hiding this comment

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

Made some comments

@@ -222,7 +223,7 @@ Where:

- `l1_handler` is a constant prefix, encoded in bytes (ASCII), as big-endian.
- `chain_id` is a constant value that specifies the network to which this transaction is sent.
- _h_ is the xref:architecture-and-concepts:cryptography/hash-functions.adoc#pedersen_hash[Pedersen] hash
- _h_ is the xref:architecture-and-concepts:cryptography/hash-functions.adoc#pedersen_hash[Pedersen] hash (note that since we're hashing an array, the # of inputs needs to be appended to the hash).
Copy link
Collaborator

@odednaor odednaor Jul 7, 2024

Choose a reason for hiding this comment

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

Suggested change
- _h_ is the xref:architecture-and-concepts:cryptography/hash-functions.adoc#pedersen_hash[Pedersen] hash (note that since we're hashing an array, the # of inputs needs to be appended to the hash).
- _h_ is the xref:architecture-and-concepts:cryptography/hash-functions.adoc#pedersen_hash[Pedersen] hash. Be aware that because an array is hashed, you need to append the number of inputs to the hash.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's a serialized array, appended may be misleading. The length of the array must be the first element of the array to be hashed.
[len, c1, c2, c3, ...].

Copy link
Collaborator

@stoobie stoobie Jul 10, 2024

Choose a reason for hiding this comment

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

@glihm Good catch. How about this:

Suggested change
- _h_ is the xref:architecture-and-concepts:cryptography/hash-functions.adoc#pedersen_hash[Pedersen] hash (note that since we're hashing an array, the # of inputs needs to be appended to the hash).
- _h_ is the xref:architecture-and-concepts:cryptography/hash-functions.adoc#pedersen_hash[Pedersen] hash. Be aware that because an array is hashed, the length of the array must be the first element of the array.

Copy link
Collaborator

@stoobie stoobie left a comment

Choose a reason for hiding this comment

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

@odednaor I made a couple of change requests.

@@ -209,10 +209,11 @@ The hash of the corresponding L1 handler transaction on L2 is computed as follow
----
l1_handler_tx_hash = ℎ(
"l1_handler",
version,
version, // at the moment only version 0 is supported
contract_address,
entry_point_selector,
ℎ(calldata),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
(calldata),
_h_(calldata),

stoobie and others added 2 commits July 8, 2024 15:29
Copy link
Collaborator

@stoobie stoobie left a comment

Choose a reason for hiding this comment

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

@odednaor Please check my suggestion for line 227. It should be clear who/what is appending the number of inputs to the hash.

@@ -222,7 +223,7 @@ Where:

- `l1_handler` is a constant prefix, encoded in bytes (ASCII), as big-endian.
- `chain_id` is a constant value that specifies the network to which this transaction is sent.
- _h_ is the xref:architecture-and-concepts:cryptography/hash-functions.adoc#pedersen_hash[Pedersen] hash
- _h_ is the xref:architecture-and-concepts:cryptography/hash-functions.adoc#pedersen_hash[Pedersen] hash (note that since we're hashing an array, the # of inputs needs to be appended to the hash).
Copy link
Contributor

Choose a reason for hiding this comment

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

If it's a serialized array, appended may be misleading. The length of the array must be the first element of the array to be hashed.
[len, c1, c2, c3, ...].

@@ -221,8 +222,9 @@ l1_handler_tx_hash = ℎ(
Where:

- `l1_handler` is a constant prefix, encoded in bytes (ASCII), as big-endian.
- `version` is the transaction version. Only version 0 is currently supported.
Copy link
Contributor

Choose a reason for hiding this comment

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

We may refer to the new table to have this being true forever, even if a new version shows up?

Copy link
Collaborator

@stoobie stoobie left a comment

Choose a reason for hiding this comment

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

@ArielElp There a few comments and suggestions. PTAL.

@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1303/ .

4 similar comments
@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1303/ .

@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1303/ .

@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1303/ .

@starknet-bot
Copy link
Collaborator

Your preview build is ready! ✨ Check the following link in 1-2 minutes: https://starknet-io.github.io/starknet-docs/pr-1303/ .

@LandauRaz LandauRaz dismissed stoobie’s stale review August 6, 2024 08:19

None of the comments and suggestions seemed crucial and @ArielElp is OOO until September

@LandauRaz LandauRaz merged commit 24efa42 into main Aug 6, 2024
1 of 2 checks passed
@LandauRaz LandauRaz deleted the fix_tx_hashes branch August 6, 2024 08:20
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.

None yet

6 participants