Include rent_epoch and executable into account hash #7415

ryoqun · 2019-12-11T00:15:50Z

Problem

rent_epoch and executable isn't included into the account hash calculation, thereby not into the bank hash too. This means those fields are falsifiable by malicious validator.

From git history, it seems that the two weren't included just due to oversight.

Summary of Changes

Include the two into the account hash calculation. Also, add rather a peculiar test for any future account's modification to be paired with account hash calculation update...

Part of #7167

ryoqun · 2019-12-11T00:18:22Z

runtime/src/accounts_db.rs

+            *byte = (INPUT_LEN - i) as u8;
+        }
+
+        //UNSAFE: forcibly cast the special byte pattern to actual account fields.


My first UNSAFE here. :) Is there any better way to archive similar test result?

Neat idea. An unsafe in a test is less concerning to me

An unsafe in a test is less concerning to me

Thx! This will be a precedent for the follow-up PRs, in which we need more of these crafted binaries to protect validator from crashing from external data. :)

ryoqun · 2019-12-11T00:21:48Z

runtime/src/accounts_db.rs

-        Self::hash_account_data(slot, account.lamports, &account.data, pubkey)
+        Self::hash_account_data(
+            slot,
+            account.lamports,


lamports and rent_epoch are both u64. So interchanging them caused no compile error. Spent some time to figure out why hashes are different. ;) So I added another assertion covering that kind of bug.

ryoqun · 2019-12-11T00:23:29Z

runtime/src/accounts_db.rs

        hasher.hash(&data);

+        if executable {


Note to myself: when sanitizing executable in follow up PR, check the upper 7-bit are cleared.

Reply to myself: yeah, I've done this correctly at #7464

ryoqun · 2019-12-11T00:28:04Z

runtime/src/accounts_db.rs

+        };
+        let account = stored_account.clone_account();
+        let expected_account_hash =
+            Hash::from_str("GGTsxvxwnMsNfN6yYbBVQaRgvbVLfxeWnGXNyB8iXDyE").unwrap();


According to this and that, this will be stable really well justifying the magic hash.

Out of my pure laziness, I've skipped to calculate this test vector by hand. :p

ryoqun · 2019-12-11T00:34:08Z

runtime/src/accounts_db.rs

+            StoredMeta,
+            AccountMeta,
+            [u8; ACCOUNT_DATA_LEN],
+            usize, // for StoredAccount::offset


I wanted to write this like typeof(StoredAccount::offset) but it seems I can't...

codecov · 2019-12-11T00:45:59Z

Codecov Report

Merging #7415 into master will decrease coverage by 2.7%.
The diff coverage is 76.1%.

@@           Coverage Diff            @@
##           master   #7415     +/-   ##
========================================
- Coverage    70.7%   67.9%   -2.8%     
========================================
  Files         245     244      -1     
  Lines       55227   56868   +1641     
========================================
- Hits        39054   38640    -414     
- Misses      16173   18228   +2055

ryoqun · 2019-12-11T00:46:34Z

@danpaul000 Really FYI, this breaks an ABI, #6123. Can ignore this mention because I'm pretty sure there are plenty of other PRs in the master with imcompatible ABI.

ryoqun · 2019-12-11T00:50:21Z

CI passed, proving there were no existing unit test for the code changed in this PR because I didn't need to touch any existing tests... ;)

mvines · 2019-12-11T03:40:04Z

@danpaul000 Really FYI, this breaks an ABI, #6123. Can ignore this mention because I'm pretty sure there are plenty of other PRs in the master with imcompatible ABI.

Breaking ABI between 0.21 and 0.22 is ok, that's gonna happen elsewhere too. But after 0.22 ships we're going to try to be much more careful about this

mvines · 2019-12-11T03:51:20Z

runtime/src/append_vec.rs

@@ -31,6 +33,8 @@ pub struct StoredMeta {
    pub data_len: u64,
 }

+/// This struct will be backed by mmaped and snapshotted data files.
+/// So the data layout must be stable and consistent across the entire cluster!


Rather than comments, I wonder if we should define an attribute to start clearly structs that are part of the Solana ABI, like #[solana_abi]. One day this'll then allow us to programmatically find all these structs and compare their values between release to flag ABI changes.

