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

refactor: simplify ChainIndex, hoist out checkpoints #3220

Merged
merged 29 commits into from
Jul 19, 2023

Conversation

lemmih
Copy link
Contributor

@lemmih lemmih commented Jul 18, 2023

Summary of changes

Changes introduced in this pull request:

  • Get rid of the skip-link code. It was used to query really old epochs but we don't need it.
  • Move tipset_by_height from ChainStore to ChainIndex.
  • Refactor the known blocks (aka checkpoints) to be simpler. They no longer need to be verified in the daemon (can be handled offline.)
  • Add new forest-cli archive checkpoints command for generating new checkpoints.

Reference issue to close (if applicable)

Closes

Other information and links

Change checklist

  • I have performed a self-review of my own code,
  • I have made corresponding changes to the documentation,
  • I have added tests that prove my fix is effective or that my feature works (if possible),
  • I have made sure the CHANGELOG is up-to-date. All user-facing changes should be reflected in this document.

@lemmih lemmih changed the base branch from main to lemmih/refactor-chain-store July 18, 2023 09:11
@lemmih lemmih marked this pull request as ready for review July 18, 2023 09:45
@lemmih lemmih requested a review from a team as a code owner July 18, 2023 09:45
@lemmih lemmih requested review from LesnyRumcajs and aatifsyed and removed request for a team July 18, 2023 09:45
@lemmih
Copy link
Contributor Author

lemmih commented Jul 18, 2023

This needs to be reviewed first: #3216

Base automatically changed from lemmih/refactor-chain-store to main July 18, 2023 11:10
@lemmih lemmih marked this pull request as draft July 18, 2023 12:49
@lemmih lemmih marked this pull request as ready for review July 18, 2023 16:58
Copy link
Contributor

@aatifsyed aatifsyed left a comment

Choose a reason for hiding this comment

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

Generally happy with these changes, only serious comment is #3220 (comment)

Comment on lines +1 to +8
# This file maps epochs to block headers for calibnet and mainnet. Forest use
# this mapping to quickly identify the origin network of a tipset.
#
# Block headers can be inspected on filfox:
# - https://filfox.info/en/block/bafy2bzacebnfm6dvxo7sm5thcxnv3kttoamb53uxycnvtdxgk5mh7d73qlly2
# - https://calibration.filfox.info/en/block/bafy2bzacedhkkz76zdekpexha55b42eop42e24ajmajm26wws4nbvtq7louvu
#
# This file was generated with `forest-cli archive checkpoints`
Copy link
Contributor

Choose a reason for hiding this comment

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

new issue:

I'd like to remove the serialization for this, and turn it into rust code, probably using something like databake, but that's a little heavyweight - we could then just

struct KnownBlocks<'a> {
    calibnet: &'a [(i64, Cid)],
    mainnet: &'a [(i64, Cid)],
}
const KNOWN_BLOCKS: KnownBlocks<'static> = todo!();

Unfortunately, multihash and cid aren't there yet:
multiformats/rust-cid#138
multiformats/rust-multihash#330

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Why should it be Rust code?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's less work, and makes an error state impossible.
Consider this:

use std::net::Ipv4Addr;

const MEH: &str = "127.0.0.1";
const NICE: Ipv4Addr = Ipv4Addr::new(127, 0, 0, 1);

fn meh() -> Ipv4Addr {
    MEH.parse().unwrap()
}

fn nice() -> Ipv4Addr {
    NICE
}

Why should meh() need to unwrap, when it can be NICE?
NICE also facilitates inlining, and all that other good stuff :)

Copy link
Contributor

Choose a reason for hiding this comment

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

Even the simple case can't be const-evaluated by the compiler: https://godbolt.org/z/ooqGrhdxE

src/blocks/tipset.rs Show resolved Hide resolved
src/chain/store/index.rs Outdated Show resolved Hide resolved
src/chain/store/index.rs Show resolved Hide resolved
src/chain/store/index.rs Outdated Show resolved Hide resolved
src/chain/store/index.rs Show resolved Hide resolved
src/chain/store/index.rs Show resolved Hide resolved
src/chain/store/index.rs Show resolved Hide resolved
src/chain/store/index.rs Show resolved Hide resolved
src/chain/store/index.rs Show resolved Hide resolved
lemmih and others added 2 commits July 19, 2023 06:23
Co-authored-by: Aatif Syed <38045910+aatifsyed@users.noreply.github.com>
src/blocks/tipset.rs Show resolved Hide resolved
src/blocks/tipset.rs Show resolved Hide resolved
src/chain/store/chain_store.rs Outdated Show resolved Hide resolved
src/cli/subcommands/archive_cmd.rs Show resolved Hide resolved
src/cli/subcommands/archive_cmd.rs Show resolved Hide resolved
src/chain/store/index.rs Outdated Show resolved Hide resolved
Copy link
Member

@LesnyRumcajs LesnyRumcajs left a comment

Choose a reason for hiding this comment

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

rock-solid

@lemmih lemmih added this pull request to the merge queue Jul 19, 2023
Merged via the queue into main with commit 68a0fd8 Jul 19, 2023
20 checks passed
@lemmih lemmih deleted the lemmih/rewrite-chain-index branch July 19, 2023 08:59
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 this pull request may close these issues.

None yet

3 participants