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(core)!: fix potential panic for sidechain merkle root with incorrect length #3788

Conversation

sdbondi
Copy link
Member

@sdbondi sdbondi commented Feb 2, 2022

Description

Fixes potential panic if a side-chain Merkle root of incorrect length is accepted by the blockchain.

Motivation and Context

Using a fixed 32-byte array forces any implementor to produce or validate the correct number of bytes. Esp. now that const generics are available, for performance and correctness, all hashes in the system should be fixed to 32 bytes and not held in a dynamic heap allocated vector.

I've changed the merkle_root type in SideChainCheckpoint because this is a relatively minor change.

This is a chain storage breaking change.

How Has This Been Tested?

Existing tests, manually

@sdbondi sdbondi force-pushed the core-side-chain-features-use-fixed-merkle branch from 9674b22 to 110eac4 Compare February 2, 2022 08:37
@sdbondi sdbondi force-pushed the core-side-chain-features-use-fixed-merkle branch from 110eac4 to 2fd5f1e Compare February 3, 2022 05:11
Copy link
Contributor

@philipr-za philipr-za left a comment

Choose a reason for hiding this comment

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

Looks good, death to the unwrap!


use tari_utilities::ByteArrayError;

pub fn copy_into_fixed_array<T: Default + Copy, const SZ: usize>(elems: &[T]) -> Result<[T; SZ], ByteArrayError> {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

Copy link
Member Author

Choose a reason for hiding this comment

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

:) Can probably be wrapped into a FixedHash type in future

@aviator-app aviator-app bot merged commit b3cc6f2 into tari-project:development Feb 4, 2022
sdbondi added a commit to sdbondi/tari that referenced this pull request Feb 8, 2022
* development:
  refactor: reduce log level of some messages (tari-project#3804)
  ci: move Apple IDs into envs (tari-project#3797)
  feat: read asset definitions from base layer (tari-project#3802)
  fix: bump flood ban messages config (tari-project#3799)
  docs: readme fixes (tari-project#3800)
  feat(wallet_ffi)!:  add base node connectivity callback to wallet ffi (tari-project#3796)
  fix: minor fixes on collectibles (tari-project#3795)
  refactor: outbound message pipeline, threads and mempool improvements (tari-project#3792)
  feat(collectibles): add delete committee member button (tari-project#3786)
  fix(comms): minor edge-case fix to handle inbound connection while dialing (tari-project#3785)
  fix(core)!: fix potential panic for sidechain merkle root with incorrect length (tari-project#3788)
  fix(core): reduce one block behind waiting period (tari-project#3798)
  feat: add specific LibWallet error code for “Fee is greater than amount” (tari-project#3793)
  fix: coinbase output recovery bug (tari-project#3789)
  feat(base-node): add number of active sync peers metric (tari-project#3784)
  chore: script to keep .gitkeep file (tari-project#3787)
  feat: re-use scanned range proofs (tari-project#3764)
  feat: ability to compile on stable rust (tari-project#3759)
  feat: add logging and config to collectibles (tari-project#3781)
  chore: remove debug info from get-mempool-state (tari-project#3782)
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.

3 participants