This is totally out of scope of this PR but if that sounds good, @ryoqun do you want to think about this more and make a design proposal for how we might start to track ABI changes

Heh, let's talk about ABI compat! That task looks interesting to me! I'll give it a try!

This reminds of my rookie days when I found an ABI-related bug: #6388 Always, these are pesky bugs.. Hopefully, I can find good solution!

mvines · 2019-12-11T03:52:26Z

runtime/src/accounts_db.rs

@@ -191,6 +191,8 @@ pub struct AccountStorageEntry {
    ///  any accounts in it
    /// status corresponding to the storage, lets us know that
    ///  the append_vec, once maxed out, then emptied, can be reclaimed
+    /// Currently serialized count isn't used at all. Rather, we recalculate it


I don't quite understand this comment

Oops, I've forgot to comment about this. You're right. No wonder to be confused.

First, this comment isn't related to this PR at all.
I added this comment to properly document the current implementation reality, which have changed over the course of prior snapshot stability PRs. I'm sure @sakridge can understand this comment, who shares the needed context.

Specifically this comment is referring to this change

@sakridge what do you think fixing this before strict ABI audit thing will be under the effect? Or, at the least, we should create an issue. After all, it seems we can reconstruct this information when ingesting snapshots without problems.

Yea, we can create an issue and we can move this comment out of this PR.

Created! #7442

mvines · 2019-12-11T03:54:21Z

runtime/src/accounts_db.rs

+            *byte = (INPUT_LEN - i) as u8;
+        }
+
+        //UNSAFE: forcibly cast the special byte pattern to actual account fields.


Neat idea. An unsafe in a test is less concerning to me

mvines · 2019-12-11T03:55:46Z

runtime/src/accounts_db.rs

    }

