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

Adds accounts lt hash featurization #3334

Merged
merged 2 commits into from
Nov 13, 2024

Conversation

brooksprumo
Copy link

Problem

The accounts lt hash must be enabled cluster-wide, so it requires a feature gate. There currently isn't one.

Summary of Changes

  • Add the accounts lt hash feature
  • Mix in the accounts lt hash into the bank
  • Remove the Epoch Accounts Hash

Feature Gate Issue: #3333

@brooksprumo brooksprumo added the feature-gate Pull Request adds or modifies a runtime feature gate label Oct 28, 2024
@brooksprumo brooksprumo self-assigned this Oct 28, 2024
@brooksprumo brooksprumo force-pushed the lthash/feature-gate branch 2 times, most recently from 847a7fe to 4c27110 Compare October 28, 2024 17:06
@brooksprumo brooksprumo marked this pull request as ready for review October 28, 2024 18:14
Copy link

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

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

what's the plan for feature enable lt hash?
replace EAH?

@brooksprumo
Copy link
Author

what's the plan for feature enable lt hash? replace EAH?

Yep! This feature gate does two things:

  1. mixes the accounts lt hash into the bank hash
  2. removes the EAH

@HaoranYi
Copy link

Can we add this #3334 (comment) to the feature code comments?

@brooksprumo
Copy link
Author

brooksprumo commented Oct 28, 2024

Can we add this #3334 (comment) to the feature code comments?

Yes! Thank you, I forgot about that.
(And once there's a SIMD, I'll change it to that number)

jeffwashington
jeffwashington previously approved these changes Oct 28, 2024
Copy link

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

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

lgtm

HaoranYi
HaoranYi previously approved these changes Oct 28, 2024
Copy link

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

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

lgtm

@brooksprumo brooksprumo marked this pull request as draft October 29, 2024 00:03
@brooksprumo brooksprumo dismissed stale reviews from HaoranYi and jeffwashington October 29, 2024 00:04

Code at time of review is missing features

@brooksprumo
Copy link
Author

I've dismissed the current reviews and moved this PR back to a draft. I need to implement calculating the accounts lt hash at feature activation time too, which was missing. After implementation and testing, I'll re-request reviews. Thanks for bearing with me here.

@brooksprumo brooksprumo marked this pull request as ready for review November 13, 2024 05:32
@HaoranYi
Copy link

HaoranYi commented Nov 13, 2024

I made a first pass review of this PR.
Overall, looks good.
I commented with a few questions.

@brooksprumo brooksprumo requested a review from HaoranYi November 13, 2024 16:33
Copy link

@HaoranYi HaoranYi left a comment

Choose a reason for hiding this comment

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

lgtm

Copy link

@jeffwashington jeffwashington left a comment

Choose a reason for hiding this comment

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

lgtm

@brooksprumo brooksprumo merged commit eef9492 into anza-xyz:master Nov 13, 2024
50 checks passed
@brooksprumo brooksprumo deleted the lthash/feature-gate branch November 13, 2024 17:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-gate Pull Request adds or modifies a runtime feature gate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants