index: Compare deserialized block hash with the block hash from the blockindex #26390
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
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. |
|
yeah @kcalvinalvin, please rebase it. Will check it asap. |
…lockindex 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.
kcalvinalvin
force-pushed
the
2022-07-13-blockfilter-check-block-hash
branch
from
a2884a6 to
8667641
Compare
|
Rebased on master Diffs between the relevant files: For [I] calvin@bitcoin ~/b/c/l/bitcoin (2022-07-13-blockfilter-check-block-hash)> git diff a2884a6c3f874c7fd16626a31557fcfb852b39bf src/index/blockfilterindex.h
diff --git a/src/index/blockfilterindex.h b/src/index/blockfilterindex.h
index 0afee01a74..9a651dcb1c 100644
--- a/src/index/blockfilterindex.h
+++ b/src/index/blockfilterindex.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2018-2021 The Bitcoin Core developers
+// Copyright (c) 2018-2022 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.For [I] calvin@bitcoin ~/b/c/l/bitcoin (2022-07-13-blockfilter-check-block-hash)> git diff a2884a6c3f874c7fd16626a31557fcfb852b39bf src/index/blockfilterindex.cpp
diff --git a/src/index/blockfilterindex.cpp b/src/index/blockfilterindex.cpp
index cb503664be..94a7351c72 100644
--- a/src/index/blockfilterindex.cpp
+++ b/src/index/blockfilterindex.cpp
@@ -1,14 +1,15 @@
-// Copyright (c) 2018-2021 The Bitcoin Core developers
+// Copyright (c) 2018-2022 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <map>
+#include <common/args.h>
#include <dbwrapper.h>
#include <hash.h>
#include <index/blockfilterindex.h>
#include <node/blockstorage.h>
-#include <util/system.h>
+#include <util/fs_helpers.h>
#include <validation.h>
using node::UndoReadFromDisk;
@@ -158,9 +159,7 @@ bool BlockFilterIndex::ReadFilterFromDisk(const FlatFilePos& pos, const uint256&
try {
filein >> read_block_hash >> encoded_filter;
if (read_block_hash != block_hash) return error("Checksum mismatch in filter decode.");
- uint256 result;
- CHash256().Write(encoded_filter).Finalize(result);
- if (result != hash) return error("Checksum mismatch in filter decode.");
+ if (Hash(encoded_filter) != hash) return error("Checksum mismatch in filter decode.");
filter = BlockFilter(GetFilterType(), block_hash, std::move(encoded_filter), /*skip_decode_check=*/true);
}
catch (const std::exception& e) { |
furszy
left a comment
furszy
left a comment
There was a problem hiding this comment.
Code reviewed, left few comments. Nothing big.
| } | ||
|
|
||
| bool BlockFilterIndex::ReadFilterFromDisk(const FlatFilePos& pos, const uint256& hash, BlockFilter& filter) const | ||
| bool BlockFilterIndex::ReadFilterFromDisk(const FlatFilePos& pos, const uint256& hash, const uint256& block_hash, BlockFilter& filter) const |
There was a problem hiding this comment.
nit:
Better if we call it expected_block_hash, and also rename the one called hash to filter_hash.
| uint256 read_block_hash; | ||
| std::vector<uint8_t> encoded_filter; | ||
| try { | ||
| filein >> block_hash >> encoded_filter; | ||
| filein >> read_block_hash >> encoded_filter; | ||
| if (read_block_hash != block_hash) return error("Checksum mismatch in filter decode."); |
There was a problem hiding this comment.
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).
| for (const auto& entry : entries) { | ||
| if (!ReadFilterFromDisk(entry.pos, entry.hash, *filter_pos_it)) { | ||
| if (!ReadFilterFromDisk(entry.second.pos, entry.second.hash, entry.first, *filter_pos_it)) { |
There was a problem hiding this comment.
Could use structured bindings:
for (const auto& [block_hash, filter] : entries) {
if (!ReadFilterFromDisk(filter.pos, filter.hash, block_hash, *filter_pos_it)) {(Same below)
|
|
||
| static bool LookupRange(CDBWrapper& db, const std::string& index_name, int start_height, | ||
| const CBlockIndex* stop_index, std::vector<DBVal>& results) | ||
| const CBlockIndex* stop_index, std::vector<std::pair<uint256, DBVal>>& results) |
There was a problem hiding this comment.
Wouldn't hurt to add a comment for this. By reading only the function's signature, it's not clear what the uin256 is.
|
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. |
|
Closing as up for grabs due to lack of activity. |
4 participants
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.