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(balanced_mp): removes some panics, adds some checks and new tests #5432

Merged
merged 1 commit into from
Jun 6, 2023

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented May 31, 2023

Description

  • number of minor optimisations (e.g MergedBalancedBinaryMerkleIndexOrHash type vs tuple with index uint to Vec conversion)
  • removed a number of possible panics
  • adds integer underflow check in BalancedMerkleProof::verify
  • adds edge case check for if all proofs to merge are zero height
  • adds some extra tests for faulty proofs and "real world" data test
  • minor memory optimisations
  • format cargo.toml and reduce minimum required versions for some dependencies

Motivation and Context

None of these changes are game changing however they do address some important bugs that could prevent a bad actor from causing panics in a remote node.

@Cifko please check that I didn't break anything.

I wanted to investigate this because of an issue in a test in the DAN repo that fails with an invalid BMT even though all unmerged proofs are valid. I imported this data and strangely, the proof verifies (both in the previous code and code from this PR), so I was unable to track down the precise reason for the failure.

How Has This Been Tested?

New and existing unit tests

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

Run tests

Breaking Changes

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

@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 31, 2023
@github-actions
Copy link

github-actions bot commented May 31, 2023

Test Results (CI)

1 153 tests  +5   1 153 ✔️ +5   10m 5s ⏱️ - 5m 41s
     37 suites ±0          0 💤 ±0 
       1 files   ±0          0 ±0 

Results for commit 54ca4e2. ± Comparison against base commit 0c1e576.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

Test Results (Integration tests)

  2 files  ±0  12 suites  ±0   17m 47s ⏱️ -7s
29 tests ±0  28 ✔️ ±0  0 💤 ±0  1 ±0 
31 runs  ±0  28 ✔️ ±0  0 💤 ±0  3 ±0 

For more details on these failures, see this check.

Results for commit 54ca4e2. ± Comparison against base commit 0c1e576.

Copy link
Contributor

@Cifko Cifko left a comment

Choose a reason for hiding this comment

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

Perfect, thanks

@ghpbot-tari-project ghpbot-tari-project removed the P-reviews_required Process - Requires a review from a lead maintainer to be merged label May 31, 2023
@stringhandler stringhandler merged commit 602f416 into tari-project:development Jun 6, 2023
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.

4 participants