test: Remove unused txmempool include from tests #26286

maflcko commented at 12:11 PM on October 10, 2022: member

Seems odd to include this heavy header in all tests despite it only being used in a few tests.

Can be reviewed with --color-moved=dimmed-zebra --ignore-all-space

fanquake added the label Tests on Oct 10, 2022

maflcko commented at 12:12 PM on October 10, 2022: member

Follow-up to #26250

fanquake commented at 12:22 PM on October 10, 2022: member

Concept ACK - looks like some previously transitively included includes will need to be added:

test/util_tests.cpp: In member function ‘void util_tests::util_seed_insecure_rand::test_method()’:
test/util_tests.cpp:1441:33: error: ‘sqrt’ was not declared in this scope
 1441 |         int err = 30*10000./mod*sqrt((1./mod*(1-1./mod))/10000.);
      |                                 ^~~~
make[2]: *** [Makefile:18499: test/test_bitcoin-util_tests.o] Error 1

hebasto commented at 12:27 PM on October 10, 2022: member

Concept ACK.

maflcko force-pushed on Oct 10, 2022

maflcko commented at 1:47 PM on October 10, 2022: member

I can reduce the fuzz compile time (user+kernel time) by 20% with a dirty txmempool.h. To reproduce:

( echo >> ./src/txmempool.h ) && /usr/bin/time --format '%U+%S (%e)' make -j $(nproc)

DrahtBot commented at 4:34 PM on October 10, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26153 (Reduce wasted pseudorandom bytes in ChaCha20 + various improvements by sipa)
#25977 (refactor: Replace std::optional<bilingual_str> with util::Result by ryanofsky)
#25974 (test, build: Separate read_json function into its own module by hebasto)
#25909 (wallet: coverage for receiving txes with same id but different witness data by furszy)
#25908 (p2p: remove adjusted time by fanquake)
#25361 (BIP324: Cipher suite by dhruv)
#25325 (Add pool based memory resource by martinus)
#25172 (refactor: use std:: prefix for std lib funcs by fanquake)
#25152 (refactor: Split util/system into exception, shell, and fs-specific files by Empact)
#23561 (BIP324: Handshake prerequisites by dhruv)
#10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)

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.

brunoerg commented at 8:50 PM on October 10, 2022: contributor

Concept ACK

DrahtBot cross-referenced this on Oct 11, 2022 from issue Reduce wasted pseudorandom bytes in ChaCha20 + various improvements by sipa

DrahtBot cross-referenced this on Oct 11, 2022 from issue refactor: Replace `std::optional<bilingual_str>` with `util::Result` by ryanofsky

DrahtBot cross-referenced this on Oct 11, 2022 from issue test, build: Separate `read_json` function into its own module by hebasto

maflcko commented at 9:14 AM on October 11, 2022: member

Does anyone know why msvc is unable to link this?

DrahtBot cross-referenced this on Oct 11, 2022 from issue wallet: coverage for receiving txes with same id but different witness data by furszy

DrahtBot cross-referenced this on Oct 11, 2022 from issue p2p: remove adjusted time by fanquake

DrahtBot cross-referenced this on Oct 11, 2022 from issue build: Add CMake-based build system by hebasto

DrahtBot cross-referenced this on Oct 11, 2022 from issue BIP324: Cipher suite by dhruv

DrahtBot cross-referenced this on Oct 11, 2022 from issue Add pool based memory resource by martinus

DrahtBot cross-referenced this on Oct 11, 2022 from issue refactor: use std:: prefix for std lib funcs by fanquake

DrahtBot cross-referenced this on Oct 11, 2022 from issue refactor: Split util/system into exception, shell, and fs-specific files by Empact

DrahtBot cross-referenced this on Oct 11, 2022 from issue BIP324: Enable v2 P2P encrypted transport by dhruv

DrahtBot cross-referenced this on Oct 12, 2022 from issue BIP324: Handshake prerequisites by dhruv

