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)!: read-only resource + other fixes #1134

Merged

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Sep 5, 2024

Description

  • Resources are locked as read-only in consensus
  • Greatly reduce the size of the fee breakdown in transaction receipt
  • Fix mempool incorrect involvement detection
  • Fix consensus crash if versioned substate requirement is already DOWN
  • Fix off-by-one error when sending state transitions in state sync

Motivation and Context

Resources are highly contended and therefore are read-only after creation. Any transaction that mutates the resource will fail however the template API has not changed in this PR to reflect that.

Any mints/burns after the initial mint will succeed only in the local-only case, multi-committee mints/burns will fail.

Ideas for improvements:

  1. Easy way: Allow user to specify read/write for resources (or perhaps substates in general). Cons: poorer UX
  2. Complex: initially locked as read-only until AllPrepared phase. Transaction is executed and if the transaction results in a write for the resource. The write lock is queued and no further progress can be made on the transaction until all preceding write/read locks are released. Once this occurs the transaction is re-executed and LocalAccept proposed with as a write.
  3. To improve concurrency, a resource issuer may lock their resource for a period guaranteeing that no minting/burning or access rule changes can occur until that period expires.
  4. Automatically and implicitly read lock a resource for a guaranteed period of time (epochs). A user may submit a transaction that updates the resource, but that transaction will not be sequenced until the lock expires.
  5. Add a resource mint/burn instruction which will only involve a single shard. This will naturally "park" the transaction until any multishard locks are released and a local-only mint/burn/access rule update is performed.
  6. Eventual consistency: Total supply and access rule changes will eventually reflect. All resource operations are logged and eventually applied in an atomic way across shard groups, perhaps with a special signed cross-shard message that will be locally proposed.
  7. Apply point (1) and check access rules for the resource. If the signer is not permitted to write the resource at locking/pledging time (before execution), the transaction is aborted. This is more related to security, not concurrency.

How Has This Been Tested?

Manually

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

Submit transactions that concurrently read from the resource address.

Breaking Changes

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

@sdbondi sdbondi changed the title fix(consensus): read-only resource + other fixes fix(consensus)!: read-only resource + other fixes Sep 5, 2024
Copy link

github-actions bot commented Sep 5, 2024

Test Results (CI)

561 tests   - 1   561 ✅  - 1   1h 41m 46s ⏱️ + 9m 23s
 61 suites ±0     0 💤 ±0 
  2 files   ±0     0 ❌ ±0 

Results for commit fd7671a. ± Comparison against base commit 5431162.

This pull request removes 1 test.
tari_dan_engine::test ‑ nft_indexes::new_nft_index

♻️ This comment has been updated with latest results.

@sdbondi sdbondi force-pushed the consensus-read-only-resource branch 2 times, most recently from 055f244 to 9bcfcae Compare September 6, 2024 13:18
@sdbondi sdbondi marked this pull request as ready for review September 9, 2024 04:44
@sdbondi sdbondi merged commit f80d338 into tari-project:development Sep 16, 2024
12 checks passed
@sdbondi sdbondi deleted the consensus-read-only-resource branch September 16, 2024 12:10
sdbondi added a commit to sdbondi/tari-dan that referenced this pull request Sep 16, 2024
* development:
  fix(consensus)!: read-only resource + other fixes (tari-project#1134)
  test(cucumber): add concurrent wallet daemon call (tari-project#1140)
  feat!: new account constructor for custom access rules (tari-project#1138)
  test(cucumber): nft and fungible features (tari-project#1137)
  test(cucumber): enable counter template scenario (tari-project#1136)
  feat(consensus)!: add sidechain id in the genesis block (tari-project#1135)
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.

2 participants