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

Ts fix cannonical #3343

Merged
merged 2 commits into from
Mar 22, 2023
Merged

Ts fix cannonical #3343

merged 2 commits into from
Mar 22, 2023

Conversation

JaredCorduan
Copy link
Contributor

Description

This fixes the bug discovered in IntersectMBO/cardano-node#4826. This issue merits a full retrospective, which we will provide. For now, however, we are applying the patch to the main ledger branch, adding property tests, and providing a terse explanation in these PR notes.

The Data.Map.Internal functions balanceR and balanceL expect the size difference between their two arguments to be 0 or 1, which allows them to re-balance the underlying tree in constant time. The canonical map can, however, shrink by one when it places the multi-assets in canonical form, resulting in the tree becoming unbalanced. Unlike balanceR and balanceL, link makes no assumptions about the relative balance, at the cost of spending log time re-balancing. The performance cost will not be noticeable.

The credit goes to @TimSheard for finding the problem so fast. Also many thanks to @lehins for his help in quickly narrowing things down.

replaces #3271

Checklist

  • Commit sequence broadly makes sense and commits have useful messages
  • New tests are added if needed and existing tests are updated
  • Any changes are noted in the CHANGELOG.md for affected package
  • Code is formatted with fourmolu (use scripts/fourmolize.sh)
  • Cabal files are formatted (use scripts/cabal-format.sh)
  • hie.yaml has been updated (use scripts/gen-hie.sh)
  • Self-reviewed the diff

Copy link
Collaborator

@lehins lehins left a comment

Choose a reason for hiding this comment

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

Looks great! Really happy to finally see this fix land in master 🙂

libs/cardano-data/cardano-data.cabal Outdated Show resolved Hide resolved
libs/cardano-data/CHANGELOG.md Outdated Show resolved Hide resolved
libs/cardano-data/CHANGELOG.md Show resolved Hide resolved
@JaredCorduan JaredCorduan force-pushed the ts-fix-cannonical branch 3 times, most recently from 94306de to 8ed7ca5 Compare March 20, 2023 20:12
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