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

test: fix mutable MMR test and add documentation #5276

Closed

Conversation

AaronFeickert
Copy link
Collaborator

@AaronFeickert AaronFeickert commented Mar 28, 2023

Description

Fixes a broken test of mutable MMR functionality. Updates documentation.

Closes issue 5268.

Motivation and Context

A mutable MMR test is currently broken. The test checks a utility function (which is currently unused) against computation of a mutable pruned MMR root. The test fails intermittently due to a failure to compress the underlying deletion bitmap prior to computing the Merkle root. This PR adds the necessary compression step to fix the test.

However, the test failure reveals a risk. The functionality used to compute the root for a mutable MMR implicitly requires that the caller have already compressed the deletion bitmap if any deletions were performed. If the caller doesn't do this (as in the failed test), the root may be different.

It may be prudent, if feasible, to include compression in get_merkle_root directly, as suggested in this issue. There are several critical places within the codebase where compression is performed, and where roots are computed. Relying on callers to do these steps correctly seems risky. This PR does not address this directly, but adds a warning to the get_merkle_root documentation.

How Has This Been Tested?

Manually checked (via looped testing) that the affected test succeeds.

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

Assert that CI passes. Run the test on a loop to assert that the test does not fail intermittently.

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 Mar 28, 2023
@AaronFeickert AaronFeickert marked this pull request as draft March 28, 2023 17:46
@AaronFeickert
Copy link
Collaborator Author

This PR is no longer needed, as the underlying problem is addressed by PR 5278.

@AaronFeickert AaronFeickert deleted the fix-mmr-test branch March 28, 2023 17:53
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 P-reviews_required Process - Requires a review from a lead maintainer to be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[CI] Investigate CI test failure
2 participants