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

Conversation

@sakridge
Copy link
Contributor

@sakridge sakridge commented Mar 2, 2020

Problem

Hack introduced to fix accounts code from cleaning up needed stores after they are recovered from a snapshot.

Summary of Changes

Restore all accounts versions in the append-vecs to prevent shielded accounts from being cleaned up.

Fixes #

@sakridge sakridge requested a review from ryoqun March 2, 2020 19:18
@sakridge sakridge force-pushed the remove-accounts-hack branch 2 times, most recently from d792dc3 to 2dac055 Compare March 2, 2020 19:45
@codecov
Copy link

codecov bot commented Mar 2, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@4f05f08). Click here to learn what that means.
The diff coverage is 98.7%.

@@           Coverage Diff            @@
##             master   #8569   +/-   ##
========================================
  Coverage          ?     80%           
========================================
  Files             ?     256           
  Lines             ?   55779           
  Branches          ?       0           
========================================
  Hits              ?   44660           
  Misses            ?   11119           
  Partials          ?       0

@sakridge sakridge force-pushed the remove-accounts-hack branch from 2dac055 to 33cc48a Compare March 2, 2020 22:59
@mvines mvines added the v1.0 label Mar 3, 2020
@mvines
Copy link
Contributor

mvines commented Mar 3, 2020

I still get a bank hash verification error when I:

  1. Boot a v1.0 node from an existing TdS snapshot with this PR
  2. Let the v1.0 node create a new snapshot
  3. Reboot the v1.0 node with the new snapshot

@sakridge sakridge force-pushed the remove-accounts-hack branch from 33cc48a to 4ab8ec1 Compare March 3, 2020 03:57
ryoqun
ryoqun previously approved these changes Mar 3, 2020
Copy link
Contributor

@ryoqun ryoqun left a comment

Choose a reason for hiding this comment

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

lgtm with nits

@sakridge sakridge force-pushed the remove-accounts-hack branch from 4ab8ec1 to 0afedaa Compare March 3, 2020 04:59
@mergify mergify bot dismissed ryoqun’s stale review March 3, 2020 05:00

Pull request has been modified.

@ryoqun
Copy link
Contributor

ryoqun commented Mar 3, 2020

@sakridge For #8436, I've flushed all previous AccountsDB-related patches to both v1.0 and v0.23. So, rebasing and back-porting this PR should be tedious and straightforward task.

However, considering #8593, I'm open to take this over for you to land far and wide. :)

@sakridge
Copy link
Contributor Author

sakridge commented Mar 3, 2020

@sakridge For #8436, I've flushed all previous AccountsDB-related patches to both v1.0 and v0.23. So, rebasing and back-porting this PR should be tedious and straightforward task.

However, considering #8593, I'm open to take this over for you to land far and wide. :)

Yea, if you can take this over, that would be great.

@sakridge sakridge force-pushed the remove-accounts-hack branch from 0afedaa to d25ab46 Compare March 3, 2020 21:49
@sakridge sakridge force-pushed the remove-accounts-hack branch from d25ab46 to 5170739 Compare March 3, 2020 22:41
@sakridge sakridge merged commit b084c1d into solana-labs:master Mar 4, 2020
@sakridge sakridge deleted the remove-accounts-hack branch March 4, 2020 04:49
mergify bot pushed a commit that referenced this pull request Mar 4, 2020
* Remove accounts hack and correctly restore append-vec counts

* Add test

(cherry picked from commit b084c1d)
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