test: fix scripts in blockfilter_basic_test #33162

pull UdjinM6 wants to merge 1 commits into bitcoin:master from UdjinM6:fix_scripts_blockfilter_basic_test changing 1 files +6 −6
  1. UdjinM6 commented at 10:07 am on August 9, 2025: contributor

    std::vector fill ctor is like this:

    0// Constructs a vector with `count` copies of elements with value `value`.
    1explicit vector( size_type count, const T& value = T(), const Allocator& alloc = Allocator() ); // (until C++11)
    2vector( size_type count, const T& value, const Allocator& alloc = Allocator() ); // (since C++11)(constexpr since C++20)
    

    https://en.cppreference.com/w/cpp/container/vector/vector.html

    i.e. std::vector<unsigned char>(0, 65) means a vector with 0 copies of 65 which feels wrong. I believe count and value were swapped in blockfilter_basic_test scripts.

  2. test: fix scripts in `blockfilter_basic_test` ca64b71ed5
  3. DrahtBot added the label Tests on Aug 9, 2025
  4. DrahtBot commented at 10:08 am on August 9, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33162.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK furszy, pablomartin4btc, janb84
    Concept ACK pinheadmz

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  5. pinheadmz commented at 11:23 am on August 9, 2025: member

    concept ACK

    I think you’re right! Found an example of another test with the values in the correct order:

    https://github.com/bitcoin/bitcoin/blob/2581258ec200efb173ea6449ad09b2e7f1cc02e0/src/bench/ccoins_caching.cpp#L38-L44

    It makes sense the block filter test isn’t actually checking scripts, just checking the compression and retrieval mechanism.

  6. furszy commented at 10:32 pm on August 9, 2025: member
    ACK ca64b71ed5ecbef66d4bb294dfcdff638157632c
  7. DrahtBot requested review from pinheadmz on Aug 9, 2025
  8. pablomartin4btc commented at 5:00 pm on August 10, 2025: member

    ACK ca64b71ed5ecbef66d4bb294dfcdff638157632c

    This change matches how std::vector constructor is being used in other tests (src/test/transaction_tests.cpp and src/bench/ccoins_caching.cpp - last one mentioned above).

  9. janb84 commented at 10:02 am on August 11, 2025: contributor

    ACK ca64b71ed5ecbef66d4bb294dfcdff638157632c

    Good find !

    PR fixes 8 year old error in the unit test while creating instances of std::vector

    The unit test does not validate the validity of the scripts therefor it does not fail. When moving a excluded (fixed) script to the included_script array the unit test will fail (tested)

    • code review ✅
    • history review ✅
    • build & tested ✅
    • purposely broken the test ✅
  10. glozow merged this on Aug 11, 2025
  11. glozow closed this on Aug 11, 2025


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-08-12 09:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me