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!: optional versions in transaction inputs #922

Merged
merged 40 commits into from
Feb 27, 2024

Conversation

mrnaveira
Copy link
Collaborator

@mrnaveira mrnaveira commented Jan 29, 2024

Description

  • Refactored transactions so inputs have optional versions. For that I reused the SubstateRequirement struct instead of the SubstateAddress we were using. This change cascaded through all the project.
  • Mempool skips execution of transactions that have some inputs without versions. It stores a mock result in database so the transaction is picked up when creating the next HotStuff block. We may want to change this behaviour later (e.g. real execution in mempool, or separated database table for those types of transactions).
  • Created a BlockTransactionExecutor used by the consensus when voting on local blocks (I leave remote consensus out of the scope for now). The goal of the executor is to allow the re-execution of transactions taking into account the outputs of previous transactions in the block. The executor will be initialised at the beginning of the block, and will keep a temporary substate memory during the block execution, so each consecutive transaction will read the most recent versions of the inputs from there. After a transaction finishes, the outputs will be used to update the memory.
  • After a transaction is executed in consensus, the results are stored in the SQLite database.

Motivation and Context

We want to allow concurrent transactions that affect the same substates. Think for example of a Uniswap-like pool, in which each transaction alters the pool ratio (price impact), in this case transactions must be executed in strict sequential order without conflicts.

The implementation of concurrent transactions is going to spawn across multiple PRs. The goal of this PR is to set the foundation needed for concurrency:

  • Allow for clients to send transactions whose inputs don't have specific versions. This is necessary because clients do not know which is the last version of the input they need in case of a high demand component.
  • Transaction execution at the HotStuff level instead of at mempool level for transactions without versions. The latest input version is selected for each transaction.

Note that transaction execution and consensus behaviour has not been changed for transactions with specific input versions (the default way of interacting with the network). To trigger the new behaviour the user has to manually craft and send to a VN a transaction that has one or more inputs without version.


Current Limitations and Future Work

  • The scope of this PR is concurrent single-shard transactions. Concurrent multi-shard transactions, that would involve more that one committee, are left for future work.
  • We may need a separated DB API for substates, with a memory implementation so each block can be processed in parallel without DB conflicts.
  • The TariDanBlockTransactionExecutor, which implements the consensus execution of transactions, has some TODO comments for improvements to be made.
  • In this PR concurrent transactions do not work (the second onwards fails) due to the behaviour of the pacemaker that we will need to address in future PRs:
    • Blocks seem to be generated very fast, at least in the cucumber scenarios (maybe due to having a single VN network), so it's not possible to force concurrent transactions into the same block. We may need a new network parameter to set up a minimum block time and use it in cucumber tests.
    • Also, there seems to be overlapping of blocks processing. Transactions of block N+x are executed in consensus before transactions in block N are stored into the DB. We will need strict sequential block execution so outputs of one transaction can be used as inputs in another transaction in a future block.

How Has This Been Tested?

New cucumber scenario for concurrent transaction execution. Currently only one transaction without input versions is executed due to the behaviour of the consensus pacemaker (check Current Limitations and Future Work)

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

Run the new cucumber scenario or try to manually send a transaction with no input versions to the VN. I recommend the counter template for testing as it's easy to craft transactions that affect only one substate.

Breaking Changes

  • None
  • Requires data directory to be deleted
  • Network reset. Now transactions inputs contain the substate ID + version, where up until now they were just a substate address (a hash). This change breaks both consensus and clients (UIs, JavaScript, etc.).

@ghpbot-tari-project ghpbot-tari-project added P-conflicts The PR has merge conflicts that need to be resolved P-acks_required P-reviews_required labels Jan 29, 2024
@ghpbot-tari-project ghpbot-tari-project removed the P-conflicts The PR has merge conflicts that need to be resolved label Jan 31, 2024
@mrnaveira mrnaveira changed the title wip: remove the version from substate address wip: optional versions in transaction inputs Feb 8, 2024
@ghpbot-tari-project ghpbot-tari-project added the P-conflicts The PR has merge conflicts that need to be resolved label Feb 8, 2024
@ghpbot-tari-project ghpbot-tari-project removed the P-conflicts The PR has merge conflicts that need to be resolved label Feb 12, 2024
@ghpbot-tari-project ghpbot-tari-project added the P-conflicts The PR has merge conflicts that need to be resolved label Feb 14, 2024
@ghpbot-tari-project ghpbot-tari-project removed the P-conflicts The PR has merge conflicts that need to be resolved label Feb 14, 2024
@mrnaveira mrnaveira changed the title wip: optional versions in transaction inputs feat: optional versions in transaction inputs Feb 22, 2024
@mrnaveira mrnaveira marked this pull request as ready for review February 22, 2024 23:23
Copy link

github-actions bot commented Feb 22, 2024

Test Results (CI)

488 tests  +1   488 ✅ +1   1h 46m 35s ⏱️ - 22m 58s
 60 suites +1     0 💤 ±0 
  2 files   ±0     0 ❌ ±0 

Results for commit beb9d7f. ± Comparison against base commit fe428b5.

♻️ This comment has been updated with latest results.

sdbondi
sdbondi previously approved these changes Feb 26, 2024
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.

ACK

Submitted some transactions and tested and seems to be working.
Nice, looks like a good first step - happy to merge

I suspect the builder interface on the spec can be removed in favour of passing in the executor directly, it seems to be a way to keep the borrow checker happy (immutable self, mutable execute) but this is very minor so no change required at this point.

We'll need to update typescript bindings (this is currently a manual process) by running npm run build in bindings/

@sdbondi sdbondi changed the title feat: optional versions in transaction inputs feat!: optional versions in transaction inputs Feb 26, 2024
@mrnaveira
Copy link
Collaborator Author

We'll need to update typescript bindings (this is currently a manual process) by running npm run build in bindings/

Done

@sdbondi sdbondi added this pull request to the merge queue Feb 27, 2024
Merged via the queue into tari-project:development with commit 4b5215a Feb 27, 2024
11 checks passed
sdbondi added a commit to sdbondi/tari-dan that referenced this pull request Feb 27, 2024
* development:
  fix(engine): prevent creation of mismatched resource type bucket (tari-project#940)
  chore: update to latest feature-dan2 (tari-project#937)
  feat!: optional versions in transaction inputs (tari-project#922)
  feat: add tari-generate (tari-project#941)
sdbondi added a commit to sdbondi/tari-dan that referenced this pull request Feb 27, 2024
* development:
  feat(walletd/js/client): use abstract transport to allow for WebRTC transport (tari-project#943)
  fix(engine): prevent creation of mismatched resource type bucket (tari-project#940)
  chore: update to latest feature-dan2 (tari-project#937)
  feat!: optional versions in transaction inputs (tari-project#922)
  feat: add tari-generate (tari-project#941)
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