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

Fix blockstore empty panic #11423

Merged

Conversation

CriesofCarrots
Copy link
Contributor

@CriesofCarrots CriesofCarrots commented Aug 6, 2020

Problem

Because of how the blockstore transaction_status_cf is initialized (to optimize compaction), an iterator on the column can match a default Signature. If slot 0 is a root, this can cause a panic.

Summary of Changes

Prevent match on index outside bounds
Also return an error instead of panicking, for redundancy

cc @jstarry

@codecov
Copy link

codecov bot commented Aug 6, 2020

Codecov Report

Merging #11423 into master will increase coverage by 0.0%.
The diff coverage is 100.0%.

@@           Coverage Diff           @@
##           master   #11423   +/-   ##
=======================================
  Coverage    81.9%    81.9%           
=======================================
  Files         320      320           
  Lines       75001    75008    +7     
=======================================
+ Hits        61450    61459    +9     
+ Misses      13551    13549    -2     

@CriesofCarrots CriesofCarrots force-pushed the fix-blockstore-empty-panic branch from 3c1212d to 86dac1d Compare August 6, 2020 18:36
@CriesofCarrots CriesofCarrots force-pushed the fix-blockstore-empty-panic branch from 86dac1d to 350f065 Compare August 6, 2020 18:44
t-nelson
t-nelson previously approved these changes Aug 6, 2020
Copy link
Contributor

@t-nelson t-nelson left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for bearing with me!

@CriesofCarrots CriesofCarrots force-pushed the fix-blockstore-empty-panic branch from 350f065 to e33da61 Compare August 6, 2020 18:46
@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Aug 6, 2020
@mergify mergify bot dismissed t-nelson’s stale review August 6, 2020 18:47

Pull request has been modified.

@mergify mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Aug 6, 2020
@mergify
Copy link
Contributor

mergify bot commented Aug 6, 2020

automerge label removed due to a CI failure

@CriesofCarrots CriesofCarrots force-pushed the fix-blockstore-empty-panic branch from e33da61 to f1ac52d Compare August 6, 2020 19:15
@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Aug 6, 2020
@mergify
Copy link
Contributor

mergify bot commented Aug 6, 2020

automerge label removed due to a CI failure

@mergify mergify bot removed the automerge Merge this Pull Request automatically once CI passes label Aug 6, 2020
@CriesofCarrots CriesofCarrots force-pushed the fix-blockstore-empty-panic branch from f1ac52d to ffaeac0 Compare August 6, 2020 19:53
Tyera Eulberg added 2 commits August 6, 2020 15:09
@CriesofCarrots CriesofCarrots added the automerge Merge this Pull Request automatically once CI passes label Aug 6, 2020
@mergify mergify bot merged commit 1061b50 into solana-labs:master Aug 6, 2020
mergify bot pushed a commit that referenced this pull request Aug 6, 2020
* Add panicking test

* Add failing test: fresh transaction-status column shouldn't point at valid root 0

* Prevent transaction status match outside of primary-index bounds

* Initialize transaction-status and address-signature primer entries with Slot::MAX

* Revert "Add failing test: fresh transaction-status column shouldn't point at valid root 0"

This reverts commit cbad2a9.

* Revert "Initialize transaction-status and address-signature primer entries with Slot::MAX"

This reverts commit ffaeac0.

(cherry picked from commit 1061b50)

# Conflicts:
#	ledger/src/blockstore.rs
mergify bot pushed a commit that referenced this pull request Aug 6, 2020
* Add panicking test

* Add failing test: fresh transaction-status column shouldn't point at valid root 0

* Prevent transaction status match outside of primary-index bounds

* Initialize transaction-status and address-signature primer entries with Slot::MAX

* Revert "Add failing test: fresh transaction-status column shouldn't point at valid root 0"

This reverts commit cbad2a9.

* Revert "Initialize transaction-status and address-signature primer entries with Slot::MAX"

This reverts commit ffaeac0.

(cherry picked from commit 1061b50)
mergify bot added a commit that referenced this pull request Aug 6, 2020
* Add panicking test

* Add failing test: fresh transaction-status column shouldn't point at valid root 0

* Prevent transaction status match outside of primary-index bounds

* Initialize transaction-status and address-signature primer entries with Slot::MAX

* Revert "Add failing test: fresh transaction-status column shouldn't point at valid root 0"

This reverts commit cbad2a9.

* Revert "Initialize transaction-status and address-signature primer entries with Slot::MAX"

This reverts commit ffaeac0.

(cherry picked from commit 1061b50)

Co-authored-by: Tyera Eulberg <[email protected]>
mergify bot added a commit that referenced this pull request Aug 7, 2020
* Fix blockstore empty panic (#11423)

* Add panicking test

* Add failing test: fresh transaction-status column shouldn't point at valid root 0

* Prevent transaction status match outside of primary-index bounds

* Initialize transaction-status and address-signature primer entries with Slot::MAX

* Revert "Add failing test: fresh transaction-status column shouldn't point at valid root 0"

This reverts commit cbad2a9.

* Revert "Initialize transaction-status and address-signature primer entries with Slot::MAX"

This reverts commit ffaeac0.

(cherry picked from commit 1061b50)

# Conflicts:
#	ledger/src/blockstore.rs

* Fix conflict

Co-authored-by: Tyera Eulberg <[email protected]>
Co-authored-by: Tyera Eulberg <[email protected]>
@CriesofCarrots CriesofCarrots deleted the fix-blockstore-empty-panic branch September 1, 2020 19:58
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.

2 participants