Convert Banks by carllin · Pull Request #9033 · solana-labs/solana

carllin · 2020-03-23T23:24:04Z

Problem

Old bank structure is incompatible with changes to bank implemented here: #8958

Summary of Changes

Add a conversion to the new bank structure when deserializing the bank from 1.0 snapshots
Write out version 1.1 snapshots
Note this change includes changes from: Store and compute node/stake state in EpochStakes struct #8958

With 1 + 2, when validators all upgrade to 1.1, only 1.1 snapshots should be present in the system and we can throw away this code.

Note that with this change, any validators that don't upgrade to this change will be unable to process the new 1.1 snapshots.

Fixes #

carllin · 2020-03-23T23:27:07Z

+                    BankVersions::Bank1_0(deserialize_from_snapshot(stream.by_ref())?)
+                }
+                SNAPSHOT_VERSION_1_1 => {
+                    BankVersions::Bank1_1(deserialize_from_snapshot(stream.by_ref())?)


@mvines, @sakridge, I ran ledger-tool verify on an old 1.0 snapshot and this works fine.

At first I was like great, the change works!

But on second thought, isn't this a vulnerability? I fundamentally changed a field in the bank that's used by many parts of the system, and the snapshot hash still verified fine...

We don't require the Bank struct be immutable. There could exist another Solana implementation written in bash that produces the same snapshot hash as the current mostly-Rust implementation

Seems like critical fields in the bank that should be verified(please check if any are missing!)

parent_hash

hard_forks

transaction_count

tick_height

signature_count

capitalization

max_tick_height

hashes_per_tick

ticks_per_slot

ns_per_slot

genesis_creation_time

slots_per_year

slots_per_segment

slot

epoch

block_height

collector_id

collector_fees

fee_calculator

fee_rate_governor

collected_rent

rent_collector

epoch_schedule

inflation

stakes

storage_accounts

epoch_stakes

is_delta

message_processor

@mvines seems like I could just replace a stake account, or a epoch stakes account, or inflation/rent in the bank, and it would affect this node's view of consensus

That would cause a bank hash mismatch

Speed is not a reason to not do it, the whole serialized size of the bank is 200KB, that should take about 500us to hash with sha-2. We are already spending 100s of ms to hash the account states.

We can, and we'll probably need one of these painful snapshot/Bank adaption PRs every time we do so.

That, or you re-compute these values from the account state which is hashed. That seems like more work to me.

hehe I'm just saying that I'm pretty sure that all of the 29 Bank fields that Carl listed above do not all need be included in the bank hash.

That's part of #7167 and on @ryoqun 's radar. Part of why we are using the flags to get verified snapshots from only trusted nodes.

@carllin FYI: As @sakridge said, this is the wip-stale-closed PR for the exact thing of this discussion: #8185. I'm steadily approaching to it. There are still other bunch of security issues in the snapshot to tackle first, though. ;)

mvines · 2020-03-23T23:36:54Z

+
+pub enum BankVersions {
+    Bank1_0(Bank1_0),
+    Bank1_1(Bank),


Bank1_1 we can just call Current, and it only gets a version when it's an older bank that we care about

mvines · 2020-03-23T23:43:28Z


-pub const SNAPSHOT_VERSION: &str = "1.0.0";
+pub const SNAPSHOT_VERSION_1_0: &str = "1.0.0";
+pub const SNAPSHOT_VERSION_1_1: &str = "1.1.0";


This can just be SNAPSHOT_VERSION ("Current" is the default, only name the legacy versions)

codecov · 2020-03-24T00:34:04Z

Codecov Report

Merging #9033 into v1.0 will decrease coverage by <.1%.
The diff coverage is 54.6%.

@@           Coverage Diff           @@
##            v1.0   #9033     +/-   ##
=======================================
- Coverage   80.4%   80.3%   -0.1%     
=======================================
  Files        263     265      +2     
  Lines      57363   57553    +190     
=======================================
+ Hits       46126   46228    +102     
- Misses     11237   11325     +88

mvines

The runtime/src/bank.rs → runtime/src/bank/mod.rs rename seems unnecessary but I'm not strongly opposed.

Let's wait until 1.0.9 ships before landing this in v1.0 though please. I didn't review runtime/src/epoch_stakes.rs, I assume that's been covered in the other PR for master

mvines · 2020-03-24T23:13:02Z

@carllin - please ping me when this PR and #8958 are green and I'll review again, thanks!

carllin · 2020-03-25T02:25:38Z

@mvines Green!

mvines

Thanks, this looks good! I'm 👎 this PR until we ship v1.0.10 this evening. I'd like to get this in v1.0.11 instead

mvines

1.0.10 has shipped, all clear

carllin requested review from mvines and sakridge March 23, 2020 23:24

carllin commented Mar 23, 2020

View reviewed changes

mvines reviewed Mar 23, 2020

View reviewed changes

Comment thread ledger/src/snapshot_utils.rs Outdated

mvines reviewed Mar 23, 2020

View reviewed changes

carllin force-pushed the VersionBanks branch from a9c3bf2 to da0b920 Compare March 23, 2020 23:44

mvines reviewed Mar 23, 2020

View reviewed changes

Comment thread runtime/src/bank.rs Outdated

carllin force-pushed the VersionBanks branch 2 times, most recently from 3fb7776 to 0bbc048 Compare March 24, 2020 01:59

mvines reviewed Mar 24, 2020

View reviewed changes

Comment thread ledger/src/snapshot_utils.rs

Comment thread ledger/src/snapshot_utils.rs Outdated

carllin force-pushed the VersionBanks branch from 0ec395d to 3e2d85c Compare March 24, 2020 22:06

carllin force-pushed the VersionBanks branch 3 times, most recently from 3900a53 to 225a6d1 Compare March 25, 2020 00:18

carllin added 11 commits March 24, 2020 21:11

Store and compute needed state in EpochStakes struct

7391368

Add test

e8f5206

Add Bank_1_0

6bac8a2

Plumb Bank_1_0, add conversion

a2978b9

Add missing file

bb26b5c

Correct version

4226133

Make conversion

5588cdc

Versioning name changes

c87b112

Remove BankVersions

ebe8208

Add total stake to node id mapping

e2c7367

Return Arcs

ab4f33f

carllin force-pushed the VersionBanks branch from 2d2dba2 to ab4f33f Compare March 25, 2020 04:12

mvines suggested changes Mar 25, 2020

View reviewed changes

mvines approved these changes Mar 26, 2020

View reviewed changes

carllin merged commit d4ddb62 into solana-labs:v1.0 Mar 26, 2020

Conversation

carllin commented Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carllin Mar 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sakridge Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

codecov Bot commented Mar 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mvines left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

mvines commented Mar 24, 2020

Uh oh!

carllin commented Mar 25, 2020

Uh oh!

mvines left a comment

Choose a reason for hiding this comment

Uh oh!

mvines left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

carllin commented Mar 23, 2020 •

edited

Loading

carllin Mar 23, 2020 •

edited

Loading

sakridge Mar 24, 2020 •

edited

Loading

codecov Bot commented Mar 24, 2020 •

edited

Loading