Refactor and add tests for BlockFilter construction #14172

pull jimpo wants to merge 3 commits into bitcoin:master from jimpo:blockfilter-refactor changing 4 files +98 −48

jimpo commented at 9:19 PM on September 8, 2018: contributor

These commits have been split out of #14121 because they are fairly independent and that PR is very large.
DrahtBot commented at 11:29 PM on September 8, 2018: member



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



Conflicts

No conflicts as of last run.
fanquake added the label Refactoring on Sep 9, 2018

in src/blockfilter.h:109 in a3d9b90502 outdated

 104 |  
 105 | -    // Construct a new BlockFilter of the specified type from a block.
 106 | +    BlockFilter() = default;
 107 | +
 108 | +    //! Reconstruct a BlockFilter from parts.
 109 | +    BlockFilter(BlockFilterType filter_type, const uint256& block_hash,

practicalswift commented at 11:01 PM on September 9, 2018:

warning: function 'BlockFilter::BlockFilter' has a definition with different parameter names [readability-inconsistent-declaration-parameter-name]

jimpo force-pushed on Sep 10, 2018
blockfilter: Refactor GCS params into struct. 20b812993a
blockfilter: Additional constructors for BlockFilter. c30620983d
jimpo force-pushed on Nov 6, 2018

in src/blockfilter.h:57 in 20b812993a outdated

  53 | @@ -45,19 +54,16 @@ class GCSFilter
  54 |  public:
  55 |  
  56 |      /** Constructs an empty filter. */
  57 | -    GCSFilter(uint64_t siphash_k0 = 0, uint64_t siphash_k1 = 0, uint8_t P = 0, uint32_t M = 0);
  58 | +    explicit GCSFilter(const Params& params = Params());

jamesob commented at 4:37 AM on December 11, 2018:

Confused about why this is marked explicit...

jimpo commented at 4:48 AM on December 11, 2018:

Developer notes: "By default, declare single-argument constructors explicit"

jamesob approved
jamesob commented at 4:38 AM on December 11, 2018: member

utACK https://github.com/bitcoin/bitcoin/pull/14172/commits/c30620983d2e2c9aee6f52878ed14ba685e8683e

Uncontroversial, cute refactoring that only touches yet-unused BIP157&8 block filter machinery.

in src/blockfilter.cpp:255 in c30620983d outdated

 253 | +        params.m_siphash_k1 = m_block_hash.GetUint64(1);
 254 | +        params.m_P = BASIC_FILTER_P;
 255 | +        params.m_M = BASIC_FILTER_M;
 256 |          break;
 257 |  
 258 |      default:

MarcoFalke commented at 11:24 PM on December 11, 2018:

nit: Could remove the default case (which is a failure) to make it a compile time warning instead of a run time exception

    // no default case, so the compiler can warn about missing cases

Example:

blockfilter.cpp:247:13: warning: enumeration value 'BASIC' not handled in switch [-Wswitch]
    switch (m_filter_type) {
            ^

jimpo commented at 12:16 AM on December 12, 2018:

Oh, cool. Didn't realize C++ compilers did that analysis on switch statements.

MarcoFalke approved
MarcoFalke commented at 11:31 PM on December 11, 2018: member
utACK c30620983d

Looks mostly like refactoring with the following changes:
- m_encoded is initialized with one char 0 instead of an empty vector when constructing an empty filter.
- The default value of M changed from 0 to 1.

blockfilter: Remove default clause in switch statement.

Now the compiler will warn if not all enums are handled in the
switch.

e4ed8ce2c8

MarcoFalke merged this on Dec 22, 2018
MarcoFalke closed this on Dec 22, 2018
MarcoFalke referenced this in commit a4564b9b07 on Dec 22, 2018
jimpo deleted the branch on Dec 23, 2018
kittywhiskers referenced this in commit 0be7f3392a on Aug 2, 2021
kittywhiskers referenced this in commit a3363db678 on Aug 2, 2021
kittywhiskers referenced this in commit af4659ff2a on Aug 8, 2021
UdjinM6 referenced this in commit aef8fc04a9 on Aug 11, 2021
kittywhiskers referenced this in commit 99409f52e5 on Aug 12, 2021
UdjinM6 referenced this in commit 607bd4b6b5 on Aug 13, 2021
DrahtBot locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#14121 Index for BIP 157 block filters

Refactor and add tests for BlockFilter construction #14172

Conflicts