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: qc was validated based on current epoch, not the qc's epoch #896

Merged
merged 2 commits into from
Jan 16, 2024

Conversation

Cifko
Copy link
Contributor

@Cifko Cifko commented Jan 15, 2024

Description

The qc validation was based on the epoch of the candidate block. So if the QC was from previous epoch the committee shard could be different. E.g. when you add a new VN into the committee, the qc was expected to have 2 signatures. But in the previous epoch there was only one VN so the QC never passed the validation.

Motivation and Context

How Has This Been Tested?

Start dan-testing with 1 VN, add a new VN and mine it into the committee. Without this change you would never see a new block coming in. You still have to wait for some leader failure on the epoch change but eventually new blocks starts coming in.

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

As above.

Breaking Changes

  • None
  • Requires data directory to be deleted
  • Other - Please specify

Copy link

github-actions bot commented Jan 15, 2024

Test Results (CI)

174 tests   - 20   167 ✅  - 27   3h 21m 53s ⏱️ + 1h 39m 4s
 42 suites  - 10     0 💤 ± 0 
  1 files    -  1     7 ❌ + 7 

For more details on these failures, see this check.

Results for commit a66f9d5. ± Comparison against base commit 63a1563.

This pull request removes 20 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.

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.

LGTM

@sdbondi sdbondi enabled auto-merge January 16, 2024 07:02
@sdbondi sdbondi added this pull request to the merge queue Jan 16, 2024
Merged via the queue into tari-project:development with commit 60b067c Jan 16, 2024
9 checks passed
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