Skip to content

Commit

Permalink
fix(balanced_mp): removes some panics, adds some checks and new tests (
Browse files Browse the repository at this point in the history
…#5432)

Description
---
- number of minor optimisations (e.g
MergedBalancedBinaryMerkleIndexOrHash type vs tuple with index uint to
Vec<u8> 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
<!-- Checklist -->
<!-- 1. Is the title of your PR in the form that would make nice release
notes? The title, excluding the conventional commit
tag, will be included exactly as is in the CHANGELOG, so please think
about it carefully. -->


Breaking Changes
---

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

<!-- Does this include a breaking change? If so, include this line as a
footer -->
<!-- BREAKING CHANGE: Description what the user should do, e.g. delete a
database, resync the chain -->
  • Loading branch information
sdbondi authored Jun 6, 2023
1 parent 50fa6b6 commit 602f416
Show file tree
Hide file tree
Showing 4 changed files with 297 additions and 114 deletions.
10 changes: 5 additions & 5 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

26 changes: 13 additions & 13 deletions base_layer/mmr/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,19 @@ native_bitmap = ["croaring"]
benches = ["criterion"]

[dependencies]
tari_utilities = "0.4.10"
tari_crypto = { version = "0.16"}
tari_common = {path = "../../common"}
thiserror = "1.0.26"
borsh = "0.9.3"
digest = "0.9.0"
tari_utilities = "0.4"
tari_crypto = { version = "0.16" }
tari_common = { path = "../../common" }
thiserror = "1.0"
borsh = "0.9"
digest = "0.9"
log = "0.4"
serde = { version = "1.0.97", features = ["derive"] }
croaring = { version = "0.5.2", optional = true }
criterion = { version="0.2", optional = true }
serde = { version = "1.0", features = ["derive"] }
croaring = { version = "0.5", optional = true }
criterion = { version = "0.2", optional = true }

[dev-dependencies]
rand="0.8.0"
rand = "0.8.0"
blake2 = "0.9.0"
serde_json = "1.0"
bincode = "1.1"
Expand All @@ -39,6 +39,6 @@ name = "bench"
harness = false

[[test]]
name="tari_mmr_integration_tests"
path="tests/mmr_integration_tests.rs"
required-features=["native_bitmap"]
name = "tari_mmr_integration_tests"
path = "tests/mmr_integration_tests.rs"
required-features = ["native_bitmap"]
Loading

0 comments on commit 602f416

Please sign in to comment.