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: allow multiple concurrent transactions into the same block #970

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

Conversation

mrnaveira
Copy link
Collaborator

Description

  • Avoid adding inputs without versions to the input evidence. Before we were adding always the version 0.
  • The count for involved shards (used for fee calculation) is calculated in a special case for transactions without versions

Motivation and Context

Following the previous work on concurrency (#922), this PR solves the problem of concurrent transactions not being batched together in the same block.

The problem happened on the new block calculation, the database function transaction_pool_get_many_ready uses the evidence field to detect conflicts and select transactions. The evidence field for inputs without version was always adding the version 0, which result on blocks only picking up one concurrent transaction per block.

This PR solves this problem by avoiding adding inputs without versions into the evidence field.

How Has This Been Tested?

Using the concurrency cucumber scenario, setting a lot of concurrent transactions (e.g. 100) and checking logs and database for block commands. Transactions eventually fail but this is due to other problems in concurrency.

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

See previous section

Breaking Changes

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

@mrnaveira mrnaveira marked this pull request as ready for review March 12, 2024 15:40
Copy link

github-actions bot commented Mar 12, 2024

Test Results (CI)

499 tests   - 23   498 ✅  - 24   1h 43m 21s ⏱️ - 19m 48s
 50 suites  - 12     0 💤 ± 0 
  1 files    -  1     1 ❌ + 1 

For more details on these failures, see this check.

Results for commit d7cc4a3. ± Comparison against base commit 5ecf68d.

This pull request removes 23 tests.
Scenario: Claim and transfer confidential assets via wallet daemon: tests/features/wallet_daemon.feature:89:3
Scenario: Claim base layer burn funds with wallet daemon: tests/features/claim_burn.feature:7:3
Scenario: Claim validator fees: tests/features/claim_fees.feature:6:3
Scenario: Concurrent calls to the Counter template: tests/features/concurrency.feature:7: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:3
Scenario: Create and mint account NFT: tests/features/wallet_daemon.feature:133:3
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:45: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.

This appears to be a workaround for the partial implementation of late execution in consensus. The evidence should always contain all involved shards, and working around it may cause unexpected bugs. I think this would revert to the previous code when we implement a more complete solution for "consensus-executed" transactions. Currently, we insert a "dummy" executed transaction in the mempool.

Consensus can fetch unexecuted transaction using TransactionRecord::get (loads either executed or unexecuted) instead of ExecutedTransaction::get (only executed) in some places and take appropriate action if the transaction was not executed in the mempool. I'd rather work towards correcting this than merge a workaround if that makes sense.

// TODO: update the evidence after execution so all transactions are treated equally here
let db_transaction = t.get_transaction(tx)?;
let involved = if db_transaction.transaction().has_inputs_without_version() {
db_transaction.transaction().all_inputs_iter().count()
Copy link
Member

Choose a reason for hiding this comment

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

The number of inputs is not necessarily (and usually not) the number of involved shards

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