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: don't add substates that are already being processed #885

Merged
merged 2 commits into from
Jan 12, 2024

Conversation

Cifko
Copy link
Contributor

@Cifko Cifko commented Jan 10, 2024

Description

Don't add transaction to a proposal if any of their substates is being processed.

Motivation and Context

I left one TODO in the code. Because if we don't switch to the "no version" transaction, them we can add aborts directly to the proposal.

How Has This Been Tested?

I tried 9 concurrent transaction 9 accounts to 1, and 1 accounts to 9.

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

Breaking Changes

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

Copy link

github-actions bot commented Jan 10, 2024

Test Results (CI)

194 tests  ±0   194 ✅ +5   1h 31m 0s ⏱️ - 1h 14m 49s
 52 suites ±0     0 💤 ±0 
  2 files   ±0     0 ❌  - 5 

Results for commit 2065be3. ± Comparison against base commit b7ccff2.

♻️ This comment has been updated with latest results.

@Cifko Cifko force-pushed the disallow-same-substate-tx branch from 4273860 to 2065be3 Compare January 12, 2024 07:14
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

Ah I didn't initially think this was happening on the database layer, we should definitely write some docs on the expected behaviour of each method in the storage interface (#890). We may encounter other issues but this should fix the specific issue with incorrect proposals.

@sdbondi sdbondi added this pull request to the merge queue Jan 12, 2024
Merged via the queue into tari-project:development with commit 63a1563 Jan 12, 2024
10 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Jan 16, 2024
…ls (#894)

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
- [x] Requires data directory to be deleted (Evidence struct changed)
- [ ] Other - Please specify
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