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

Convert snapshot#8380

Closed
carllin wants to merge 21 commits intosolana-labs:masterfrom
carllin:ConvertSnapshot
Closed

Convert snapshot#8380
carllin wants to merge 21 commits intosolana-labs:masterfrom
carllin:ConvertSnapshot

Conversation

@carllin
Copy link
Copy Markdown
Contributor

@carllin carllin commented Feb 21, 2020

Problem

No method exists to upgrade accounts in old snapshot to newer versions of those accounts, specifically vote accounts

Note: The only relevant changes are in ledger-tool/main.rs, all other changes are from: #8348

Summary of Changes

Hack together some throwaway code to try and achieve above.

Note: Nothing has been tested or even run yet 😛
Fixes #

@carllin carllin requested review from ryoqun and sakridge February 21, 2020 10:47
Comment thread ledger-tool/src/main.rs Outdated
});
if account.owner == solana_vote_program::id() {
VoteStateVersions::convert_from_raw(&mut account, pubkey);
root_bank.store_account_convert(*slot, pubkey, &account);
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@sakridge @ryoqun, my main question is will this dumb approach of reading out/converting/re-storing these accounts work, especially on accounts from older slots?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This might work; but I'm unsure what you want to achieve here. This way you can certainly could rewrite serialized vote accounts in snapshot. But this changes the binary (right?) then, account.hash and slot_hash and any future votes and PoH as well. This is a cascading effect. We can technically rewrite those, but we stop here: we can't rewrite third-party's signatures.

So, I think the ledger integrity is compromised. Could you explain why snapshot rewrite is needed nevertheless? :)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We want to keep the existing TdS ledger state, so all TdSers don't need to restart from genesis. The actual ledger history is ok to compromise on TdS right now 😬

Comment thread ledger-tool/src/main.rs Outdated
exit(1);
});

let storages: Vec<_> = root_bank.get_snapshot_storages();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

you're very trendy for incorporating (rebasing on) my latest pr: #8339 :p

}

pub fn convert_from_raw(account: &mut Account, pubkey: &Pubkey) {
let vote_state: VoteState0_23_5 = account
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reminder: depending on the use case of snapshot rewrite, we must be careful to update account.hash to what its definition should be.

pub struct CircBuf<I> {
pub buf: [I; MAX_ITEMS],
/// next pointer
pub idx: usize,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nits; when serializing explicit bitwidth is preferred? In this case, u64?

@@ -0,0 +1,60 @@
use super::*;

const MAX_ITEMS: usize = 32;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

related to this, if these are expected order of numbers, u8 is just enough?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@ryoqun this is copy-pasted from the old version of the vote state, trying to stay as close to the original as possible.

Comment thread programs/vote/src/vote_state/vote_state_versions.rs Outdated
@carllin carllin force-pushed the ConvertSnapshot branch 2 times, most recently from b8f79f2 to c0fe94c Compare February 22, 2020 08:40
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants