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(consensus)!: allow multiple read-only shard references in proposals #894

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Jan 15, 2024

Description

Includes locks in evidence
Checks locks when proposing to allow multiple input refs

Motivation and Context

Evidence for each shard within a TransactionAtom now includes the lock type.
Updated the proposal db query to check each lock type and ensure that conflicts either don't occur or if they do they are all read locks.

How Has This Been Tested?

Ran a stress test from #880, previously after #885 was merged, funding the tariswap components would take a very long time (I've never actually run it to completion, but ran for 30 mins without completing). With this PR funding took roughly a minute on my test. Swap batches are also reaching finalization within an acceptable timeframe.

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

Submit multiple transactions which use a single substate as an input ref and check that they can be added to the same block.

Breaking Changes

  • None
  • Requires data directory to be deleted (Evidence struct changed)
  • Other - Please specify

Copy link

github-actions bot commented Jan 15, 2024

Test Results (CI)

73 tests   - 121   72 ✅  - 122   41m 45s ⏱️ - 1h 1m 4s
16 suites  -  36    0 💤 ±  0 
 1 files    -   1    1 ❌ +  1 

For more details on these failures, see this check.

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

This pull request removes 121 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
Collaborator

@mrnaveira mrnaveira 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 added this pull request to the merge queue Jan 16, 2024
Merged via the queue into tari-project:development with commit 651016d Jan 16, 2024
10 of 11 checks passed
@sdbondi sdbondi deleted the consensus-handle-input-refs-no-conflict branch January 16, 2024 10:35
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