Skip to content
This repository was archived by the owner on Jan 22, 2025. It is now read-only.

Start saving/loading prior_roots(_with_hash) to snapshot#23844

Merged
jeffwashington merged 4 commits intosolana-labs:masterfrom
jeffwashington:mar62
Mar 24, 2022
Merged

Start saving/loading prior_roots(_with_hash) to snapshot#23844
jeffwashington merged 4 commits intosolana-labs:masterfrom
jeffwashington:mar62

Conversation

@jeffwashington
Copy link
Copy Markdown
Contributor

@jeffwashington jeffwashington commented Mar 22, 2022 •
edited
Loading

Problem

previous work: #23331
The idea is this is a non-breaking change to the snapshot file format. New fields are added to the end on save. The fields are loaded as empty on default by everyone atm.
https://discord.com/channels/428295358100013066/950515661862498354/955897214683734048

Summary of Changes

Fixes #

@jeffwashington jeffwashington force-pushed the mar62 branch 5 times, most recently from fdd065b to 6f812a3 Compare March 22, 2022 21:30
@jeffwashington
Copy link
Copy Markdown
Contributor Author

jeffwashington commented Mar 23, 2022

This should handle what @carllin needs. What @brooksprumo needs is already merged in master. This handles what I need for eliminating rewrites.

@brooksprumo
Copy link
Copy Markdown
Contributor

brooksprumo commented Mar 23, 2022

@AshwinSekar Tagging you to make sure you're in the loop. I think you've already seen this PR, but here it is, if not!

@jeffwashington jeffwashington marked this pull request as ready for review March 24, 2022 02:05
brooksprumo
brooksprumo previously approved these changes Mar 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@brooksprumo brooksprumo 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 to me. I'd feel best to make sure @mvines takes a look too.

Comment thread runtime/src/serde_snapshot.rs
Comment thread runtime/src/serde_snapshot.rs Outdated
Comment on lines +436 to +446
/*
todo in future pr:
deserialize prior_roots and prior_roots_with_hash
example:
{
let mut writer = accounts_db.accounts_index.roots_tracker.write().unwrap();
for x in snapshot_prior_roots {
writer.roots_original.insert(x);
}
}
*/
Copy link
Copy Markdown
Contributor

@brooksprumo brooksprumo Mar 24, 2022

Choose a reason for hiding this comment

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

nit: Maybe this is just me, but I'd prefer if this were removed. Not a strong conviction though.

Copy link
Copy Markdown
Contributor

@mvines mvines Mar 24, 2022

Choose a reason for hiding this comment

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

I'm a fan of removing as well. Issues can be used to track the details of future PRs better than littering in the code

mvines
mvines previously approved these changes Mar 24, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@mvines mvines left a comment

Choose a reason for hiding this comment

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

lgtm, all I have is nits

Comment thread runtime/src/accounts_index.rs Outdated
Comment thread runtime/src/accounts_index.rs Outdated
Comment thread runtime/src/serde_snapshot.rs Outdated
Comment on lines +436 to +446
/*
todo in future pr:
deserialize prior_roots and prior_roots_with_hash
example:
{
let mut writer = accounts_db.accounts_index.roots_tracker.write().unwrap();
for x in snapshot_prior_roots {
writer.roots_original.insert(x);
}
}
*/
Copy link
Copy Markdown
Contributor

@mvines mvines Mar 24, 2022

Choose a reason for hiding this comment

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

I'm a fan of removing as well. Issues can be used to track the details of future PRs better than littering in the code

jeffwashington and others added 2 commits March 24, 2022 09:52
@jeffwashington @mvines
Update runtime/src/accounts_index.rs
adc0780 
Co-authored-by: Michael Vines <mvines@gmail.com>
@jeffwashington @mvines
Update runtime/src/accounts_index.rs
583c1cc 
Co-authored-by: Michael Vines <mvines@gmail.com>
@mergify mergify Bot dismissed stale reviews from brooksprumo and mvines March 24, 2022 14:56

Pull request has been modified.

@jeffwashington jeffwashington merged commit 396b49a into solana-labs:master Mar 24, 2022
mergify Bot pushed a commit that referenced this pull request Apr 2, 2022
@jeffwashington @mvines
Start saving/loading prior_roots(_with_hash) to snapshot (#23844)
c5c2c05 
* Start saving/loading prior_roots(_with_hash) to snapshot

* Update runtime/src/accounts_index.rs

Co-authored-by: Michael Vines <mvines@gmail.com>

* Update runtime/src/accounts_index.rs

Co-authored-by: Michael Vines <mvines@gmail.com>

* update comment

Co-authored-by: Michael Vines <mvines@gmail.com>
(cherry picked from commit 396b49a)
@mergify mergify Bot mentioned this pull request Apr 2, 2022
Merged
jeffwashington added a commit that referenced this pull request Apr 2, 2022
@jeffwashington
Note this is a modified backport that does not SAVE the new fields, b…
a67496f 
…ut does load them.

Original:
Start saving/loading prior_roots(_with_hash) to snapshot (#23844)

    * Start saving/loading prior_roots(_with_hash) to snapshot

    * Update runtime/src/accounts_index.rs

    Co-authored-by: Michael Vines <mvines@gmail.com>

    * Update runtime/src/accounts_index.rs

    Co-authored-by: Michael Vines <mvines@gmail.com>

    * update comment

    Co-authored-by: Michael Vines <mvines@gmail.com>
    (cherry picked from commit 396b49a)
jeffwashington added a commit that referenced this pull request Apr 2, 2022
@jeffwashington
Note this is a modified backport that does not SAVE the new fields, b…
32a01ef 
…ut does load them.

Original:
Start saving/loading prior_roots(_with_hash) to snapshot (#23844)

    * Start saving/loading prior_roots(_with_hash) to snapshot

    * Update runtime/src/accounts_index.rs

    Co-authored-by: Michael Vines <mvines@gmail.com>

    * Update runtime/src/accounts_index.rs

    Co-authored-by: Michael Vines <mvines@gmail.com>

    * update comment

    Co-authored-by: Michael Vines <mvines@gmail.com>
    (cherry picked from commit 396b49a)
mergify Bot added a commit that referenced this pull request Apr 2, 2022
@mergify @jeffwashington
Note this is a modified backport that does not SAVE the new fields, b…
b157a91 
…ut does load them. (#24074)

Original:
Start saving/loading prior_roots(_with_hash) to snapshot (#23844)

    * Start saving/loading prior_roots(_with_hash) to snapshot

    * Update runtime/src/accounts_index.rs

    Co-authored-by: Michael Vines <mvines@gmail.com>

    * Update runtime/src/accounts_index.rs

    Co-authored-by: Michael Vines <mvines@gmail.com>

    * update comment

    Co-authored-by: Michael Vines <mvines@gmail.com>
    (cherry picked from commit 396b49a)

Co-authored-by: Jeff Washington (jwash) <wash678@gmail.com>
mergify Bot added a commit that referenced this pull request Apr 2, 2022
@mergify @jeffwashington
Note this is a modified backport that does not SAVE the new fields, b…
50d1eae 
…ut does load them. (#24074)

Original:
Start saving/loading prior_roots(_with_hash) to snapshot (#23844)

    * Start saving/loading prior_roots(_with_hash) to snapshot

    * Update runtime/src/accounts_index.rs

    Co-authored-by: Michael Vines <mvines@gmail.com>

    * Update runtime/src/accounts_index.rs

    Co-authored-by: Michael Vines <mvines@gmail.com>

    * update comment

    Co-authored-by: Michael Vines <mvines@gmail.com>
    (cherry picked from commit 396b49a)

Co-authored-by: Jeff Washington (jwash) <wash678@gmail.com>
(cherry picked from commit b157a91)
jeffwashington pushed a commit that referenced this pull request Apr 8, 2022
@mergify
Note this is a modified backport that does not SAVE the new fields, b…
1489cbf 
…ut does load them. (#24074) (#24078)

Original:
Start saving/loading prior_roots(_with_hash) to snapshot (#23844)

    * Start saving/loading prior_roots(_with_hash) to snapshot

    * Update runtime/src/accounts_index.rs

    Co-authored-by: Michael Vines <mvines@gmail.com>

    * Update runtime/src/accounts_index.rs

    Co-authored-by: Michael Vines <mvines@gmail.com>

    * update comment

    Co-authored-by: Michael Vines <mvines@gmail.com>
    (cherry picked from commit 396b49a)

Co-authored-by: Jeff Washington (jwash) <wash678@gmail.com>
(cherry picked from commit b157a91)

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@steviez steviez mentioned this pull request Jul 6, 2022
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@yhchiang-sol yhchiang-sol Awaiting requested review from yhchiang-sol

@carllin carllin Awaiting requested review from carllin

2 more reviewers

@brooksprumo brooksprumo brooksprumo left review comments

@mvines mvines mvines left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jeffwashington @brooksprumo @mvines