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

fault_proving(compression): include commitment to transaction ids within compressed block header #2572

Closed
wants to merge 6 commits into from

Conversation

rymnc
Copy link
Member

@rymnc rymnc commented Jan 15, 2025

Linked Issues/PRs

closes #2569 and closes #2574

Description

instead of the merkle root of transaction ids, we directly commit to a hash of all tx ids ordered as they are in the block.

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

@rymnc rymnc self-assigned this Jan 15, 2025
@rymnc rymnc force-pushed the proving/compressed-header-changes-p3 branch 3 times, most recently from 7fffaa2 to d2d2248 Compare January 15, 2025 14:59
@rymnc rymnc force-pushed the proving/compressed-header-changes-p2 branch from 4d998ae to 4ca0445 Compare January 15, 2025 15:13
@rymnc rymnc force-pushed the proving/compressed-header-changes-p3 branch from d2d2248 to 62b882d Compare January 15, 2025 15:16
netrome
netrome previously approved these changes Jan 16, 2025
Copy link
Contributor

@netrome netrome left a comment

Choose a reason for hiding this comment

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

Looks alright to me. Would be nice to have test coverage of this computation.

@rymnc rymnc force-pushed the proving/compressed-header-changes-p3 branch from 62b882d to 3e090b0 Compare January 22, 2025 14:47
@rymnc rymnc changed the base branch from proving/compressed-header-changes-p2 to master January 22, 2025 14:49
@rymnc rymnc dismissed netrome’s stale review January 22, 2025 14:49

The base branch was changed.

@rymnc rymnc requested a review from netrome January 22, 2025 15:09
@rymnc rymnc marked this pull request as ready for review January 22, 2025 15:49
Copy link
Contributor

@acerone85 acerone85 left a comment

Choose a reason for hiding this comment

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

no comment from my side, looks in good shape

Copy link
Collaborator

@xgreenx xgreenx left a comment

Choose a reason for hiding this comment

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

I don't see how it is useful for us to store hash of all transaction ids in the CompressedHeader. I see how it can be used while it is in the regular header(because we can prove that it is part of the block id, and block id is our main connection point with the block). While in the case of the compressed header, we only can prove that compressed header contains the hash of the some decompressed transactions, these transactions are not connected to the block.

@rymnc
Copy link
Member Author

rymnc commented Jan 24, 2025

I don't see how it is useful for us to store hash of all transaction ids in the CompressedHeader. I see how it can be used while it is in the regular header(because we can prove that it is part of the block id, and block id is our main connection point with the block). While in the case of the compressed header, we only can prove that compressed header contains the hash of the some decompressed transactions, these transactions are not connected to the block.

when the user performs decompression, they end up with a PartialFuelBlock which has a list of transactions. they can then validate the generated PartialFuelBlock's Transactions with the tx_id_commitment in the compressed block header (which is what the validate_with() function does. we also have commitment to the block_id within the compressed header if you are looking for a connection point?

@xgreenx
Copy link
Collaborator

xgreenx commented Jan 27, 2025

The tx_commitment is not related to the block or block id. You can prove that you can decompress transactions using data from the compressed header, but it doesn't prove decompression for the regular block.

Anyone can submit a valid compressed header with valid transactions, and you can verify that you can decompress transactions, but these transactions will not be from the block, they can be any random transactions.

If tx_commitment was a part of the regular block, then you could prove that decompressed transactions are unrelated to the block within the decompression game.

By having tx_commitment only in a compressed header, we require our block-proving game to take a compressed header as an input along with a regular header, mixing execution game and decompression game.

@netrome
Copy link
Contributor

netrome commented Jan 27, 2025

The tx_commitment is not related to the block or block id. You can prove that you can decompress transactions using data from the compressed header, but it doesn't prove decompression for the regular block.

Anyone can submit a valid compressed header with valid transactions, and you can verify that you can decompress transactions, but these transactions will not be from the block, they can be any random transactions.

If tx_commitment was a part of the regular block, then you could prove that decompressed transactions are unrelated to the block within the decompression game.

By having tx_commitment only in a compressed header, we require our block-proving game to take a compressed header as an input along with a regular header, mixing execution game and decompression game.

@rymnc, @xgreenx and I just discussed this and we agreed to:

  1. Have the tx_commitment (which commits to the transaction IDs) be part of the full uncompressed block header.
  2. Add a "deregister" range of block heights to the compressed block header. This range identifies which blocks that have been "evicted" from the temporal registry.
  3. Make the compressed block header a strict superset of the full uncompressed block header. Note 1: This means that we don't need to explicitly store the block ID in the compressed block header, since this should be computable. Note 2: We'll still have fields (e.g. temporal registry commitment etc.) that belong in the compressed block header but not full block header.

@rymnc
Copy link
Member Author

rymnc commented Jan 29, 2025

closing in favor of #2572

@rymnc rymnc closed this Jan 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants