Skip to content

Ensure validation of transaction hashes to provide to the prover #1774

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 17 commits into from
Jun 25, 2024
Merged

Ensure validation of transaction hashes to provide to the prover #1774

merged 17 commits into from
Jun 25, 2024

Conversation

@dlachaume
Copy link
Collaborator

@dlachaume dlachaume commented Jun 24, 2024 •
edited
Loading

Content

This PR includes a validation step for the transaction hashes transmitted via the HTTP route /proof/cardano-transaction. It was implemented with:

  • a new configuration parameter for the aggregator node: cardano_transactions_prover_max_hashes_allowed_by_request that limits the number of hashes accepted as parameters on the /proof/cardano-transaction route.
  • a new structure cardano_transactions_prover exposed on the root route /.
    This structure is stored within the existing capabilities structure and currently contains only the previous configuration parameter value under the property name max_hashes.
  • a validator that returns an error if the hash validation criteria are not met (empty string, length != 64, ASCII hexadecimal digit characters only). In case of an error, a 400 Bad Request is returned on the /proof/cardano-transaction route.

Pre-submit checklist

  • Branch
    • Tests are provided (if possible)
    • Crates versions are updated (if relevant)
    • CHANGELOG file is updated (if relevant)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested
  • Documentation
    • Update README file (if relevant)
    • Update documentation website (if relevant)
    • Add dev blog post (if relevant)

Issue(s)

Closes #1757

@github-actions
Copy link

github-actions bot commented Jun 24, 2024 •
edited
Loading

Test Results

    4 files  ± 0     51 suites  ±0   8m 44s ⏱️ +4s
1 089 tests +10  1 089 ✅ +10  0 💤 ±0  0 ❌ ±0 
1 237 runs  +10  1 237 ✅ +10  0 💤 ±0  0 ❌ ±0 

Results for commit 9f63490. ± Comparison against base commit f5bf868.

♻️ This comment has been updated with latest results.

@dlachaume dlachaume temporarily deployed to testing-preview June 24, 2024 15:36 — with GitHub Actions Inactive
@dlachaume dlachaume temporarily deployed to testing-sanchonet June 24, 2024 15:36 — with GitHub Actions Inactive
@dlachaume dlachaume changed the title Add hashes validator on aggregator /proof/cardano-transaction route Ensure validation of transaction hashes to provide to the prover Jun 24, 2024
@dlachaume dlachaume marked this pull request as ready for review June 24, 2024 15:58
@dlachaume dlachaume self-assigned this Jun 24, 2024
sfauvel
sfauvel approved these changes Jun 25, 2024
View reviewed changes
Copy link
Collaborator

@sfauvel sfauvel left a comment

Choose a reason for hiding this comment

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

Just one comment

dlachaume and others added 13 commits June 25, 2024 10:49
@dlachaume @Alenar @sfauvel
Implement test tools equivalent_to and assert_equivalent
486f189 
Co-authored-by: DJO <[email protected]>
Co-authored-by: Sébastien Fauvel <[email protected]>
@dlachaume @sfauvel @Alenar
Handle deduplicate list of transactions hashes in `compute_transactio…
05c197b 
…ns_proofs`

Co-authored-by: Sébastien Fauvel <[email protected]>
Co-authored-by: DJO <[email protected]>
@dlachaume @Alenar @sfauvel
Handle empty cardano transactions hash in prover
e618751 
Co-authored-by: DJO <[email protected]>
Co-authored-by: Sébastien Fauvel <[email protected]>
@dlachaume @Alenar @sfauvel
Create a dedicated structure that filter a list of Cardano transactio…
7cc4a95 
…ns hashes

Co-authored-by: DJO <[email protected]>
Co-authored-by: Sébastien Fauvel <[email protected]>
@dlachaume @sfauvel
Add a limit for the number of transactions hashes to be certified by …
46dbc10 
…the prover

Co-authored-by: Sébastien Fauvel <[email protected]>
@dlachaume @Alenar @sfauvel
Add a validator for the transactions hash list that will be called on…
d3ec53c 
… the HTTP prover route

