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

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
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
MarcoFalke added the label Tests on Oct 5, 2018
jamesob commented at 8:16 AM on October 5, 2018: member

Concept ACK
JeremyRubin force-pushed on Oct 5, 2018
DrahtBot commented at 10:41 AM on October 5, 2018: 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:
- #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.

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

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 :-)

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.

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 :-)

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!

JeremyRubin force-pushed on Oct 6, 2018
MarcoFalke added the label Up for grabs on Nov 2, 2018
JeremyRubin force-pushed on Nov 24, 2018
JeremyRubin force-pushed on Nov 24, 2018
JeremyRubin force-pushed on Nov 24, 2018
JeremyRubin force-pushed on Nov 24, 2018
JeremyRubin force-pushed on Nov 25, 2018
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)
MarcoFalke removed the label Up for grabs on Nov 25, 2018

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(); });

MarcoFalke approved
MarcoFalke commented at 1:38 AM on November 25, 2018: member

Squash the fixup, so all commits compile?
JeremyRubin force-pushed on Nov 25, 2018
Add Benchmark to test input de-duplication worst case
Fix nits replace utiltime?
e4eee7d09d
JeremyRubin force-pushed on Nov 25, 2018
JeremyRubin commented at 1:54 AM on November 25, 2018: contributor

should be ready to merge now, pending integration tests.

Thanks!
jb55 commented at 8:20 PM on November 25, 2018: member

utACK e4eee7d09df7dc85ae7a0c5e68e1d84f580cc9f7
MarcoFalke referenced this in commit 327129f7a6 on Nov 25, 2018
MarcoFalke merged this on Nov 25, 2018
MarcoFalke closed this on Nov 25, 2018
Munkybooty referenced this in commit df8134b1a7 on Jul 30, 2021
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels

Linked (view graph)

#14387 Faster Input Deduplication Algorithm #14397 Faster duplicate input check in CheckTransaction (alternative to #14387)

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

Conflicts