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

feat(state): Implements reconsider_block method #9260

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

elijahhampton
Copy link
Contributor

Motivation

The changes create and test a reconsider_block method to later be integrated into the zebra RPC. This method reconsiders previously invalidated_blocks into a chain that exist in the chain_set. Closes #8842.

Specifications & References

Solution

  • Added a reconsider_block method to NonFinalizedState that allows previously invalidated blocks to be reconsidered and re-inserted into the chain.

Tests

  • Added test coverage in 'zebra/zebra-state/src/service/non_finalized_state/tests/vectors.rs'

Follow-up Work

PR Author's Checklist

  • The PR name will make sense to users.
  • The PR provides a CHANGELOG summary.
  • The solution is tested.
  • The documentation is up to date.
  • The PR has a priority label.

PR Reviewer's Checklist

  • The PR Author's checklist is complete.
  • The PR resolves the issue.

@elijahhampton elijahhampton requested a review from a team as a code owner February 17, 2025 02:35
@elijahhampton elijahhampton requested review from arya2 and removed request for a team February 17, 2025 02:35
@github-actions github-actions bot added the C-feature Category: New features label Feb 17, 2025
@arya2 arya2 added A-state Area: State / database changes P-Medium ⚡ labels Feb 17, 2025
arya2
arya2 previously approved these changes Feb 17, 2025
Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

Thank you! This looks good, I left a suggestion for some cleanup, but there are no blockers to merging this as-is if preferred.

…m validate_and_commit if a candidate block's hash is in the map of invalidated blocks. Stores invalidated_blocks by height and clears when finalizing. Checks against non finalized tip hash to create a new chain if parnt_chain doesn't exist. Renames ReconsiderError variant NonPreviouslyInvalidatedBlock to MissingInvalidatedBlock.
Copy link
Contributor

mergify bot commented Feb 19, 2025

This pull request has been removed from the queue for the following reason: pull request dequeued.

Pull request #9260 has been dequeued, merge conditions unmatch:

  • #approved-reviews-by >= 1 [🛡 GitHub branch protection]
  • #approved-reviews-by >= 1 [🛡 GitHub repository ruleset rule]
  • any of [🛡 GitHub branch protection]:
    • check-neutral = Test stable on ubuntu-latest
    • check-skipped = Test stable on ubuntu-latest
    • check-success = Test stable on ubuntu-latest
  • any of [🛡 GitHub repository ruleset rule]:
    • check-neutral = Install zebrad from lockfile without cache on ubuntu-latest
    • check-skipped = Install zebrad from lockfile without cache on ubuntu-latest
    • check-success = Install zebrad from lockfile without cache on ubuntu-latest
  • any of [🛡 GitHub repository ruleset rule]:
    • check-neutral = Build tower-batch-control crate
    • check-skipped = Build tower-batch-control crate
    • check-success = Build tower-batch-control crate
  • any of [🛡 GitHub repository ruleset rule]:
    • check-neutral = Build zebra-chain crate
    • check-skipped = Build zebra-chain crate
    • check-success = Build zebra-chain crate
  • any of [🛡 GitHub repository ruleset rule]:
    • check-neutral = Build zebra-consensus crate
    • check-skipped = Build zebra-consensus crate
    • check-success = Build zebra-consensus crate
  • any of [🛡 GitHub repository ruleset rule]:
    • check-neutral = Build zebra-grpc crate
    • check-skipped = Build zebra-grpc crate
    • check-success = Build zebra-grpc crate
  • any of [🛡 GitHub repository ruleset rule]:
    • check-neutral = Build zebra-network crate
    • check-skipped = Build zebra-network crate
    • check-success = Build zebra-network crate
  • any of [🛡 GitHub repository ruleset rule]:
    • check-neutral = Build zebra-node-services crate
    • check-skipped = Build zebra-node-services crate
    • check-success = Build zebra-node-services crate
  • any of [🛡 GitHub repository ruleset rule]:
    • check-neutral = Build zebra-rpc crate
    • check-skipped = Build zebra-rpc crate
    • check-success = Build zebra-rpc crate
  • any of [🛡 GitHub repository ruleset rule]:
    • check-neutral = Build zebra-scan crate
    • check-skipped = Build zebra-scan crate
    • check-success = Build zebra-scan crate
  • any of [🛡 GitHub repository ruleset rule]:
    • check-neutral = Build zebra-script crate
    • check-skipped = Build zebra-script crate
    • check-success = Build zebra-script crate
  • any of [🛡 GitHub repository ruleset rule]:
    • check-neutral = Build tower-fallback crate
    • check-skipped = Build tower-fallback crate
    • check-success = Build tower-fallback crate
  • any of [🛡 GitHub repository ruleset rule]:
    • check-neutral = Build zebra-state crate
    • check-skipped = Build zebra-state crate
    • check-success = Build zebra-state crate
  • any of [🛡 GitHub repository ruleset rule]:
    • check-neutral = Build zebra-utils crate
    • check-skipped = Build zebra-utils crate
    • check-success = Build zebra-utils crate
  • any of [🛡 GitHub repository ruleset rule]:
    • check-neutral = Test beta on ubuntu-latest
    • check-skipped = Test beta on ubuntu-latest
    • check-success = Test beta on ubuntu-latest
  • any of [🛡 GitHub repository ruleset rule]:
    • check-neutral = Test stable on ubuntu-latest
    • check-skipped = Test stable on ubuntu-latest
    • check-success = Test stable on ubuntu-latest
  • any of [🛡 GitHub repository ruleset rule]:
    • check-neutral = Build CI Docker / Build images
    • check-skipped = Build CI Docker / Build images
    • check-success = Build CI Docker / Build images
  • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
  • #changes-requested-reviews-by = 0 [🛡 GitHub repository ruleset rule]
  • #review-threads-unresolved = 0 [🛡 GitHub repository ruleset rule]
  • any of [🛡 GitHub branch protection]:
    • check-skipped = Test CD custom Docker config file / Test custom-conf in Docker
    • check-neutral = Test CD custom Docker config file / Test custom-conf in Docker
    • check-success = Test CD custom Docker config file / Test custom-conf in Docker
  • any of [🛡 GitHub repository ruleset rule]:
    • check-success = Rustfmt
    • check-neutral = Rustfmt
    • check-skipped = Rustfmt
  • any of [🛡 GitHub repository ruleset rule]:
    • check-success = mergefreeze
    • check-neutral = mergefreeze
    • check-skipped = mergefreeze
  • any of [🛡 GitHub repository ruleset rule]:
    • check-success = Clippy
    • check-neutral = Clippy
    • check-skipped = Clippy
  • any of [🛡 GitHub repository ruleset rule]:
    • check-success = Check deny.toml bans
    • check-neutral = Check deny.toml bans
    • check-skipped = Check deny.toml bans
  • any of [🛡 GitHub repository ruleset rule]:
    • check-success = Check deny.toml bans --all-features
    • check-neutral = Check deny.toml bans --all-features
    • check-skipped = Check deny.toml bans --all-features
  • any of [🛡 GitHub repository ruleset rule]:
    • check-success = Check deny.toml bans --features default-release-binaries
    • check-neutral = Check deny.toml bans --features default-release-binaries
    • check-skipped = Check deny.toml bans --features default-release-binaries
  • any of [🛡 GitHub repository ruleset rule]:
    • check-success = Check deny.toml sources
    • check-neutral = Check deny.toml sources
    • check-skipped = Check deny.toml sources
  • any of [🛡 GitHub repository ruleset rule]:
    • check-success = Check deny.toml sources --all-features
    • check-neutral = Check deny.toml sources --all-features
    • check-skipped = Check deny.toml sources --all-features
  • any of [🛡 GitHub repository ruleset rule]:
    • check-success = Check deny.toml sources --features default-release-binaries
    • check-neutral = Check deny.toml sources --features default-release-binaries
    • check-skipped = Check deny.toml sources --features default-release-binaries
  • any of [🛡 GitHub repository ruleset rule]:
    • check-success = Check Cargo.lock is up to date
    • check-neutral = Check Cargo.lock is up to date
    • check-skipped = Check Cargo.lock is up to date
  • any of [🛡 GitHub repository ruleset rule]:
    • check-success = Build zebra-test crate
    • check-neutral = Build zebra-test crate
    • check-skipped = Build zebra-test crate
  • any of [🛡 GitHub repository ruleset rule]:
    • check-neutral = Mergify Merge Protections
    • check-skipped = Mergify Merge Protections
    • check-success = Mergify Merge Protections
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Build CD Docker / Build images
    • check-neutral = Build CD Docker / Build images
    • check-success = Build CD Docker / Build images
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Integration tests / Check if cached state disks exist for Mainnet / Get Mainnet cached disk
    • check-neutral = Integration tests / Check if cached state disks exist for Mainnet / Get Mainnet cached disk
    • check-success = Integration tests / Check if cached state disks exist for Mainnet / Get Mainnet cached disk
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Integration tests / Zebra checkpoint update / Run sync-past-checkpoint test
    • check-neutral = Integration tests / Zebra checkpoint update / Run sync-past-checkpoint test
    • check-success = Integration tests / Zebra checkpoint update / Run sync-past-checkpoint test
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Integration tests / Zebra tip update / Run update-to-tip test
    • check-neutral = Integration tests / Zebra tip update / Run update-to-tip test
    • check-success = Integration tests / Zebra tip update / Run update-to-tip test
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Integration tests / Generate checkpoints mainnet / Run checkpoints-mainnet test
    • check-neutral = Integration tests / Generate checkpoints mainnet / Run checkpoints-mainnet test
    • check-success = Integration tests / Generate checkpoints mainnet / Run checkpoints-mainnet test
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Integration tests / Zebra tip JSON-RPC / Run fully-synced-rpc test
    • check-neutral = Integration tests / Zebra tip JSON-RPC / Run fully-synced-rpc test
    • check-success = Integration tests / Zebra tip JSON-RPC / Run fully-synced-rpc test
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Integration tests / lightwalletd tip send / Run lwd-send-transactions test
    • check-neutral = Integration tests / lightwalletd tip send / Run lwd-send-transactions test
    • check-success = Integration tests / lightwalletd tip send / Run lwd-send-transactions test
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Integration tests / get block template / Run get-block-template test
    • check-neutral = Integration tests / get block template / Run get-block-template test
    • check-success = Integration tests / get block template / Run get-block-template test
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Integration tests / submit block / Run submit-block test
    • check-neutral = Integration tests / submit block / Run submit-block test
    • check-success = Integration tests / submit block / Run submit-block test
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Integration tests / lightwalletd tip update / Run lwd-update-sync test
    • check-neutral = Integration tests / lightwalletd tip update / Run lwd-update-sync test
    • check-success = Integration tests / lightwalletd tip update / Run lwd-update-sync test
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Integration tests / lightwalletd GRPC tests / Run lwd-grpc-wallet test
    • check-neutral = Integration tests / lightwalletd GRPC tests / Run lwd-grpc-wallet test
    • check-success = Integration tests / lightwalletd GRPC tests / Run lwd-grpc-wallet test
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Unit tests / Test all
    • check-neutral = Unit tests / Test all
    • check-success = Unit tests / Test all
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Unit tests / Test with fake activation heights
    • check-neutral = Unit tests / Test with fake activation heights
    • check-success = Unit tests / Test with fake activation heights
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Unit tests / Test checkpoint sync from empty state
    • check-neutral = Unit tests / Test checkpoint sync from empty state
    • check-success = Unit tests / Test checkpoint sync from empty state
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Unit tests / Test integration with lightwalletd
    • check-neutral = Unit tests / Test integration with lightwalletd
    • check-success = Unit tests / Test integration with lightwalletd
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Unit tests / Test CI default Docker config file / Test default-conf in Docker
    • check-neutral = Unit tests / Test CI default Docker config file / Test default-conf in Docker
    • check-success = Unit tests / Test CI default Docker config file / Test default-conf in Docker
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Unit tests / Test CI testnet Docker config file / Test testnet-conf in Docker
    • check-neutral = Unit tests / Test CI testnet Docker config file / Test testnet-conf in Docker
    • check-success = Unit tests / Test CI testnet Docker config file / Test testnet-conf in Docker
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Unit tests / Test CI custom Docker config file / Test custom-conf in Docker
    • check-neutral = Unit tests / Test CI custom Docker config file / Test custom-conf in Docker
    • check-success = Unit tests / Test CI custom Docker config file / Test custom-conf in Docker
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Test CD default Docker config file / Test default-conf in Docker
    • check-neutral = Test CD default Docker config file / Test default-conf in Docker
    • check-success = Test CD default Docker config file / Test default-conf in Docker
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Test CD testnet Docker config file / Test testnet-conf in Docker
    • check-neutral = Test CD testnet Docker config file / Test testnet-conf in Docker
    • check-success = Test CD testnet Docker config file / Test testnet-conf in Docker
  • any of [🛡 GitHub repository ruleset rule]:
    • check-skipped = Test CD custom Docker config file / Test custom-conf in Docker
    • check-neutral = Test CD custom Docker config file / Test custom-conf in Docker
    • check-success = Test CD custom Docker config file / Test custom-conf in Docker
  • any of [🛡 GitHub repository ruleset rule]:
    • check-success = Get disk name / Get Mainnet cached disk
    • check-neutral = Get disk name / Get Mainnet cached disk
    • check-skipped = Get disk name / Get Mainnet cached disk
  • any of [🛡 GitHub repository ruleset rule]:
    • check-success = mergefreeze
    • check-neutral = mergefreeze
    • check-skipped = mergefreeze

You should look at the reason for the failure and decide if the pull request needs to be fixed or if you want to requeue it.

If you want to requeue this pull request, you need to post a comment with the text: @mergifyio requeue

Copy link
Contributor

@arya2 arya2 left a comment

Choose a reason for hiding this comment

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

This is looking great, thank you for adding the IndexMap.

There's one small issue around adding chains to the non-finalized state with roots below the finalized tip, but otherwise this PR looks ready to merge.

…). Removes unused ReconsiderError variant. Opts to refuse block consideration if parent_chain does not exist. Adds db handle to reconsider_block function. Edits max blocks constant documentation
@elijahhampton elijahhampton requested a review from arya2 February 21, 2025 18:38
Comment on lines +366 to +369
if let Some(best_chain_root_height) = finalized_state.finalized_tip_height() {
self.invalidated_blocks
.retain(|height, _blocks| *height >= best_chain_root_height);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: It would be better to check the finalized tip height before calling self.parent_chain() so it could insert a new chain instead.

parent_chain() will return None if the block's parent is missing in the non-finalized state, but if the block's parent is the finalized tip, normally Zebra inserts a new chain. It's not crucial for the reconsiderblock method, but it would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I understand what you are saying now. If the parent is already finalized it would better to create a new chain of non finalized blocks and insert that. If not then we can make the call to parent_chain and proceed

…ized blocks only before checking parent_chain.
@elijahhampton elijahhampton requested a review from arya2 February 22, 2025 01:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-state Area: State / database changes C-feature Category: New features P-Medium ⚡
Projects
Status: Review/QA
2 participants