Skip to content

Conversation

richardkiss
Copy link
Contributor

Purpose:

Current Behavior:

New Behavior:

Testing Notes:

@richardkiss richardkiss requested a review from a team as a code owner July 30, 2025 21:17
@richardkiss richardkiss marked this pull request as draft July 30, 2025 21:17
@richardkiss richardkiss added the Changed Required label for PR that categorizes merge commit message as "Changed" for changelog label Jul 30, 2025
@richardkiss richardkiss requested a review from Copilot July 30, 2025 21:24
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR simplifies the BlockStore interface by adding helper methods and improving error handling, making the code more maintainable and consistent. The changes eliminate direct access to internal database wrapper methods and provide cleaner abstractions.

  • Adds transaction() and get_block_from_cache() helper methods to BlockStore
  • Replaces database version checks with exception-based error handling using UnsupportedDatabaseVersionError
  • Improves batch processing in get_block_records_by_hash() to respect database parameter limits

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
chia/full_node/block_store.py Adds helper methods and exception-based error handling for database operations
chia/simulator/full_node_simulator.py Uses new transaction() helper method instead of direct database wrapper access
chia/full_node/full_node_api.py Replaces version check with exception handling for unsupported database operations
chia/consensus/blockchain.py Uses new helper methods and removes manual batch processing logic
chia/_tests/blockchain/test_blockchain.py Updates test to use simplified API without batch_size parameter
chia/_tests/blockchain/blockchain_test_utils.py Uses new transaction() helper method
Comments suppressed due to low confidence (1)

@richardkiss richardkiss force-pushed the block_store_abstractions branch from 0bb7113 to bda9f37 Compare July 30, 2025 22:28
@richardkiss richardkiss marked this pull request as ready for review July 30, 2025 22:30
@richardkiss richardkiss requested review from altendky and arvidn July 30, 2025 22:30
aqk
aqk previously approved these changes Jul 31, 2025
Copy link
Contributor

@aqk aqk left a comment

Choose a reason for hiding this comment

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

nice.

arvidn
arvidn previously approved these changes Jul 31, 2025
@arvidn
Copy link
Contributor

arvidn commented Jul 31, 2025

test coverage:

chia/full_node/block_store.py (93.3%): Missing lines 482

@arvidn
Copy link
Contributor

arvidn commented Jul 31, 2025

the one line missed by test coverage is the small change you made:

        if self.db_wrapper.db_version != 2:
            raise RuntimeError("get_block_bytes_in_range requires DB version 2")

This used to be an assert, which suggests that this is checked earlier already

Copy link
Contributor

@almogdepaz almogdepaz left a comment

Choose a reason for hiding this comment

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

we get_block_records_at for weight proof creation to get all sub epoch summary blocks, the current number of ses blocks should be something like 19283 so this would break when creating weight proofs on mainnet

@almogdepaz almogdepaz dismissed their stale review July 31, 2025 20:00

false alarm

almogdepaz
almogdepaz previously approved these changes Jul 31, 2025
@richardkiss richardkiss dismissed stale reviews from almogdepaz, arvidn, and aqk via aec98d0 August 1, 2025 00:15
@cmmarslender cmmarslender merged commit cac84b5 into Chia-Network:main Aug 1, 2025
503 of 515 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Changed Required label for PR that categorizes merge commit message as "Changed" for changelog

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants