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: prevent mempool panic #6239

Merged

Conversation

hansieodendaal
Copy link
Contributor

Description

Under certain conditions, it is possible to let the mempool panic when is inserting an empty transaction. This PR prevents it.

Motivation and Context

HAZOP finding

How Has This Been Tested?

Unit tests added

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

Review code changes and unit tests

Breaking Changes

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

Under certain conditions it is possible to let the mempool panic
when is inserting an empty transaction. This PR prevents it.
@hansieodendaal hansieodendaal requested a review from a team as a code owner March 27, 2024 10:09
Copy link
Collaborator

@stringhandler stringhandler left a comment

Choose a reason for hiding this comment

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

Where was the original panic? I can't see where it would happen from this pr

Copy link

github-actions bot commented Mar 27, 2024

Test Results (CI)

    3 files    120 suites   35m 33s ⏱️
1 273 tests 1 273 ✅ 0 💤 0 ❌
3 811 runs  3 811 ✅ 0 💤 0 ❌

Results for commit f90d99d.

♻️ 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 Mar 27, 2024
Copy link

github-actions bot commented Mar 27, 2024

Test Results (Integration tests)

33 tests  +33   33 ✅ +33   14m 49s ⏱️ + 14m 49s
11 suites +11    0 💤 ± 0 
 2 files   + 2    0 ❌ ± 0 

Results for commit f90d99d. ± Comparison against base commit 148e398.

♻️ This comment has been updated with latest results.

@hansieodendaal
Copy link
Contributor Author

hansieodendaal commented Mar 27, 2024

Where was the original panic? I can't see where it would happen from this pr

The panic is where we divide by zero weight in PrioritizedTransaction::new, here

priority: FeePriority::new(&transaction, insert_epoch, weight)?,

and here

fee_per_byte: transaction.body.get_total_fee()?.as_u64().saturating_mul(1000) / weight,

Copy link
Collaborator

@AaronFeickert AaronFeickert left a comment

Choose a reason for hiding this comment

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

LGTM

@SWvheerden SWvheerden merged commit 5380e1f into tari-project:development Apr 2, 2024
15 of 16 checks passed
@hansieodendaal hansieodendaal deleted the ho_prevent_mempool_panic branch April 2, 2024 09:35
sdbondi added a commit to sdbondi/tari that referenced this pull request Apr 15, 2024
* development:
  fix!: avoid `Encryptable` domain collisions (tari-project#6275)
  ci(fix): docker image build fix and ci improvements (tari-project#6270)
  feat: keep smt memory (tari-project#6265)
  feat: show warning when GRPC method is disallowed (tari-project#6246)
  fix(chat): metadata panic (tari-project#6247)
  feat: add monerod detection as an option to the merge mining proxy (tari-project#6248)
  chore(deps): bump h2 from 0.3.24 to 0.3.26 (tari-project#6250)
  feat: improve lmdb dynamic growth (tari-project#6242)
  feat: allow wallet type from db to have preference (tari-project#6245)
  feat: prevent mempool panic (tari-project#6239)
  ci: bump nightly version (tari-project#6241)
  feat: add validation for zero confirmation block sync (tari-project#6237)
  feat: new template with coinbase call (tari-project#6226)
  feat: improve wallet sql queries (tari-project#6232)
  chore: remove ahash as dependancy (tari-project#6238)
  feat: add dynamic growth to lmdb (tari-project#6231)
  chore(deps): bump borsh from 0.10.3 to 1.0.0 in /applications/minotari_ledger_wallet (tari-project#6236)
sdbondi added a commit to sdbondi/tari that referenced this pull request Apr 15, 2024
* development:
  fix!: avoid `Encryptable` domain collisions (tari-project#6275)
  ci(fix): docker image build fix and ci improvements (tari-project#6270)
  feat: keep smt memory (tari-project#6265)
  feat: show warning when GRPC method is disallowed (tari-project#6246)
  fix(chat): metadata panic (tari-project#6247)
  feat: add monerod detection as an option to the merge mining proxy (tari-project#6248)
  chore(deps): bump h2 from 0.3.24 to 0.3.26 (tari-project#6250)
  feat: improve lmdb dynamic growth (tari-project#6242)
  feat: allow wallet type from db to have preference (tari-project#6245)
  feat: prevent mempool panic (tari-project#6239)
  ci: bump nightly version (tari-project#6241)
  feat: add validation for zero confirmation block sync (tari-project#6237)
  feat: new template with coinbase call (tari-project#6226)
  feat: improve wallet sql queries (tari-project#6232)
  chore: remove ahash as dependancy (tari-project#6238)
  feat: add dynamic growth to lmdb (tari-project#6231)
  chore(deps): bump borsh from 0.10.3 to 1.0.0 in /applications/minotari_ledger_wallet (tari-project#6236)
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 P-reviews_required Process - Requires a review from a lead maintainer to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants