bench: Add missed `ECCVerifyHandle` instance #26179

hebasto commented at 7:20 PM on September 24, 2022: member

To clearly observe the lack of an ECCVerifyHandle instance,

apply the following diff:

--- a/src/Makefile.bench.include
+++ b/src/Makefile.bench.include
@@ -19,11 +19,9 @@ bench_bench_bitcoin_SOURCES = \
   bench/bench.h \
   bench/bench_bitcoin.cpp \
   bench/block_assemble.cpp \
-  bench/ccoins_caching.cpp \
   bench/chacha20.cpp \
   bench/chacha_poly_aead.cpp \
   bench/checkblock.cpp \
-  bench/checkqueue.cpp \
   bench/crypto_hash.cpp \
   bench/data.cpp \
   bench/data.h \
@@ -46,8 +44,7 @@ bench_bench_bitcoin_SOURCES = \
   bench/rpc_blockchain.cpp \
   bench/rpc_mempool.cpp \
   bench/strencodings.cpp \
-  bench/util_time.cpp \
-  bench/verify_script.cpp
+  bench/util_time.cpp
 
 nodist_bench_bench_bitcoin_SOURCES = $(GENERATED_BENCH_FILES)

then

$ ./autogen
$ ./configure
$ make clean
$ make

then

$ ./src/bench/bench_bitcoin -filter=ExpandDescriptor
bench_bitcoin: pubkey.cpp:296: bool CPubKey::IsFullyValid() const: Assertion `secp256k1_context_verify && "secp256k1_context_verify must be initialized to use CPubKey."' failed.
Aborted (core dumped)

DrahtBot added the label Tests on Sep 24, 2022

amovfx commented at 5:07 PM on September 25, 2022: none

Hey man, I ran this and here are the results. | ns/op | op/s | err% | total | benchmark |--------------------:|--------------------:|--------:|----------:|:---------- | 9,980,750.00 | 100.19 | 0.0% | 0.11 | ExpandDescriptor

Hope this helps.

bench: Add missed `ECCVerifyHandle` instance f09d47b263

hebasto force-pushed on Sep 26, 2022

in src/bench/descriptors.cpp:7 in f09d47b263

   3 | @@ -4,6 +4,7 @@
   4 |  
   5 |  #include <bench/bench.h>
   6 |  #include <key.h>
   7 | +#include <pubkey.h>

yancyribbens commented at 10:40 AM on September 26, 2022:

ECC_Start() is defined in key.h so this include is unneeded.

hebasto commented at 9:06 PM on September 27, 2022:

https://github.com/bitcoin/bitcoin/blob/f09d47b263459e27faf5416287255eb3aed369a2/src/pubkey.h#L335

yancyribbens commented at 11:10 PM on September 27, 2022:

Thanks for pointing that out, that must be why the additional include. Although, it doesn't appear to be needed. Also, if you look at verify script, it uses EccVerifyHandle but doesn't include pubkey.h. It seems like maybe it's being included by another include as well. If we add pubkey.h here, do you think it should also be added to verify_script? I find the c/c++ include system really confusing and hard to trace where dependencies are being pulled in from.

hebasto commented at 7:39 AM on September 28, 2022:

Although, it doesn't appear to be needed.

In this project we follow our Developer Notes:

Every .cpp and .h file should #include every header file it directly uses classes, functions or other definitions from, even if those headers are already included indirectly through other headers.

If we add pubkey.h here, do you think it should also be added to verify_script?

Not in this PR, to keep it focused.

I find the c/c++ include system really confusing and hard to trace where dependencies are being pulled in from.

To make it easier we started to use IWYU. See #24831 and other related PRs.

yancyribbens commented at 2:08 PM on September 28, 2022:

In this project we follow our Developer Notes:

When I last read the developer-notes I must have overlooked this. I agree and think it's a good practice to help with dependency confusion. It would be better if the compiler enforced this behavior however because I've found more than a few places where this practice isn't implemented. Perhaps I'll put in a PR for fixing other includes but I'm not sure if it's important enough to get others to review.

To make it easier we started to use IWYU. See #24831 and other related PRs.

Thanks, I'll look into it.

yancyribbens commented at 10:43 AM on September 26, 2022: contributor

No need to include pukey.h. Concept Ack.

DrahtBot commented at 3:29 PM on September 26, 2022: contributor

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

Conflicts

No conflicts as of last run.

yancyribbens commented at 2:09 PM on September 28, 2022: contributor

Concept Ack

maflcko commented at 11:44 AM on September 29, 2022: member

Easier diff to reproduce:

diff --git a/src/bench/verify_script.cpp b/src/bench/verify_script.cpp
index 8e4708f260..79fc447a33 100644
--- a/src/bench/verify_script.cpp
+++ b/src/bench/verify_script.cpp
@@ -61,16 +61,6 @@ static void VerifyScriptBench(benchmark::Bench& bench)
         assert(err == SCRIPT_ERR_OK);
         assert(success);
 
-#if defined(HAVE_CONSENSUS_LIB)
-        CDataStream stream(SER_NETWORK, PROTOCOL_VERSION);
-        stream << txSpend;
-        int csuccess = bitcoinconsensus_verify_script_with_amount(
-            txCredit.vout[0].scriptPubKey.data(),
-            txCredit.vout[0].scriptPubKey.size(),
-            txCredit.vout[0].nValue,
-            (const unsigned char*)stream.data(), stream.size(), 0, flags, nullptr);
-        assert(csuccess == 1);
-#endif
     });
     ECC_Stop();
 }

hebasto commented at 6:21 PM on October 18, 2022: member

Friendly ping @Empact as a benchmark author, and @achow101.

w0xlt approved

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

Interesting. I hadn't noticed that secp256k1_context is a global variable.

ACK https://github.com/bitcoin/bitcoin/pull/26179/commits/f09d47b263459e27faf5416287255eb3aed369a2

achow101 commented at 10:11 PM on October 18, 2022: member

ACK f09d47b263459e27faf5416287255eb3aed369a2

maflcko merged this on Oct 19, 2022

maflcko closed this on Oct 19, 2022

hebasto deleted the branch on Oct 19, 2022

sidhujag referenced this in commit 8bd7427b2c on Oct 23, 2022

bitcoin locked this on Oct 19, 2023