Verify number of hashes for each block of entries #6262

jstarry · 2019-10-07T22:17:47Z

Problem

We don't currently verify the number of ticks per block and the number of hashes per tick

Summary of Changes

Update create_ticks to accept number of hashes per tick
Verify tick count and hash count when processing entry slices
Verify that all blocks end in a tick entry
Remove BlobError and replace it with BlockError
Add granularity to BlocktreeProcessorError

Fixes #4282

codecov · 2019-10-08T03:35:55Z

Codecov Report

Merging #6262 into master will increase coverage by 6.3%.
The diff coverage is 94.7%.

@@           Coverage Diff            @@
##           master   #6262     +/-   ##
========================================
+ Coverage    72.2%   78.5%   +6.3%     
========================================
  Files         212     213      +1     
  Lines       44405   41129   -3276     
========================================
+ Hits        32076   32308    +232     
+ Misses      12329    8821   -3508

mvines

It looks like we're only verifying the hash count when blocktree processor first loads the ledger from disk. We also need to verify the hash count in replay stage as each new block is ingested

mvines · 2019-10-18T15:14:23Z

core/src/entry.rs

@@ -299,6 +287,20 @@ impl EntrySlice for [Entry] {
        );
        res
    }
+
+    fn validate_hash_separation(&self, hashes_per_tick: u64) -> bool {


I'm not a fan of the name of this function: validate_hash_separation()

validate_hash_count()?

Sure, do you think that it would be surprising that validate_hash_count rejects an entry slice that doesn't end in a tick entry? I want to enforce that but maybe it would be better do in another place

I don't think we want to do that. There's no guarantee an entry slice always ends with a tick

Ok, what about slots? Should we enforce that they end in ticks?

I don't think we need that as the number of ticks in a slot is something that's computed dynamically by the bank on each node. The incoming ledger data itself has no "slot marker".
eg,

solana/runtime/src/bank.rs

Lines 807 to 810 in 5468be2

} else if current_tick_height % self.ticks_per_slot == 0 {

// Register a new block hash if at the last tick in the slot

Some(self.blockhash_queue.write().unwrap())

} else {

@aeyakovenko, @rob-solana, maybe marking this type of failure as a dead slot isn't the correct approach? Summary of concerns here: #6498

@carllin so from your last comment here: #6498 (comment) is the conclusion that this PR is fine with the last entry check? Between #6498 and #6512 I think we're covered

@jstarry So from my understanding, in this PR, the situation that will trigger BlockError::TrailingEntry in both replay_stage and blocktree_processor:

Observed max_tick_height, and the last entry was not a tick (extra trailing entry after the last tick)

This is a slightly larger set of conditions under which BlockError::TrailingEntry should be triggered than what was proposed in #6512, which was:

Observed max_tick_height, and the last tick shred was not marked with LAST_SHRED_IN_SLOT.

Essentially the difference between the proposal and what's in this PR is, if the If the last tick shred was marked with LAST_SHRED_IN_SLOT, this slot shouldn't be marked as dead, even if there are trailing entries (because there might be a subset of this slot transmission that reaches consensus). If the last tick shred wasn't marked with LAST_SHRED_IN_SLOT, then this should be marked as dead.

@carllin the reason there's a separate issue for the "last tick needs to coincide with last shred" is that this PR doesn't cover it yet, and can land on its own.

yup agreed, just summarizing the difference for clarity.

jstarry · 2019-10-18T19:09:39Z

It looks like we're only verifying the hash count when blocktree processor first loads the ledger from disk. We also need to verify the hash count in replay stage as each new block is ingested

Yes, my mistake, working on a fix now

EDIT: Fixed

core/src/replay_stage.rs

jstarry · 2019-10-21T16:03:20Z

core/src/replay_stage.rs

+    }
+
+    #[test]
+    fn test_dead_fork_invalid_slot_tick_count() {


@rob-solana in response to this comment: #6262 (comment)...

I have a test here for making sure the fork is marked as dead. Looks like mark_dead_slot sets the BankProgress to dead as well as the blocktree slot 👍

ledger/src/entry.rs

core/src/replay_stage.rs

jstarry · 2019-10-24T23:17:29Z

Hm CI says not so fast, investigating...

carllin · 2019-10-24T23:35:34Z

wow @sagar-solana local-cluster catching things 😛

Pull request has been modified.

jstarry · 2019-10-25T03:14:25Z

@carllin pretty sure it's fixed, CI is just having time out issues now. Can you review my last change? fad6bda

ledger/src/blocktree_processor.rs

jstarry force-pushed the verify-num-hashes branch 2 times, most recently from 23a43d3 to 51b67b2 Compare October 8, 2019 03:35

jstarry force-pushed the verify-num-hashes branch from 51b67b2 to 76115e7 Compare October 17, 2019 05:04

jstarry marked this pull request as ready for review October 17, 2019 05:04

jstarry requested review from rob-solana and carllin October 17, 2019 14:23

mvines reviewed Oct 18, 2019

View reviewed changes

jstarry force-pushed the verify-num-hashes branch 4 times, most recently from ef84bdb to 64e4727 Compare October 20, 2019 16:10

jstarry commented Oct 20, 2019

View reviewed changes

core/src/replay_stage.rs Outdated Show resolved Hide resolved

jstarry force-pushed the verify-num-hashes branch from e3a39e4 to 22cfa26 Compare October 20, 2019 22:03

jstarry commented Oct 21, 2019

View reviewed changes

carllin reviewed Oct 21, 2019

View reviewed changes

ledger/src/entry.rs Outdated Show resolved Hide resolved

carllin reviewed Oct 21, 2019

View reviewed changes

core/src/replay_stage.rs Outdated Show resolved Hide resolved

jstarry force-pushed the verify-num-hashes branch 2 times, most recently from 694a3b3 to 05685bb Compare October 22, 2019 16:25

jstarry mentioned this pull request Oct 22, 2019

Replay stage can consider a slot to be completed before the blocktree slot is full #6498

Closed

jstarry force-pushed the verify-num-hashes branch 2 times, most recently from d5f8650 to e1fa7dd Compare October 24, 2019 22:52

carllin previously approved these changes Oct 24, 2019

View reviewed changes

jstarry force-pushed the verify-num-hashes branch from fad6bda to edf509b Compare October 25, 2019 13:23

Verify number of hashes for each block of entries

4ab9919

jstarry added 2 commits October 31, 2019 10:12

Fix blocktree processor tick check

9361e58

Rebase once more

86545d6

jstarry force-pushed the verify-num-hashes branch from edf509b to 86545d6 Compare October 31, 2019 17:33

carllin reviewed Oct 31, 2019

View reviewed changes

ledger/src/blocktree_processor.rs Show resolved Hide resolved

carllin approved these changes Oct 31, 2019

View reviewed changes

jstarry merged commit e8e5ddc into solana-labs:master Oct 31, 2019

ilya-bobyr mentioned this pull request Mar 7, 2023

make_slot_entries(): Use unique hashes #30627

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify number of hashes for each block of entries #6262

Verify number of hashes for each block of entries #6262

jstarry commented Oct 7, 2019 •

edited

Loading

codecov bot commented Oct 8, 2019 •

edited

Loading

mvines left a comment

mvines Oct 18, 2019

jstarry Oct 18, 2019

mvines Oct 18, 2019

jstarry Oct 18, 2019

mvines Oct 18, 2019

carllin Oct 22, 2019

jstarry Oct 24, 2019

carllin Oct 24, 2019 •

edited

Loading

rob-solana Oct 24, 2019

carllin Oct 24, 2019

jstarry commented Oct 18, 2019 •

edited

Loading

jstarry Oct 21, 2019

jstarry commented Oct 24, 2019

carllin commented Oct 24, 2019

jstarry commented Oct 25, 2019

	} else if current_tick_height % self.ticks_per_slot == 0 {
	// Register a new block hash if at the last tick in the slot
	Some(self.blockhash_queue.write().unwrap())
	} else {

Verify number of hashes for each block of entries #6262

Verify number of hashes for each block of entries #6262

Conversation

jstarry commented Oct 7, 2019 • edited Loading

Problem

Summary of Changes

codecov bot commented Oct 8, 2019 • edited Loading

Codecov Report

mvines left a 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

carllin Oct 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstarry commented Oct 18, 2019 • edited Loading

Choose a reason for hiding this comment

jstarry commented Oct 24, 2019

carllin commented Oct 24, 2019

jstarry commented Oct 25, 2019

jstarry commented Oct 7, 2019 •

edited

Loading

codecov bot commented Oct 8, 2019 •

edited

Loading

carllin Oct 24, 2019 •

edited

Loading

jstarry commented Oct 18, 2019 •

edited

Loading