DrahtBot cross-referenced this on Oct 12, 2022 from issue BIP324: Add encrypted p2p transport {de}serializer by dhruv

fanquake commented at 2:05 AM on October 12, 2022: member

Does anyone know why msvc is unable to link this? @sipsorcery @hebasto any thoughts?

hebasto commented at 9:50 AM on October 12, 2022: member

Does anyone know why msvc is unable to link this?

--- a/build_msvc/test_bitcoin-qt/test_bitcoin-qt.vcxproj
+++ b/build_msvc/test_bitcoin-qt/test_bitcoin-qt.vcxproj
@@ -54,6 +54,9 @@
     <ProjectReference Include="..\libbitcoin_zmq\libbitcoin_zmq.vcxproj">
       <Project>{792d487f-f14c-49fc-a9de-3fc150f31c3f}</Project>
     </ProjectReference>
+    <ProjectReference Include="..\libtest_util\libtest_util.vcxproj">
+      <Project>{1e065f03-3566-47d0-8fa9-daa72b084e7d}</Project>
+    </ProjectReference>
     <ProjectReference Include="..\libleveldb\libleveldb.vcxproj">
       <Project>{18430fef-6b61-4c53-b396-718e02850f1b}</Project>
     </ProjectReference>

See https://cirrus-ci.com/task/6132886477733888.

DrahtBot cross-referenced this on Oct 12, 2022 from issue ci: Use all available CPUs for functional tests in "Win64 native" task by hebasto

DrahtBot cross-referenced this on Oct 12, 2022 from issue ci: Integrate `bitcoin-tidy` clang-tidy plugin by fanquake

DrahtBot added the label Needs rebase on Oct 14, 2022

test: Remove unused txmempool include from tests fad7f2239c

maflcko force-pushed on Oct 18, 2022

DrahtBot removed the label Needs rebase on Oct 18, 2022

in src/test/util/txmempool.h:28 in fad7f2239c outdated

  23 | +    unsigned int sigOpCost;
  24 | +    LockPoints lp;
  25 | +
  26 | +    TestMemPoolEntryHelper() :
  27 | +        nFee(0), nTime(0), nHeight(1),
  28 | +        spendsCoinbase(false), sigOpCost(4) { }

aureleoules commented at 1:26 PM on October 18, 2022:

if you retouch

    CAmount nFee{0};
    int64_t nTime{0};
    unsigned int nHeight{1};
    bool spendsCoinbase{false};
    unsigned int sigOpCost{4};
    LockPoints lp;

aureleoules commented at 2:55 PM on October 18, 2022: member

ACK fad7f2239c74a4db31b3023f2bcaf1f0852453f8

test: Use C++11 member initializers for TestMemPoolEntryHelper

Co-authored-by: Aurèle Oulès <aurele@oules.com>

1c48dae76f

aureleoules approved

aureleoules commented at 3:38 PM on October 18, 2022: member

reACK 1c48dae76fc808bf7154aca976e7303364c56509 Thanks for the co-author.

hebasto approved

hebasto commented at 5:48 PM on October 18, 2022: member

ACK 1c48dae76fc808bf7154aca976e7303364c56509, I have reviewed the code and it looks OK, I agree it can be merged.

w0xlt approved

w0xlt commented at 6:21 PM on October 18, 2022: contributor

ACK https://github.com/bitcoin/bitcoin/pull/26286/commits/1c48dae76fc808bf7154aca976e7303364c56509

maflcko merged this on Oct 19, 2022

maflcko closed this on Oct 19, 2022

maflcko deleted the branch on Oct 19, 2022

sidhujag referenced this in commit 1b048eca0a on Oct 23, 2022

ryanofsky cross-referenced this on Jan 20, 2023 from issue Add fee_est tool for debugging fee estimation code by ryanofsky

bitcoin locked this on Oct 19, 2023