-    pub fn hash_account_data(slot: Slot, lamports: u64, data: &[u8], pubkey: &Pubkey) -> Hash {
+    pub fn hash_account_data(


What do you think about passing the entire account: &Account into this function rather than the 4 account fields?

Thought about the same, too! But there is a reason for the status quo: This function should be accessible from the other code path (no Accounts there): https://github.com/solana-labs/solana/pull/7415/files#diff-2099c5256db4eb5975c8834af38f6456R810

Of course, we can always normalize to Account by stored_account.clone_account(), but performance might be affected or rustc is clever enough?

Oh ok. I think the current code is fine then, probably not worth getting fancy yet. Maybe when we add the 5th or 6th account field later :)

ryoqun · 2019-12-11T08:39:36Z

@sakridge @rob-solana

From this old comment:

@sakridge once this is in place, we'll have to decide whether you hash() accounts before or after I deduct rent...

Is there any remaining concern about this comment from the old PR, still applicable to the current situation?

I naively thought that rent_epoch can be treated equally like other fields. So, considering that comment's concern, we're hash()-ing after we deduct rent and when storing the updated account into the AccountsDB. Is there any problem for this?

ryoqun · 2019-12-11T08:49:47Z

runtime/src/accounts_db.rs

+        rent_epoch: Epoch,
+        data: &[u8],
+        pubkey: &Pubkey,
+    ) -> Hash {
        if lamports == 0 {


Well, this hash algorithm seems littered with many modifications... #6315 #5573 #3801

rob-solana · 2019-12-11T20:49:20Z

@sakridge @rob-solana

From this old comment:

@sakridge once this is in place, we'll have to decide whether you hash() accounts before or after I deduct rent...

Is there any remaining concern about this comment from the old PR, still applicable to the current situation?

I naively thought that rent_epoch can be treated equally like other fields. So, considering that comment's concern, we're hash()-ing after we deduct rent and when storing the updated account into the AccountsDB. Is there any problem for this?

hash should be the last thing, after all account updates, including deduction and distribution of rent

rob-solana

nice work

sakridge

lgtm

Pull request has been modified.

ryoqun · 2019-12-12T04:02:09Z

Merging this because I got approvals (They got removed by this tiny commit, though)

ryoqun · 2023-08-31T08:39:23Z

runtime/src/accounts_db.rs

@@ -2103,4 +2130,56 @@ pub mod tests {

        Ok(())
    }
+
+    #[test]
+    fn test_hash_stored_account() {


ryoqun added 2 commits December 10, 2019 15:40

Clean a bit

65681ef

Add rent_epoch and executable into account hashing

ff504c3

ryoqun requested review from sakridge and mvines December 11, 2019 00:15

ryoqun commented Dec 11, 2019

View reviewed changes

ryoqun changed the title ~~Hash epoch rent executable~~ Include rent_epoch and executable into account hash Dec 11, 2019

mvines reviewed Dec 11, 2019

View reviewed changes

ryoqun mentioned this pull request Dec 11, 2019

Design Solana ABI management process #7421

Closed

ryoqun commented Dec 11, 2019

View reviewed changes

rob-solana previously approved these changes Dec 11, 2019

View reviewed changes

sakridge previously approved these changes Dec 11, 2019

View reviewed changes

Remove comment and instead create an issue

ae6e54e

ryoqun merged commit a1ab81a into solana-labs:master Dec 12, 2019

ryoqun mentioned this pull request Dec 17, 2019

Propose Solana ABI management #7524

Merged

ryoqun mentioned this pull request Feb 3, 2020

Add snapshot account state verify proposal #8043

Closed

jstarry mentioned this pull request May 7, 2020

Account owner is not included in account hash #9916

Closed

ryoqun commented Aug 31, 2023

View reviewed changes

runtime/src/accounts_db.rs

@@ -2103,4 +2130,56 @@ pub mod tests {

Ok(())

}

#[test]

fn test_hash_stored_account() {

Copy link

Member Author

ryoqun Aug 31, 2023

Choose a reason for hiding this comment

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

here

ryoqun mentioned this pull request Aug 31, 2023

Add #[repr(C)] for more future-proof byte mangling #33085

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Include rent_epoch and executable into account hash #7415

Include rent_epoch and executable into account hash #7415

ryoqun commented Dec 11, 2019

ryoqun Dec 11, 2019

mvines Dec 11, 2019

ryoqun Dec 11, 2019

ryoqun Dec 11, 2019

ryoqun Dec 11, 2019 •

edited

ryoqun Dec 13, 2019

ryoqun Dec 11, 2019 •

edited

ryoqun Dec 11, 2019 •

edited

ryoqun May 8, 2020

ryoqun Dec 11, 2019

codecov bot commented Dec 11, 2019 •

edited

ryoqun commented Dec 11, 2019 •

edited

ryoqun commented Dec 11, 2019

mvines commented Dec 11, 2019

mvines Dec 11, 2019

ryoqun Dec 11, 2019

mvines Dec 11, 2019

ryoqun Dec 11, 2019

sakridge Dec 11, 2019

ryoqun Dec 12, 2019

mvines Dec 11, 2019

mvines Dec 11, 2019

ryoqun Dec 11, 2019

mvines Dec 11, 2019

ryoqun commented Dec 11, 2019

ryoqun Dec 11, 2019 •

edited

rob-solana commented Dec 11, 2019

rob-solana left a comment

sakridge left a comment

ryoqun commented Dec 12, 2019 •

edited

ryoqun Aug 31, 2023

Include rent_epoch and executable into account hash #7415

Include rent_epoch and executable into account hash #7415

Conversation

ryoqun commented Dec 11, 2019

Problem

Summary of Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoqun Dec 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoqun Dec 11, 2019 • edited

Choose a reason for hiding this comment

ryoqun Dec 11, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Dec 11, 2019 • edited

Codecov Report

ryoqun commented Dec 11, 2019 • edited

ryoqun commented Dec 11, 2019

mvines commented Dec 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryoqun commented Dec 11, 2019

ryoqun Dec 11, 2019 • edited

Choose a reason for hiding this comment

rob-solana commented Dec 11, 2019

rob-solana left a comment

Choose a reason for hiding this comment

sakridge left a comment

Choose a reason for hiding this comment

ryoqun commented Dec 12, 2019 • edited

Choose a reason for hiding this comment

ryoqun Dec 11, 2019 •

edited

ryoqun Dec 11, 2019 •

edited

ryoqun Dec 11, 2019 •

edited

codecov bot commented Dec 11, 2019 •

edited

ryoqun commented Dec 11, 2019 •

edited

ryoqun Dec 11, 2019 •

edited

ryoqun commented Dec 12, 2019 •

edited