Co-authored-by: DJO <[email protected]>
Co-authored-by: Sébastien Fauvel <[email protected]>
@dlachaume @Alenar
Implement the validator in the proof route
48f4af1 
Co-authored-by: DJO <[email protected]>
@dlachaume @sfauvel @Alenar
Handle deduplicate transactions hashes in the proof route
809d243 
Co-authored-by: Sébastien Fauvel <[email protected]>
Co-authored-by: DJO <[email protected]>
@dlachaume @sfauvel @Alenar
Extend aggregator configuration by adding the maximum number of trans…
9ec5039 
…actions hashes allowed by request to the prover

Co-authored-by: Sébastien Fauvel <[email protected]>
Co-authored-by: DJO <[email protected]>
@dlachaume @sfauvel @Alenar
Add aggregator parameter `cardano_transactions_prover_max_hashes_allo…
1d107d4 
…wed_by_request` in `root_routes` HTTP response

Co-authored-by: Sébastien Fauvel <[email protected]>
Co-authored-by: DJO <[email protected]>
@dlachaume @sfauvel @Alenar
Revert prover modifications after moving control and limit logic in…
fc107c9 
… the `proof_route`

Co-authored-by: Sébastien Fauvel <[email protected]>
Co-authored-by: DJO <[email protected]>
@dlachaume @sfauvel @Alenar
Handle Cardano transactions prover capability in explorer
9a4fc2d 
Co-authored-by: Sébastien Fauvel <[email protected]>
Co-authored-by: DJO <[email protected]>
@dlachaume @Alenar @sfauvel
Fix wrong key for `cardano_transactions_database_connection_pool_size…
845a532 
…` parameter to build the default configuration

Co-authored-by: DJO <[email protected]>
Co-authored-by: Sébastien Fauvel <[email protected]>
@dlachaume dlachaume force-pushed the ensemble/1757/limit-cardano-transactions-prover-input branch from f910d58 to 845a532 Compare June 25, 2024 08:50
@dlachaume dlachaume temporarily deployed to testing-preview June 25, 2024 08:57 — with GitHub Actions Inactive
@dlachaume dlachaume temporarily deployed to testing-sanchonet June 25, 2024 08:57 — with GitHub Actions Inactive
jpraynaud
jpraynaud reviewed Jun 25, 2024
View reviewed changes
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

I'm a bit concerned about the scope of the validator which should be responsible for computing the list of transactions hashes used by the prover.

@dlachaume @Alenar @sfauvel
Refacto validator error messages and generic types in ClientError c…
8316a80 
…onstructor

Co-authored-by: DJO <[email protected]>
Co-authored-by: Sébastien Fauvel <[email protected]>
@dlachaume dlachaume temporarily deployed to testing-preview June 25, 2024 13:03 — with GitHub Actions Inactive
@dlachaume dlachaume temporarily deployed to testing-sanchonet June 25, 2024 13:03 — with GitHub Actions Inactive
@dlachaume dlachaume force-pushed the ensemble/1757/limit-cardano-transactions-prover-input branch from 53ba1a7 to 7a9e6ea Compare June 25, 2024 13:25
@dlachaume @Alenar @sfauvel
Bump openapi.yml and mithril-explorer versions
20c74ff 
Co-authored-by: DJO <[email protected]>
Co-authored-by: Sébastien Fauvel <[email protected]>
jpraynaud
jpraynaud approved these changes Jun 25, 2024
View reviewed changes
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM

@dlachaume @Alenar @sfauvel
Bump crates versions
9f63490 
Co-authored-by: DJO <[email protected]>
Co-authored-by: Sébastien Fauvel <[email protected]>
@dlachaume dlachaume temporarily deployed to testing-preview June 25, 2024 14:12 — with GitHub Actions Inactive
@dlachaume dlachaume temporarily deployed to testing-sanchonet June 25, 2024 14:12 — with GitHub Actions Inactive
@dlachaume dlachaume merged commit 47c2bb8 into main Jun 25, 2024
@dlachaume dlachaume deleted the ensemble/1757/limit-cardano-transactions-prover-input branch June 25, 2024 14:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Alenar Alenar Alenar approved these changes

@jpraynaud jpraynaud jpraynaud approved these changes

+1 more reviewer

@sfauvel sfauvel sfauvel approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dlachaume dlachaume

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Limit Cardano transactions prover input

5 participants

@dlachaume @Alenar @sfauvel @jpraynaud