Skip to content

Commit

Permalink
fix(core/base_node): safe mmr_position cast in horizon sync (#5503)
Browse files Browse the repository at this point in the history
Description
---
Perform safe numeric conversion of the `mmr_position` inside the horizon
state sync

Motivation and Context
---
There was an unsafe cast from `u64` to `u32` of the `mmr_position`
inside the horizon state sync. This PR does a safe cast, raising a typed
error instead of truncating the value.

The existing TODO comment suggested to do it inside the database
function, but in my opinion at this point this would overcomplicate the
solution.

How Has This Been Tested?
---
Unit tests

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

<!-- 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 -->

---------

Co-authored-by: stringhandler <stringhandler@gmail.com>
Co-authored-by: SW van Heerden <swvheerden@gmail.com>
  • Loading branch information
3 people authored Jun 26, 2023
1 parent 301ca49 commit fb3ac60
Show file tree
Hide file tree
Showing 2 changed files with 7 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ pub enum HorizonSyncError {
expected_hex: String,
actual_hex: String,
},
#[error("Invalid MMR position {mmr_position} at height {at_height}")]
InvalidMmrPosition { at_height: u64, mmr_position: u64 },
#[error("Invalid range proof for output:{0} : {1}")]
InvalidRangeProof(String, String),
#[error("RPC error: {0}")]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -319,11 +319,11 @@ impl<'a, B: BlockchainBackend + 'static> HorizonStateSynchronization<'a, B> {

kernel_hashes.push(kernel.hash());

// TODO: We mix u32 and u64 for the mmr length. This comes down to the use of a 32-bit croaring Bitmap.
// Suggest should use u64 externally (u64 is in the header) and error on the database calls if they
// are > u32::MAX. Remove the clippy exception once fixed.
#[allow(clippy::cast_possible_truncation)]
txn.insert_kernel_via_horizon_sync(kernel, *current_header.hash(), mmr_position as u32);
let mmr_position_u32 = u32::try_from(mmr_position).map_err(|_| HorizonSyncError::InvalidMmrPosition {
at_height: current_header.height(),
mmr_position,
})?;
txn.insert_kernel_via_horizon_sync(kernel, *current_header.hash(), mmr_position_u32);
if mmr_position == current_header.header().kernel_mmr_size - 1 {
let num_kernels = kernel_hashes.len();
debug!(
Expand Down

0 comments on commit fb3ac60

Please sign in to comment.