index: Compare deserialized block hash with the block hash from the blockindex

kcalvinalvin commented at 5:40 am on October 26, 2022: contributor

There is already an integrity check for the blockfilter but there aren’t any integrity checks for the block hash that’s stored with the blockfilter. We can add this check by comparing the deserialized block hash from the disk with the block hash provided to us by the blockindex.

DrahtBot added the label UTXO Db and Indexes on Oct 26, 2022

DrahtBot commented at 11:01 am on October 26, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Needs rebase on Feb 1, 2023

achow101 commented at 3:20 pm on April 25, 2023: member

Are you still working on this?

kcalvinalvin commented at 4:53 am on April 26, 2023: contributor

Are you still working on this?

Is there any interest for the PR itself? It’s not much but you can get additional assurance that the blockfilter stored on disk is not corrupted.

I can rebase if there’s interest in that feature.

furszy commented at 9:30 am on April 26, 2023: member

yeah @kcalvinalvin, please rebase it. Will check it asap.

index: Compare deserialized block hash with the block hash from the blockindex

There is already an integrity check for the blockfilter but there aren't
any integrity checks for the block hash that's stored with the
blockfilter. We can add this check by comparing the deserialized block
hash from the disk with the block hash provided to us by the blockindex.

8667641123

kcalvinalvin force-pushed on Apr 30, 2023

kcalvinalvin commented at 3:51 pm on April 30, 2023: contributor

Rebased on master

Diffs between the relevant files:

For blockfilter.h:

0[I] calvin@bitcoin ~/b/c/l/bitcoin (2022-07-13-blockfilter-check-block-hash)> git diff a2884a6c3f874c7fd16626a31557fcfb852b39bf src/index/blockfilterindex.h
1diff --git a/src/index/blockfilterindex.h b/src/index/blockfilterindex.h
2index 0afee01a74..9a651dcb1c 100644
3--- a/src/index/blockfilterindex.h
4+++ b/src/index/blockfilterindex.h
5@@ -1,4 +1,4 @@
6-// Copyright (c) 2018-2021 The Bitcoin Core developers
7+// Copyright (c) 2018-2022 The Bitcoin Core developers
8 // Distributed under the MIT software license, see the accompanying
9 // file COPYING or http://www.opensource.org/licenses/mit-license.php.

For blockfilter.cpp:

 0[I] calvin@bitcoin ~/b/c/l/bitcoin (2022-07-13-blockfilter-check-block-hash)> git diff a2884a6c3f874c7fd16626a31557fcfb852b39bf src/index/blockfilterindex.cpp
 1diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp
 2index cb503664be..94a7351c72 100644
 3--- a/src/index/blockfilterindex.cpp
 4+++ b/src/index/blockfilterindex.cpp
 5@@ -1,14 +1,15 @@
 6-// Copyright (c) 2018-2021 The Bitcoin Core developers
 7+// Copyright (c) 2018-2022 The Bitcoin Core developers
 8// Distributed under the MIT software license, see the accompanying
 9// file COPYING or http://www.opensource.org/licenses/mit-license.php.
10
11#include <map>
12
13+#include <common/args.h>
14#include <dbwrapper.h>
15#include <hash.h>
16#include <index/blockfilterindex.h>
17#include <node/blockstorage.h>
18-#include <util/system.h>
19+#include <util/fs_helpers.h>
20#include <validation.h>
21
22using node::UndoReadFromDisk;
23@@ -158,9 +159,7 @@ bool BlockFilterIndex::ReadFilterFromDisk(const FlatFilePos& pos, const uint256&
24    try {
25        filein >> read_block_hash >> encoded_filter;
26        if (read_block_hash != block_hash) return error("Checksum mismatch in filter decode.");
27-        uint256 result;
28-        CHash256().Write(encoded_filter).Finalize(result);
29-        if (result != hash) return error("Checksum mismatch in filter decode.");
30+        if (Hash(encoded_filter) != hash) return error("Checksum mismatch in filter decode.");
31        filter = BlockFilter(GetFilterType(), block_hash, std::move(encoded_filter), /*skip_decode_check=*/true);
32    }
33    catch (const std::exception& e) {

DrahtBot removed the label Needs rebase on Apr 30, 2023

DrahtBot added the label CI failed on Apr 30, 2023

in src/index/blockfilterindex.cpp:149 in 8667641123

145@@ -146,18 +146,19 @@ bool BlockFilterIndex::CustomCommit(CDBBatch& batch)
146     return true;
147 }
148 
149-bool BlockFilterIndex::ReadFilterFromDisk(const FlatFilePos& pos, const uint256& hash, BlockFilter& filter) const
150+bool BlockFilterIndex::ReadFilterFromDisk(const FlatFilePos& pos, const uint256& hash, const uint256& block_hash, BlockFilter& filter) const

furszy commented at 5:38 pm on April 30, 2023:

nit: Better if we call it expected_block_hash, and also rename the one called hash to filter_hash.

in src/index/blockfilterindex.cpp:161 in 8667641123

159+    uint256 read_block_hash;
160     std::vector<uint8_t> encoded_filter;
161     try {
162-        filein >> block_hash >> encoded_filter;
163+        filein >> read_block_hash >> encoded_filter;
164+        if (read_block_hash != block_hash) return error("Checksum mismatch in filter decode.");

furszy commented at 5:40 pm on April 30, 2023:

This error would be easier to debug if it logs an specific error like “Block hash mismatch in filter decode” (it’s currently logging the same error as a filter hash mismatch).

Plus, could leave this as it was before if the new arg is called expected_block_hash (no need to do the read_block_hash rename).

in src/index/blockfilterindex.cpp:437 in 8667641123

434 
435     filters_out.resize(entries.size());
436     auto filter_pos_it = filters_out.begin();
437     for (const auto& entry : entries) {
438-        if (!ReadFilterFromDisk(entry.pos, entry.hash, *filter_pos_it)) {
439+        if (!ReadFilterFromDisk(entry.second.pos, entry.second.hash, entry.first, *filter_pos_it)) {

furszy commented at 5:48 pm on April 30, 2023:

Could use structured bindings:

0for (const auto& [block_hash, filter] : entries) {
1    if (!ReadFilterFromDisk(filter.pos, filter.hash, block_hash, *filter_pos_it)) {

(Same below)

in src/index/blockfilterindex.cpp:330 in 8667641123

326@@ -326,7 +327,7 @@ static bool LookupOne(const CDBWrapper& db, const CBlockIndex* block_index, DBVa
327 }
328 
329 static bool LookupRange(CDBWrapper& db, const std::string& index_name, int start_height,
330-                        const CBlockIndex* stop_index, std::vector<DBVal>& results)
331+                        const CBlockIndex* stop_index, std::vector<std::pair<uint256, DBVal>>& results)

furszy commented at 5:58 pm on April 30, 2023:

Wouldn’t hurt to add a comment for this. By reading only the function’s signature, it’s not clear what the uin256 is.

furszy commented at 5:58 pm on April 30, 2023: member

Code reviewed, left few comments. Nothing big.

DrahtBot commented at 9:03 am on August 8, 2023: contributor

There hasn’t been much activity lately. What is the status here?

Finding reviewers may take time. However, if the patch is no longer relevant, please close this pull request. If the author lost interest or time to work on this, please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

DrahtBot removed the label CI failed on Aug 17, 2023

DrahtBot added the label CI failed on Aug 30, 2023

DrahtBot removed the label CI failed on Sep 4, 2023

achow101 commented at 4:35 pm on September 20, 2023: member

Closing as up for grabs due to lack of activity.

achow101 closed this on Sep 20, 2023

achow101 added the label Up for grabs on Sep 20, 2023

bitcoin locked this on Sep 19, 2024

index: Compare deserialized block hash with the block hash from the blockindex #26390

Reviews

Conflicts