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: add check to verify mempool state #6316

Merged

Conversation

hansieodendaal
Copy link
Contributor

Description

Added a check to verify that the mempool last processed block is in sync with the blockchain state whenever a new block template is requested, as adding a block to the db and processing the same block in the mempool happens asynchronously. Miners can get block templates with double spending if this is not done.

Motivation and Context

In a recent stress test the following behaviour was observed multiple times:

  1. [mempool] New transaction 4776...2001 received with output 000e9...6b57c
  2. [mempool] Transaction 4776d...82001 inserted into the mempool
  3. [grpc] Miner mined block af0c8...c76c0 containing output 000e9...6b57c
  4. [lmdb_db] Output 000e9...6b57c inserted into the db
  5. [grpc] New block template requested (`GetNewBlockTemplate)
  6. [mempool] New block template prepared containing output 000e9...6b57c
  7. [mempool] Transaction 4776d...82001 with output 000e9...6b57c removed from the mempool
  8. [mempool] Processed new block af0c8...c76c0 containing output 000e9...6b57c
  9. [grpc] New block requested (GetNewBlock)
  10. [grpc] Using the latest block template, pub fn prepare_new_block(&self, template: NewBlockTemplate) fails at let roots = match calculate_mmr_roots(&*db, self.rules(), &block, &mut smt) with ERROR Output commitment(000e9e1fd41f672e0b88f89b3d24390beb37d50e8be2967980e24a13d796b57c) already in SMT

How Has This Been Tested?

System-level stress test to be done

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

Review code change

Breaking Changes

  • None
  • Requires data directory on base node to be deleted
  • Requires hard fork
  • Other - Please specify

Added a check to verify that the mempool last processed block is in sync with the
blockchain state whenever a new block template is requested, as adding a block to
the db and processing the same block in the mempool happens asynchronously. If this
is not done, miners can get block templates with double spends.
@hansieodendaal hansieodendaal requested a review from a team as a code owner May 2, 2024 12:32
Copy link

github-actions bot commented May 2, 2024

Test Results (CI)

    3 files    120 suites   35m 35s ⏱️
1 277 tests 1 277 ✅ 0 💤 0 ❌
3 823 runs  3 823 ✅ 0 💤 0 ❌

Results for commit 1c32bc1.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added P-acks_required Process - Requires more ACKs or utACKs P-reviews_required Process - Requires a review from a lead maintainer to be merged labels May 2, 2024
SWvheerden
SWvheerden previously approved these changes May 2, 2024
Copy link
Collaborator

@SWvheerden SWvheerden left a comment

Choose a reason for hiding this comment

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

lets see how this performs

@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label May 2, 2024
Copy link

github-actions bot commented May 2, 2024

Test Results (Integration tests)

 2 files  + 2  11 suites  +11   40m 24s ⏱️ + 40m 24s
33 tests +33  31 ✅ +31  0 💤 ±0  2 ❌ +2 
37 runs  +37  31 ✅ +31  0 💤 ±0  6 ❌ +6 

For more details on these failures, see this check.

Results for commit 1c32bc1. ± Comparison against base commit ac8558e.

♻️ This comment has been updated with latest results.

@ghpbot-tari-project ghpbot-tari-project added the P-reviews_required Process - Requires a review from a lead maintainer to be merged label May 3, 2024
@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label May 3, 2024
@SWvheerden SWvheerden merged commit 925d29a into tari-project:development May 3, 2024
14 of 16 checks passed
@hansieodendaal hansieodendaal deleted the ho_mempool_out_of_sync branch May 3, 2024 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P-acks_required Process - Requires more ACKs or utACKs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants