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

v1.1: Include account.owner into account hash#9918

Merged
solana-grimes merged 3 commits intosolana-labs:v1.1from
ryoqun:hash-owner-v1.1
May 7, 2020
Merged

v1.1: Include account.owner into account hash#9918
solana-grimes merged 3 commits intosolana-labs:v1.1from
ryoqun:hash-owner-v1.1

Conversation

@ryoqun
Copy link
Copy Markdown
Contributor

@ryoqun ryoqun commented May 7, 2020

manual backport of #9917 with gating logic for the v1.x / tds

fixes #9916

Comment thread runtime/src/accounts_db.rs
@codecov
Copy link
Copy Markdown

codecov Bot commented May 7, 2020

Codecov Report

Merging #9918 into v1.1 will increase coverage by 0.0%.
The diff coverage is 78.2%.

@@          Coverage Diff          @@
##            v1.1   #9918   +/-   ##
=====================================
  Coverage   80.7%   80.7%           
=====================================
  Files        282     282           
  Lines      64420   64436   +16     
=====================================
+ Hits       51996   52027   +31     
+ Misses     12424   12409   -15     

@mvines mvines requested a review from sakridge May 7, 2020 18:05
@ryoqun
Copy link
Copy Markdown
Contributor Author

ryoqun commented May 7, 2020

@mvines Thanks for review!

I'm testing this locally now with tds snapshots...

@mvines mvines changed the title Include account.owner into account hash v1.1: Include account.owner into account hash May 7, 2020
@mvines
Copy link
Copy Markdown
Contributor

mvines commented May 7, 2020

@ryoqun - is your testing complete? Ok to merge?

@mvines mvines added the automerge Merge this Pull Request automatically once CI passes label May 7, 2020
@solana-grimes solana-grimes merged commit 9498f11 into solana-labs:v1.1 May 7, 2020
@ryoqun
Copy link
Copy Markdown
Contributor Author

ryoqun commented May 7, 2020

@mvines Sorry for being late; But I've successfully finished testing. I've done similar tests for tds with this.


pub fn include_owner_in_hash(slot: Slot) -> bool {
// Account hashing updated to include owner activates at this slot on the testnet
slot >= 14_000_000
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.

Well, my bad. This will not quite work for devnet............ devnet is still at around slot 5_356_748.

Gating logic is hard. Sorry about that. The excuse is that I only cared tds and mainnet-beta; that's already complicated enough to do flawless rolling update; not at all for devnet at the time of this pr....

This causes bankhash incompatibility between v1.1 branch and master branch for devnet: #10163 (comment)

FYI: @CriesofCarrots

mvines pushed a commit to mvines/solana that referenced this pull request May 25, 2020
solana-grimes pushed a commit that referenced this pull request May 25, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

automerge Merge this Pull Request automatically once CI passes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants