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

sdk: evict hard_forks #3962

Merged
merged 7 commits into from
Dec 11, 2024
Merged

Conversation

buffalojoec
Copy link

Problem

Moving toward evicting rent_collector from the SDK, hard_forks stands in the way.
#3932

Summary of Changes

Extract it into solana-hard-forks standalone crate.

@buffalojoec
Copy link
Author

Note CI is broken on master.

Copy link

@joncinque joncinque 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 overall! Just tiny things. I hesitated about whether we should use the Slot type from solana_clock for better clarity.

It's such a tiny type that it seems silly to add the dependency for a type alias, and the API is clear enough that they're slots, so I lean towards your approach.

sdk/src/lib.rs Outdated Show resolved Hide resolved
sdk/Cargo.toml Show resolved Hide resolved
@buffalojoec
Copy link
Author

I hesitated about whether we should use the Slot type from solana_clock for better clarity.

It's such a tiny type that it seems silly to add the dependency for a type alias, and the API is clear enough that they're slots, so I lean towards your approach.

Yeah I feel like the dependency for just a type alias is silly, too. I noticed we've moved to u64 in other crates that got split out, so I think that's fine to do here as well.

@buffalojoec buffalojoec force-pushed the sdk-evict-hard-forks branch 2 times, most recently from 468c58b to 8958213 Compare December 6, 2024 09:02
@kevinheavey
Copy link

Will this end up being used anywhere that the solana-genesis-config crate is not used? If not we could just put them in the same crate #3860

No strong feelings though, I consider crates cheap

@buffalojoec
Copy link
Author

Will this end up being used anywhere that the solana-genesis-config crate is not used? If not we could just put them in the same crate #3860

No strong feelings though, I consider crates cheap

There are a few places it's used without genesis_config, but putting them in the same crate wouldn't break those callsites (esp. if we do the proper re-exporting in SDK). I just felt like they made sense separate from each other, given they aren't too tightly coupled concepts.

Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Just the last bit to fix, then we can get this in. @yihau can you accept solana-hard-forks?

sdk/hard-forks/Cargo.toml Outdated Show resolved Hide resolved
@joncinque
Copy link

According to the failing test, looks like a frozen-abi digest needs to be updated:

thread 'bank::serde_snapshot::tests::test_bank_serialize::BankAbiTestWrapper_frozen_abi::test_abi_digest' panicked at runtime/src/bank/serde_snapshot.rs:573:13:
assertion `left == right` failed: Possibly ABI changed? Confirm the diff by rerunning before and after this test failed with SOLANA_ABI_DUMP_DIR!
  left: "2bWtYJSWVVvCEnBw6W2PsYZaR7RVs2V7CthFcHArdbUR"
 right: "9NABbrKjv1mmcPAmvtav1tVq4cJ7tTwKpHbYj6RDp3hi"
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
test bank::serde_snapshot::tests::test_bank_serialize::BankAbiTestWrapper_frozen_abi::test_abi_digest ... FAILED

@buffalojoec
Copy link
Author

According to the failing test, looks like a frozen-abi digest needs to be updated:

Sorry, I didn't see this was failing after I committed your suggestion. Will fix.

Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Sorry, one last thing to gate this properly on the full feature as it was before

sdk/src/lib.rs Show resolved Hide resolved
Copy link

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

@buffalojoec buffalojoec merged commit 15525f2 into anza-xyz:master Dec 11, 2024
52 checks passed
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