Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Panic in NonFinalizedTree::as_chain_information #1689

Closed
DragonDev1906 opened this issue Feb 21, 2024 · 2 comments · Fixed by #1695
Closed

Panic in NonFinalizedTree::as_chain_information #1689

DragonDev1906 opened this issue Feb 21, 2024 · 2 comments · Fixed by #1695

Comments

@DragonDev1906
Copy link
Contributor

Smoldot (rust crate version: 0.17.0) crashes (Rust panic) with a NonLinearBabeEpochs Error while calling NonFinalizedTree::as_chain_information(). I do not know what exactly causes this (you probably have more knowledge about substrate internals and how smoldot works internally), but it does happen on Rococo if the finalized block is 9228000 (hash: 0xfc97a2a8632bbde91c4ec0f3c4b065d0ecfcf3d948092896328c9dfbce36e8c2).

Demonstration (rust code): babe-epoch-bug.rs.txt

Note that this happens after checking the validity of the headers and justification used, so this is either a bug in the validity checking of block data or a bug in the implementation of "as_chain_information". From what I understand there is an (implicit) invariant that as_chain_information on a correctly set-up NonFinalizedTree should never fail (hence the unwrap() that causes this panic), since the NonFinalizedTree should always be in a valid state.

Panic message (output of the attached script):

[[babe-epoch-bug-3.rs:55](http://babe-epoch-bug-3.rs:55/)] &tree = NonFinalizedTree {
    finalized_block_hash: "0xfc97a2a8632bbde91c4ec0f3c4b065d0ecfcf3d948092896328c9dfbce36e8c2",
    non_finalized_blocks: {},
}
Pre panic
thread 'main' panicked at /home/jens/.cargo/registry/src/index.crates.io-6f17d22bba15001f/smoldot-0.17.0/src/chain/blocks_tree.rs:333:72:
called `Result::unwrap()` on an `Err` value: NonLinearBabeEpochs

Relevant Code:

Steps to reproduce:

Run the above linked rust code or:

  • Use smoldot (rust crate) on Rococo
  • Get to the state where the block 9228000 is the finalized block (e.g. by processing a grandpa justification)
  • Call NonFinalizedTree::as_chain_information (does not matter if the tree contains blocks after that finalized block)

To me this looks like a broken invariant, a bug in or related to the most security critical section of smoldot and I'm pretty sure nobody expects "as_chain_information" to randomly panic either. I doubt there is a non-documented restriction that you are not allowed to call "as_chain_information on some finalized blocks.

@DragonDev1906
Copy link
Contributor Author

@DragonDev1906
Copy link
Contributor Author

This commit seems to be related to this issue. It takes the amount of skipped epochs into consideration when calculating the slot_number but not when calculating the epoch index.

From what I've seen so far the following needs to adjust the epoch index according to the skipped epochs, too, or the invariant mentioned in the issue description has to be removed.

I do not know enough about the consensus to know what is correct, it would be good if someone with more implementation and/or consensus knowledge could look at this.

            let start_slot_number = Some(
                block_epoch_info
                    .start_slot_number
                    .unwrap_or(slot_number)
                    .checked_add(config.slots_per_epoch.get())
                    .ok_or(VerifyError::NextEpochStartSlotNumberOverflow)?
                    // If some epochs have been skipped, we need to adjust the starting slot of
                    // the next epoch.
                    .checked_add(
                        skipped_epochs
                            .checked_mul(config.slots_per_epoch.get())
                            .ok_or(VerifyError::NextEpochStartSlotNumberOverflow)?,
                    )
                    .ok_or(VerifyError::NextEpochStartSlotNumberOverflow)?,
            );

            Some(chain_information::BabeEpochInformation {
                epoch_index: block_epoch_index
                    .checked_add(1)
                    .ok_or(VerifyError::EpochIndexOverflow)?,
                start_slot_number,
                authorities: info.authorities.map(Into::into).collect(),
                randomness: *info.randomness,
                c: maybe_config.map_or(block_epoch_info.c, |config| config.c),
                allowed_slots: maybe_config.map_or(block_epoch_info.allowed_slots, |config| {
                    config.allowed_slots
                }),
            })

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant