Add Benchmark to test input de-duplication worst case #14400

pull JeremyRubin wants to merge 1 commits into bitcoin:master from JeremyRubin:benchmark-reject-duplicate-inputs changing 2 files +101 −0
  1. JeremyRubin commented at 8:09 AM on October 5, 2018: contributor

    Because there are now 2PRs referencing this benchmark commit, we may as well add it independently as it is worth landing the benchmark even if neither patch is accepted.

    #14397 https://github.com/bitcoin/bitcoin/pull/14387

  2. in src/bench/duplicate_inputs.cpp:87 in 90b3b22417 outdated 
      86 | +
  87 | +    block.vtx.push_back(MakeTransactionRef(std::move(coinbaseTx)));
  88 | +    block.vtx.push_back(MakeTransactionRef(std::move(naughtyTx)));
  89 | +
  90 | +    block.hashMerkleRoot = BlockMerkleRoot(block);
  91 | +
    MarcoFalke commented at 8:15 AM on October 5, 2018:

    nit: there is still trailing whitespace, which prevents this from getting merged. Could do clang-format-diff.py?

    MarcoFalke commented at 5:05 AM on October 21, 2018:

    Are you still working on this?

    JeremyRubin commented at 8:33 AM on October 21, 2018:

    Ah I thought I did this -- I can take care of it soon.

    Yes, I would like to see this merged

    JeremyRubin commented at 8:33 AM on October 21, 2018:

    Ah I thought I did this -- I can take care of it soon.

    Yes, I would like to see this merged

  3. MarcoFalke added the label Tests on Oct 5, 2018
  4. jamesob commented at 8:16 AM on October 5, 2018: member

    Concept ACK

  5. JeremyRubin force-pushed on Oct 5, 2018
  6. DrahtBot commented at 10:41 AM on October 5, 2018: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #14397 (Faster duplicate input check in CheckTransaction (alternative to #14387) by sipa)

    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.

  7. DrahtBot cross-referenced this on Oct 5, 2018 from issue Faster duplicate input check in CheckTransaction (alternative to #14387) by sipa
  8. DrahtBot cross-referenced this on Oct 5, 2018 from issue Faster Input Deduplication Algorithm by JeremyRubin
  9. in src/bench/duplicate_inputs.cpp:50 in e1ed29fd90 outdated 
      45 | +        ::pcoinsTip.reset(new CCoinsViewCache(pcoinsdbview.get()));
  46 | +
  47 | +        thread_group.create_thread(boost::bind(&CScheduler::serviceQueue, &scheduler));
  48 | +        GetMainSignals().RegisterBackgroundSignalScheduler(scheduler);
  49 | +        LoadGenesisBlock(chainparams);
  50 | +        CValidationState state;
    practicalswift commented at 11:46 AM on October 5, 2018:

    Choose another name: state is already used in this scope :-)

    JeremyRubin commented at 2:17 AM on October 6, 2018:

    noting this is from existing code in bench/block_assemble.cpp

  10. in src/bench/duplicate_inputs.cpp:93 in e1ed29fd90 outdated 
      88 | +    block.vtx.push_back(MakeTransactionRef(std::move(naughtyTx)));
  89 | +
  90 | +    block.hashMerkleRoot = BlockMerkleRoot(block);
  91 | +
  92 | +    while (state.KeepRunning()) {
  93 | +        CValidationState state{};
    practicalswift commented at 11:47 AM on October 5, 2018:

    Same here: Choose another name :-)

  11. in src/bench/duplicate_inputs.cpp:66 in e1ed29fd90 outdated 
      61 | +    CBlockIndex* pindexPrev = ::chainActive.Tip();
  62 | +    assert(pindexPrev != nullptr);
  63 | +    block.nBits = GetNextWorkRequired(pindexPrev, &block, chainparams.GetConsensus());
  64 | +    block.nNonce = 0;
  65 | +    auto nHeight = pindexPrev->nHeight + 1;
  66 | +    block.nTime = ::chainActive.Tip()->GetMedianTimePast() + 1;
    practicalswift commented at 11:48 AM on October 5, 2018:

    Make this implicit conversion explicit :-)

    JeremyRubin commented at 2:22 AM on October 6, 2018:

    Please fix this everywhere in the codebase! This is done fairly frequently.

  12. in src/bench/duplicate_inputs.cpp:28 in e1ed29fd90 outdated 
      23 | +#include <vector>
  24 | +
  25 | +
  26 | +static void DuplicateInputs(benchmark::State& state)
  27 | +{
  28 | +    const std::vector<unsigned char> op_true{OP_TRUE};
    practicalswift commented at 11:49 AM on October 5, 2018:

    This is unused – please remove :-)

  13. in src/bench/duplicate_inputs.cpp:81 in e1ed29fd90 outdated 
      76 | +
  77 | +    naughtyTx.vout.resize(1);
  78 | +    naughtyTx.vout[0].nValue = 0;
  79 | +    naughtyTx.vout[0].scriptPubKey = SCRIPT_PUB;
  80 | +
  81 | +    int n_inputs = (((MAX_BLOCK_SERIALIZED_SIZE / WITNESS_SCALE_FACTOR) - (CTransaction(coinbaseTx).GetTotalSize() + CTransaction(naughtyTx).GetTotalSize())) / 41) - 100;
    practicalswift commented at 11:55 AM on October 5, 2018:

    Make the implicit conversions here explicit :-)

    Conversion from uint32_t to int64_t may theoretically alter value, so better make it explicit!

  14. JeremyRubin force-pushed on Oct 6, 2018
  15. MarcoFalke added the label Up for grabs on Nov 2, 2018
  16. JeremyRubin force-pushed on Nov 24, 2018
  17. JeremyRubin force-pushed on Nov 24, 2018
  18. JeremyRubin force-pushed on Nov 24, 2018
  19. JeremyRubin force-pushed on Nov 24, 2018
  20. JeremyRubin force-pushed on Nov 25, 2018
  21. JeremyRubin commented at 1:18 AM on November 25, 2018: contributor

    @MarcoFalke rebased, clang-formatted, etc.

    failure is unrelated to this PR now (one of the PBST tests)

  22. MarcoFalke removed the label Up for grabs on Nov 25, 2018
  23. in src/bench/duplicate_inputs.cpp:44 in bb5baab247 outdated 
      39 | +    {
  40 | +        ::pblocktree.reset(new CBlockTreeDB(1 << 20, true));
  41 | +        ::pcoinsdbview.reset(new CCoinsViewDB(1 << 23, true));
  42 | +        ::pcoinsTip.reset(new CCoinsViewCache(pcoinsdbview.get()));
  43 | +
  44 | +        thread_group.create_thread(boost::bind(&CScheduler::serviceQueue, &scheduler));
    MarcoFalke commented at 1:38 AM on November 25, 2018:

    µnit: Could probably use something like [&] { scheduler.serviceQueue(); } to avoid boost::bind. Or if you prefer bind, use std::bind.

            thread_group.create_thread([&] { scheduler.serviceQueue(); });
  24. MarcoFalke approved
  25. MarcoFalke commented at 1:38 AM on November 25, 2018: member

    Squash the fixup, so all commits compile?

  26. JeremyRubin force-pushed on Nov 25, 2018
  27. Add Benchmark to test input de-duplication worst case
    Fix nits

replace utiltime?
    e4eee7d09d
  28. JeremyRubin force-pushed on Nov 25, 2018
  29. JeremyRubin commented at 1:54 AM on November 25, 2018: contributor

    should be ready to merge now, pending integration tests.

    Thanks!

  30. jb55 commented at 8:20 PM on November 25, 2018: contributor

    utACK e4eee7d09df7dc85ae7a0c5e68e1d84f580cc9f7

  31. MarcoFalke referenced this in commit 327129f7a6 on Nov 25, 2018
  32. MarcoFalke merged this on Nov 25, 2018
  33. MarcoFalke closed this on Nov 25, 2018
  34. Munkybooty referenced this in commit df8134b1a7 on Jul 30, 2021
  35. bitcoin locked this on Sep 8, 2021
Contributors
JeremyRubinMarcoFalkejamesobDrahtBotpracticalswiftjb55
Labels
Tests
Linked (view graph)
#14387 Faster Input Deduplication Algorithm#14397 Faster duplicate input check in CheckTransaction (alternative to #14387)
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: 2026-05-20 06:54 UTC