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

feat(indexer): quorum certificate validation #893

Open
wants to merge 11 commits into
base: development
Choose a base branch
from

Conversation

mrnaveira
Copy link
Collaborator

@mrnaveira mrnaveira commented Jan 14, 2024

Description

  • VNs now include QCs in the GetSubstateResponse type.
  • Unified the QC validation function inside the consensus crate, and reuse it for block validations inside the VN as well as in the indexer.
  • Created a new error type for QC validation errors
  • Moved the TariSignatureService to tari_dan_app_utilities, so it can be reused from VNs and indexers.

Motivation and Context

Right now indexers and wallets trust the substate values received from VNs. Ideally we would want this process to be totally trustless by validating a QC sent by the VN alongside the substate value.

But right now this is not possible as the QC does not include the appropriate information for validating the substate value. Anyway, the goal of this PR to implement the boilerplate needed as the first step towards trustless VNs:

  • Add the QC to the VN responses along the substate values
  • Perform a initial QC validation: signatures, merkle root and minimum quorum. As mentioned earlier, in the future (when it's possible) we would like to check that a substate value is validated by the QC.

How Has This Been Tested?

Related integration tests pass

What process can a PR reviewer use to test or verify this change?

Not applicable

Breaking Changes

  • None
  • Requires data directory to be deleted
  • Indexers now expect a valid QC from VNs, so older VNs are incompatible

Copy link

github-actions bot commented Jan 14, 2024

Test Results (CI)

80 tests   - 367   79 ✅  - 368   59m 36s ⏱️ - 1h 51m 7s
16 suites  -  41    0 💤 ±  0 
 1 files    -   1    1 ❌ +  1 

For more details on these failures, see this check.

Results for commit 6f5dca6. ± Comparison against base commit b526c09.

This pull request removes 367 tests.
Scenario: Claim and transfer confidential assets via wallet daemon: tests/features/wallet_daemon.feature:89:5
Scenario: Claim base layer burn funds with wallet daemon: tests/features/claim_burn.feature:6:3
Scenario: Claim validator fees: tests/features/claim_fees.feature:6:3
Scenario: Confidential transfer to unexisting account: tests/features/transfer.feature:163:3
Scenario: Counter template registration and invocation: tests/features/counter.feature:7:3
Scenario: Create account and transfer faucets via wallet daemon: tests/features/wallet_daemon.feature:7:5
Scenario: Create and mint account NFT: tests/features/wallet_daemon.feature:133:5
Scenario: Create resource and mint in one transaction: tests/features/nft.feature:73:3
Scenario: Double Claim base layer burn funds with wallet daemon. should fail: tests/features/claim_burn.feature:44:3
Scenario: Indexer GraphQL requests events over network substate indexing: tests/features/indexer.feature:118:3
…

♻️ This comment has been updated with latest results.

@mrnaveira mrnaveira marked this pull request as ready for review January 15, 2024 09:52
sdbondi
sdbondi previously approved these changes Jan 16, 2024
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

utACK

@sdbondi sdbondi added this pull request to the merge queue Jan 17, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants