Verify the block filter hash when reading from disk.

pstratem commented at 0:19 am on June 15, 2020: contributor

What’s stored in the block filter index data files is the block_hash and the encoded gcs filter.

If there’s a disk error that corrupts the encoded filter there’s no way for the node to know.

Clients that use the “header” chain will notice the error though.

It takes 1ms to verify the hash on a filter with 10000 elements.

fanquake added the label UTXO Db and Indexes on Jun 15, 2020

MarcoFalke added the label P2P on Jun 15, 2020

pstratem commented at 1:53 am on June 15, 2020: contributor

the Travis failure doesn’t seem related

jonatack commented at 9:17 am on June 15, 2020: member

Code review ACK 180e094d52262322d525ccdef9, did not benchmark the performance cost.

MarcoFalke commented at 10:24 am on June 15, 2020: member

We don’t do this when reading blocks from disk and serving them to p2p. Also, with silent corruption on disk, wouldn’t it be better to abort the node?

pstratem force-pushed on Jun 15, 2020

pstratem commented at 4:18 pm on June 15, 2020: contributor

@MarcoFalke that’s a good point, I’ve changed it so that we don’t run the checks in GCSFilter::GCSFilter when the encoded filter hash has been checked. That check is about twice as expensive as calculating the hash.

pstratem commented at 4:31 pm on June 15, 2020: contributor

I’ve added the benchmarks I used.

DrahtBot commented at 7:46 pm on June 15, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#18011 (Replace current benchmarking framework with nanobench by martinus)

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.

Verify the block filter hash when reading from disk.

Don't run the (relatively) expensive sanity check in GCSFilter constructor
if we've checked the encoded filter hash.

3f0be5abab

Add DecodeGCSFilter and BlockFilterGetHash benchmarks.

Also standardize on the BASIC filter parameters so we can compare between all the benchmarks.

1b7c2a07c4

Add DecodeCheckedGCSFilter benchmark 75e86b1884

pstratem force-pushed on Jun 15, 2020

ryanofsky approved

ryanofsky commented at 8:56 pm on June 30, 2020: member

Code review ACK 75e86b18846aa21a88463dfd3e710534afb6c104. If I’m understanding this correctly, it is replacing slower less comprehensive checks that filters contain the right number of elements with faster checks verifying complete hashes of the filters. It also adds a benchmark. So this seems better in every respect.

in src/blockfilter.cpp:93 in 75e86b1884

 96-    for (uint64_t i = 0; i < m_N; ++i) {
 97-        GolombRiceDecode(bitreader, m_params.m_P);
 98-    }
 99-    if (!stream.empty()) {
100-        throw std::ios_base::failure("encoded_filter contains excess data");
101+    if (!filter_checked) {

jonatack commented at 12:55 pm on July 4, 2020:

3f0be5a If you retouch, returning early might be preferable and a simpler change (feel free to ignore).

0+    if (filter_checked) return;
1-    if (!filter_checked) {
2-       .../...
3-       }

in src/index/blockfilterindex.cpp:158 in 75e86b1884

153@@ -153,7 +154,10 @@ bool BlockFilterIndex::ReadFilterFromDisk(const FlatFilePos& pos, BlockFilter& f
154     std::vector<unsigned char> encoded_filter;
155     try {
156         filein >> block_hash >> encoded_filter;
157-        filter = BlockFilter(GetFilterType(), block_hash, std::move(encoded_filter));
158+        uint256 result;
159+        CHash256().Write(encoded_filter.data(), encoded_filter.size()).Finalize(result.begin());

jonatack commented at 3:02 pm on July 4, 2020:

This looks like a future candidate to become a Span.

jonatack commented at 3:06 pm on July 4, 2020: member

ACK 75e86b1 per git diff 180e094 75e86b1

Ran comparative benchmarks built with --enable-bench CXXFLAGS="-O2".

This branch rebased on master:

0# Benchmark, evals, iterations, total, min, max, median
1DecodeCheckedGCSFilter, 100, 50000, 4.28914, 7.30889e-07, 1.02493e-06, 8.58921e-07
2
3DecodeGCSFilter, 10, 5000, 16.2784, 0.000314371, 0.000337642, 0.000329746
4
5BlockFilterGetHash, 10, 10000, 15.4826, 0.000142116, 0.000163642, 0.000158849
6
7ConstructGCSFilter, 10, 1000, 22.988, 0.00217746, 0.00253763, 0.00227277
8
9MatchGCSFilter, 10, 50000, 18.8786, 3.56681e-05, 4.12871e-05, 3.7422e-05

Master, with the standardized BASIC filter params. Ran twice:

0# Benchmark, evals, iterations, total, min, max, median
1ConstructGCSFilter, 10, 1000, 23.2998, 0.00226351, 0.00248678, 0.00233295
2ConstructGCSFilter, 10, 1000, 23.2406, 0.00225238, 0.00251339, 0.00230733
3
4MatchGCSFilter, 10, 50000, 19.3379, 3.70859e-05, 4.26215e-05, 3.85519e-05
5MatchGCSFilter, 10, 50000, 18.7122, 3.54355e-05, 3.90555e-05, 3.78147e-05

DrahtBot added the label Needs rebase on Jul 30, 2020

DrahtBot commented at 2:29 pm on July 30, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot commented at 11:21 am on December 15, 2021: member

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose 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.

MarcoFalke commented at 1:53 pm on December 15, 2021: member

This had two acks before conflicting. @pstratem are you still working on this or should someone pick this up?

DrahtBot commented at 1:07 pm on March 21, 2022: member

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose 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.

MarcoFalke added the label Up for grabs on Mar 21, 2022

MarcoFalke closed this on Mar 21, 2022

fanquake removed the label Up for grabs on Apr 12, 2022

fanquake referenced this in commit 5abbc9afec on Jul 7, 2022

sidhujag referenced this in commit a42480f022 on Jul 8, 2022

DrahtBot locked this on Apr 12, 2023

Verify the block filter hash when reading from disk. #19280

Conflicts