Skip to content

Backport 12254 and 14073 (BIP158)#3416

Merged
UdjinM6 merged 6 commits intodashpay:developfrom
PastaPastaPasta:backport-12254
Jun 16, 2020
Merged

Backport 12254 and 14073 (BIP158)#3416
UdjinM6 merged 6 commits intodashpay:developfrom
PastaPastaPasta:backport-12254

Conversation

@PastaPastaPasta
Copy link
Member

@PastaPastaPasta PastaPastaPasta commented Apr 16, 2020 •
edited
Loading

builds on #3415, currently failing. Will rebase after #3415

Backports bitcoin#12254 and bitcoin#14073

@PastaPastaPasta PastaPastaPasta added this to the 17 milestone May 14, 2020
@PastaPastaPasta PastaPastaPasta mentioned this pull request Jun 12, 2020
Merged
laanwj and others added 5 commits June 13, 2020 14:08
@laanwj @PastaPastaPasta
Merge bitcoin#12254: BIP 158: Compact Block Filters for Light Clients
1792907 
254c85b bench: Benchmark GCS filter creation and matching. (Jim Posen)
f33b717 blockfilter: Optimization on compilers with int128 support. (Jim Posen)
97b64d6 blockfilter: Unit test against BIP 158 test vectors. (Jim Posen)
a4afb9c blockfilter: Additional helper methods to compute hash and header. (Jim Posen)
cd09c79 blockfilter: Serialization methods on BlockFilter. (Jim Posen)
c1855f6 blockfilter: Construction of basic block filters. (Jim Posen)
53e7874 blockfilter: Simple test for GCSFilter construction and Match. (Jim Posen)
558c536 blockfilter: Implement GCSFilter Match methods. (Jim Posen)
cf70b55 blockfilter: Implement GCSFilter constructors. (Jim Posen)
c454f0a blockfilter: Declare GCSFilter class for BIP 158 impl. (Jim Posen)
9b622dc streams: Unit tests for BitStreamReader and BitStreamWriter. (Jim Posen)
fe943f9 streams: Implement BitStreamReader/Writer classes. (Jim Posen)
87f2d9e streams: Unit test for VectorReader class. (Jim Posen)
947133d streams: Create VectorReader stream interface for vectors. (Jim Posen)

Pull request description:

  This implements the compact block filter construction in [BIP 158](https://github.com/bitcoin/bips/blob/master/bip-0158.mediawiki). The code is not used anywhere in the Bitcoin Core code base yet. The next step towards [BIP 157](https://github.com/bitcoin/bips/blob/master/bip-0157.mediawiki) support would be to create an indexing module similar to `TxIndex` that constructs the basic and extended filters for each validated block.

  ### Filter Sizes

  [Here](https://gateway.ipfs.io/ipfs/QmRqaAAQZ5ZX5eqxP7J2R1MzFrc2WDdKSWJEKtQzyawqog) is a CSV of filter sizes for blocks in the main chain.

  As you can see below, the ratio of filter size to block size drops after the first ~150,000 blocks:

  ![filter_sizes](https://user-images.githubusercontent.com/881253/42900589-299772d4-8a7e-11e8-886d-0d4f3f4fbe44.png)

  The reason for the relatively large filter sizes is that Golomb-coded sets only achieve good compression with a sufficient number of elements. Empirically, the average element size with 100 elements is 14% larger than with 10,000 elements.

  The ratio of filter size to block size is computed without witness data for basic filters. Here is a summary table of filter size ratios *for blocks after height 150,000*:

  | Stat | Filter Type |
  |-------|--------------|
  | Weighted Size Ratio Mean | 0.0198 |
  | Size Ratio Mean | 0.0224 |
  | Size Ratio Std Deviation | 0.0202 |
  | Mean Element Size (bits) | 21.145 |
  | Approx Theoretical Min Element Size (bits) | 21.025 |

Tree-SHA512: 2d045fbfc3fc45490ecb9b08d2f7e4dbbe7cd8c1c939f06bbdb8e8aacfe4c495cdb67c820e52520baebbf8a8305a0efd8e59d3fa8e367574a4b830509a39223f
@PastaPastaPasta
comment out part of benchmark
ce81cc4 
Signed-off-by: Pasta <pasta@dashboost.org>
@PastaPastaPasta
test_bitcoin -> test_dash
27a4351 
Signed-off-by: Pasta <pasta@dashboost.org>
@PastaPastaPasta
Rename BlockFilterType::BASIC to BASIC_FILTER
223dfc9 
bls dependency defines a macro BASIC as 1 in relic_conf.h.
This caused blockfilter.h to not compile after macro expansion when it says BASIC = 0.

Maybe there is a fancy C++ way to solve this, but renaming it seemed good to me :)

Signed-off-by: Pasta <pasta@dashboost.org>
@laanwj @PastaPastaPasta
Merge bitcoin#14073: blockfilter: Avoid out-of-bounds script access.
ea90d1b 
f055995 blockfilter: Omit empty scripts from filter contents. (Jim Posen)

Pull request description:

  Caught during review of bitcoin#12254 by @TheBlueMatt. bitcoin#12254 (comment)

Tree-SHA512: cfc9e3eeaba12a14fd3d2e1ccce1a1f89e8cf44cc340ceec05d2d5fa61d27ff64e355603f4ad2184ff73c0ed23dfdab6e2103bddc48f3b76cb13b88d428770ac
@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented Jun 13, 2020

functional tests looking good, code looks good to me however Unit Tests fail with

FAIL: test/test_dash
====================

Running 303 test cases...
test/blockfilter_tests.cpp(137): error: in "blockfilter_tests/blockfilters_json_test": check computed_filter_basic.GetFilter().GetEncoded() == filter_basic has failed
test/blockfilter_tests.cpp(140): error: in "blockfilter_tests/blockfilters_json_test": check computed_header_basic == filter_header_basic has failed
test/blockfilter_tests.cpp(137): error: in "blockfilter_tests/blockfilters_json_test": check computed_filter_basic.GetFilter().GetEncoded() == filter_basic has failed
test/blockfilter_tests.cpp(140): error: in "blockfilter_tests/blockfilters_json_test": check computed_header_basic == filter_header_basic has failed
test/blockfilter_tests.cpp(137): error: in "blockfilter_tests/blockfilters_json_test": check computed_filter_basic.GetFilter().GetEncoded() == filter_basic has failed
test/blockfilter_tests.cpp(140): error: in "blockfilter_tests/blockfilters_json_test": check computed_header_basic == filter_header_basic has failed
test/blockfilter_tests.cpp(137): error: in "blockfilter_tests/blockfilters_json_test": check computed_filter_basic.GetFilter().GetEncoded() == filter_basic has failed
test/blockfilter_tests.cpp(140): error: in "blockfilter_tests/blockfilters_json_test": check computed_header_basic == filter_header_basic has failed
test/blockfilter_tests.cpp(137): error: in "blockfilter_tests/blockfilters_json_test": check computed_filter_basic.GetFilter().GetEncoded() == filter_basic has failed
test/blockfilter_tests.cpp(140): error: in "blockfilter_tests/blockfilters_json_test": check computed_header_basic == filter_header_basic has failed
test/blockfilter_tests.cpp(120): fatal error: in "blockfilter_tests/blockfilters_json_test": critical check DecodeHexBlk(block, test[pos++].get_str()) has failed

*** 11 failures are detected in the test module "Bitcoin Test Suite"
FAIL test/test_dash (exit status: 201)

I'm not sure if this is dependant on earlier PRs. I can't find any. This is low priority for review.

@PastaPastaPasta PastaPastaPasta marked this pull request as ready for review June 13, 2020 20:28
UdjinM6
UdjinM6 requested changes Jun 14, 2020
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

Block filters rely on block hashes and we use X11, not sha256d. Also test data should have no segwit txes and no tests for witness also. UdjinM6@dbddd36 should fix these issues.

@UdjinM6 @PastaPastaPasta
Dashify blockfilter tests
436872a 
Drop segwit txes and witness test case. Update block hashes (x11 instead of sha256d), block filters and block filter headers.
@PastaPastaPasta
Copy link
Member Author

PastaPastaPasta commented Jun 14, 2020

Cherry-picked 👍 That makes sense

@UdjinM6 UdjinM6 changed the title Backport 12254 Backport 12254 and 14073 (BIP158) Jun 14, 2020
@PastaPastaPasta PastaPastaPasta requested a review from UdjinM6 June 16, 2020 06:34
UdjinM6
UdjinM6 approved these changes Jun 16, 2020
View reviewed changes
Copy link

@UdjinM6 UdjinM6 left a comment

Choose a reason for hiding this comment

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

utACK

@UdjinM6 UdjinM6 merged commit f12c592 into dashpay:develop Jun 16, 2020
@PastaPastaPasta PastaPastaPasta deleted the backport-12254 branch August 12, 2021 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@UdjinM6 UdjinM6 UdjinM6 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

17

Development

Successfully merging this pull request may close these issues.

3 participants

@PastaPastaPasta @UdjinM6 @laanwj