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: add sanity checks to prepare_new_block #3448

Merged

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Oct 12, 2021

Description

  • Add basic sanity checks to prepare_new_block
  • Check calc_median_timestamp invariant holds when used in prepare_new_block
  • get_new_block GRPC call returns invalid argument error if invalid arguments are passed to prepare_new_block
  • Add new unit tests (tests previous panic)

Motivation and Context

Possible panic observed in cucumber test is prevented

Testing scenario "Verify all coinbases in hybrid mining are accounted for"
.seed: SEED_A
........[mmProxy client] Tip at 0 ...(stopping at 100)
stderr: thread 'tokio-runtime-worker' panicked at 'calc_median_timestamp: timestamps cannot be empty', base_layer/core/src/validation/helpers.rs:79:5

stderr: stack backtrace:

stderr:    0: std::panicking::begin_panic

stderr:    1: tari_core::validation::helpers::calc_median_timestamp
   2: tari_core::chain_storage::blockchain_database::BlockchainDatabase<B>::prepare_new_block
   3: tari_core::chain_storage::async_db::trace_log
   4: tokio::loom::std::unsafe_cell::UnsafeCell<T>::with_mut

stderr:    5: tokio::runtime::task::harness::Harness<T,S>::poll

stderr:    6: tokio::runtime::blocking::pool::Inner::run
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[mmProxy client] Tip at 35 ...(stopping at 100)
[mmProxy client] Tip at 95 ...(stopping at 100)

How Has This Been Tested?

New unit tests + existing tests

@sdbondi sdbondi force-pushed the core-calc-median-timestamp-panic branch from 46c40c4 to b22c583 Compare October 12, 2021 13:38
delta1
delta1 previously approved these changes Oct 12, 2021
Copy link
Contributor

@delta1 delta1 left a comment

Choose a reason for hiding this comment

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

Nice LGTM

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.

One of the sanity checks goes against a current config option/feature. We need to remove one of the two.

* development:
  chore: improve cucumber tags and run time speed (tari-project#3439)
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.

LGTM

@aviator-app aviator-app bot merged commit 76bc1f0 into tari-project:development Oct 14, 2021
@sdbondi sdbondi deleted the core-calc-median-timestamp-panic branch October 14, 2021 19:11
sdbondi added a commit to sdbondi/tari that referenced this pull request Oct 15, 2021
* development:
  fix: add display_currency_decimal method (tari-project#3445)
  fix: add sanity checks to prepare_new_block (tari-project#3448)
  test: profile wallet sqlite database operations (tari-project#3451)
  test: create cucumber test directory if necessary (tari-project#3452)
  chore: improve cucumber tags and run time speed (tari-project#3439)
@sdbondi sdbondi restored the core-calc-median-timestamp-panic branch February 3, 2022 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants