Change block index verification

[braingram]

Prior to this PR the first and last blocks were loaded.

Now, prior to the index finding the start of the first block is found
The first blocks typically starts immediately after the tree.
This is nice because the code just finished reading the tree meaning
checking for the first block should usually only take reading 4 bytes
to read the block magic. The exception is when the tree is padded.
Here the entire padding needs to be read.

Now that the code knows where the first block starts, it uses
that offset to check the block index (if one is found). If the
first index doesn't match, the code falls back to reading serially.
If it matches, the code uses the block index.

This access pattern should be more efficient (and a better match
to cloud access) where the old code did something like the following.

=== old routine ===

Assume we have a file with:
[ ---- tree --- padding blk0 blk1 ... blkN index ]

For old and new reading starts with the tree
fp index
|
[ ---- tree --- ********************************** ]

For old next the index was found which required a seek to the end
then reading backwards to find the index
|----------------------- seek --->|
[ ---- tree --- **************************** index ]

then a seek to the start to read the first block (after padding)
|<-seek-----------------|
[ ---- tree --- padding blk0 ************* index ]

then a seek to the last block and read it
---seek->|
[ ---- tree --- padding blk0 ******** blkN index ]

=== new routine ===

For the new routine the process is simplier and starts the same with
reading the tree:
fp index
|
[ ---- tree --- ********************************** ]

Then the first block is found (just the magic)
|
[ ---- tree --- padding ************************ ]

Then the file is searched for a block index (and read):
|-------------- seek --->|
[ ---- tree --- padding *******************index ]

and that's it.

One possible (and very unlikey) downside is if a file somehow has

• a valid block index
• a valid first block offset
• an invalid last block offset

we would not catch the error (but we would before). It seems impossible
to generate such a file without manually modifying block headers. If
that's happening there is no reason non first/last blocks might also
be incorrect so the old routine was also not absolute.

Tasks

• run prek on your machine
• run pytest on your machine
• Does this PR add new features and / or change user-facing code / API? (if not, label with no-changelog-entry-needed)
  • write news fragment(s) in changes/: echo "changed something" > changes/<PR#>.<changetype>.rst (see below for change types)
  • update relevant docstrings and / or docs/ page
  • for any new features, add unit tests

news fragment change types...

• changes/<PR#>.feature.rst: new feature
• changes/<PR#>.bugfix.rst: bug fix
• changes/<PR#>.doc.rst: documentation change
• changes/<PR#>.removal.rst: deprecation or removal of public API
• changes/<PR#>.general.rst: infrastructure or miscellaneous change

[codspeed-hq]

Merging this PR will not alter performance

✅ 122 untouched benchmarks

---

_{Comparing braingram:change_index_verification (1a41fe0) with main (b763db8)}

[sydduckworth]

There's a chunk of code not being hit by unit tests, which is the logic described in #2062 that emits a warning if there's an empty block index but no blocks.
Should probably add tests to capture that behavior.

Otherwise LGTM

[braingram]

There's a chunk of code not being hit by unit tests, which is the logic described in #2062 that emits a warning if there's an empty block index but no blocks. Should probably add tests to capture that behavior.

Otherwise LGTM

Thanks! Good eye finding the uncovered code. I pushed a change in 2e43751 that:

• makes the warning within that block consistent with the other warnings
• updates the test_invalid_block_index test to cover that code (I noticed this test wasn't truncating the file after trashing the index and since truncation is a mess on windows I updated the test to use an in-memory buffer instead of writing to disk)

The updated test was causing issues with pyrefly. Part of this seemed to be an incorrect type for the return value of read_block_index so I updated that to be list[int] instead of list[int | None].

[sydduckworth]

The updated test was causing issues with pyrefly. Part of this seemed to be an incorrect type for the return value of read_block_index so I updated that to be list[int] instead of list[int | None].

Ah yeah I think I set the typing of that function because when constructing the block index the values can be None, and I hadn't yet found the logic that prevents writing out a block index that contains None values.