Do not derive Copy for EpochSchedule and Rent #32767

apfitzge · 2023-08-08T21:49:21Z

Problem

In #32547, I added a clippy allow lint for CloneZeroed types that were Copy to have an "incorrect" clone. For any Copy types, clippy considers a clone that is anything other than Copy to be incorrect.

EpochSchedule and Rent turned out to be the only Copy types using this derive macro.

Summary of Changes

EpochSchedule no longer derives Copy
Rent no longer derives Copy
Replace use of Copy by Clone (this will ensure padding bytes are zero consistently)
Use references where appropriate

Fixes #32588

codecov · 2023-08-08T22:50:57Z

Codecov Report

Merging #32767 (83843dc) into master (8a298f1) will decrease coverage by 0.1%.
Report is 1 commits behind head on master.
The diff coverage is 100.0%.

Additional details and impacted files

@@            Coverage Diff            @@
##           master   #32767     +/-   ##
=========================================
- Coverage    81.9%    81.9%   -0.1%     
=========================================
  Files         819      819             
  Lines      219129   219131      +2     
=========================================
- Hits       179532   179498     -34     
- Misses      39597    39633     +36

ledger/src/leader_schedule_cache.rs

brooksprumo · 2023-08-09T18:34:42Z

runtime/src/bank.rs

@@ -1470,7 +1470,7 @@ impl Bank {
                );
            } else {
                // Save a snapshot of stakes for use in consensus and stake weighted networking
-                let leader_schedule_epoch = epoch_schedule.get_leader_schedule_epoch(slot);
+                let leader_schedule_epoch = new.epoch_schedule().get_leader_schedule_epoch(slot);


brooksprumo

lgtm, thanks!

apfitzge · 2023-08-24T20:57:40Z

this got stale because there was a merge conflict I didn't resolve for a bit.

I think some of this may conflict with changes #32961 (or related PRs) has. So going to just hold this off until those get merged.

Lichtso · 2023-09-15T08:51:02Z

@apfitzge I think you can rebase

apfitzge · 2023-09-19T16:21:35Z

Some downstream issues w/ removing Copy from EpochSchedule in stake_pool. Will have to look at how that's being used

apfitzge · 2023-09-20T16:46:02Z

SPL fix PR: solana-labs/solana-program-library#5323

apfitzge · 2023-09-21T00:15:00Z

SPL fix PR: solana-labs/solana-program-library#5323

Hmm must have been another issue in SPL. I had run cargo test-sbf which I thought would have run all the tests. Will have to check again tomorrow...

apfitzge · 2023-09-25T20:08:16Z

Finally fixed all the SPL stuff 😦 @Lichtso, @brooksprumo

brooksprumo

Lgtm. Probably worthwhile to wait for approval from the other reviewers as well.

apfitzge · 2023-10-19T02:12:46Z

@Lichtso had to rebase this again, but should be in a good state again now.

apfitzge marked this pull request as ready for review August 9, 2023 17:25

apfitzge requested review from brooksprumo and Lichtso August 9, 2023 17:29

brooksprumo reviewed Aug 9, 2023

View reviewed changes

brooksprumo previously approved these changes Aug 9, 2023

View reviewed changes

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Aug 24, 2023

apfitzge dismissed brooksprumo’s stale review via f6e7054 August 24, 2023 20:56

apfitzge force-pushed the 32588 branch from 79ca5b6 to f6e7054 Compare August 24, 2023 20:56

github-actions bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Aug 25, 2023

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Sep 8, 2023

github-actions bot closed this Sep 15, 2023

Lichtso reopened this Sep 15, 2023

github-actions bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Sep 18, 2023

apfitzge force-pushed the 32588 branch from f6e7054 to 1edaee6 Compare September 18, 2023 20:56

apfitzge changed the title ~~Do not derive Copy for EpochSchedule~~ Do not derive Copy for EpochSchedule and Rent Sep 18, 2023

apfitzge mentioned this pull request Sep 20, 2023

Access epoch_schedule by reference solana-labs/solana-program-library#5323

Merged

apfitzge force-pushed the 32588 branch from 1edaee6 to e5c2c93 Compare September 20, 2023 19:57

apfitzge mentioned this pull request Sep 21, 2023

Remove EpochSchedule Copy solana-labs/solana-program-library#5329

Merged

apfitzge force-pushed the 32588 branch from e5c2c93 to 6bad80f Compare September 25, 2023 17:54

brooksprumo previously approved these changes Sep 25, 2023

View reviewed changes

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Oct 10, 2023

github-actions bot closed this Oct 18, 2023

apfitzge reopened this Oct 19, 2023

apfitzge dismissed brooksprumo’s stale review via e1a6228 October 19, 2023 01:12

apfitzge force-pushed the 32588 branch from 6bad80f to e1a6228 Compare October 19, 2023 01:12

apfitzge removed the stale [bot only] Added to stale content; results in auto-close after a week. label Oct 19, 2023

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Nov 2, 2023

apfitzge removed the stale [bot only] Added to stale content; results in auto-close after a week. label Nov 3, 2023

github-actions bot added the stale [bot only] Added to stale content; results in auto-close after a week. label Nov 17, 2023

apfitzge added 3 commits November 20, 2023 22:16

Do not derive Copy for EpochSchedule

9cb532a

remove clone in LeaderScheduleCache::new

2e939ce

do not derive Copy for Rent

83843dc

apfitzge force-pushed the 32588 branch from e1a6228 to 83843dc Compare November 20, 2023 22:23

github-actions bot removed the stale [bot only] Added to stale content; results in auto-close after a week. label Nov 21, 2023

Lichtso approved these changes Dec 1, 2023

View reviewed changes

apfitzge merged commit 2294801 into solana-labs:master Dec 1, 2023
42 checks passed

apfitzge deleted the 32588 branch December 1, 2023 15:57

willhickey mentioned this pull request Mar 28, 2024

v1.18 commits - please ignore anza-xyz/agave#475

Open

This was referenced Jun 14, 2024

v1.18: sdk: Rederive Copy on Rent and EpochSchedule anza-xyz/agave#1744

Merged

CHANGELOG: Copy no longer derived on Rent and EpochSchedule anza-xyz/agave#1746

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not derive Copy for EpochSchedule and Rent #32767

Do not derive Copy for EpochSchedule and Rent #32767

apfitzge commented Aug 8, 2023 •

edited

Loading

codecov bot commented Aug 8, 2023 •

edited

Loading

brooksprumo Aug 9, 2023

brooksprumo left a comment

apfitzge commented Aug 24, 2023

Lichtso commented Sep 15, 2023

apfitzge commented Sep 19, 2023

apfitzge commented Sep 20, 2023

apfitzge commented Sep 21, 2023

apfitzge commented Sep 25, 2023

brooksprumo left a comment

apfitzge commented Oct 19, 2023

Do not derive Copy for EpochSchedule and Rent #32767

Do not derive Copy for EpochSchedule and Rent #32767

Conversation

apfitzge commented Aug 8, 2023 • edited Loading

Problem

Summary of Changes

codecov bot commented Aug 8, 2023 • edited Loading

Codecov Report

brooksprumo Aug 9, 2023

Choose a reason for hiding this comment

brooksprumo left a comment

Choose a reason for hiding this comment

apfitzge commented Aug 24, 2023

Lichtso commented Sep 15, 2023

apfitzge commented Sep 19, 2023

apfitzge commented Sep 20, 2023

apfitzge commented Sep 21, 2023

apfitzge commented Sep 25, 2023

brooksprumo left a comment

Choose a reason for hiding this comment

apfitzge commented Oct 19, 2023

apfitzge commented Aug 8, 2023 •

edited

Loading

codecov bot commented Aug 8, 2023 •

edited

Loading