bitcoin-tx (and probably others) fails to build without libevent

luke-jr commented at 3:19 pm on March 29, 2020: member

libevent seems to have invaded libbitcoin_common via url.cpp

url.cpp is used only from test/fuzz/string.cpp and wallet/rpcwallet.cpp

libevent should only be required for bitcoind, bitcoin-cli and (due only to the hack of passing wallet names) bitcoin-qt

luke-jr added the label Bug on Mar 29, 2020

luke-jr commented at 3:19 pm on March 29, 2020: member

See also: https://bugs.gentoo.org/715268

MarcoFalke commented at 3:33 pm on March 29, 2020: member

It could be moved to bitcoin_server (src/rpc/url or so)?

luke-jr commented at 0:24 am on March 30, 2020: member

I suspect that will make problems for bitcoin-wallet tool

ryanofsky commented at 9:34 am on March 30, 2020: member

I can reproduce this on ubuntu with:

0sudo apt remove libevent-dev
1./configure --disable-util-cli --disable-util-wallet --with-daemon=no --with-gui=no --disable-tests --disable-bench
2make

Expected behavior: successful build of bitcoin-tx

Actual behavior: compile error

0  CXX      util/libbitcoin_util_a-url.o
1util/url.cpp:7:10: fatal error: event2/http.h: No such file or directory
2 #include <event2/http.h>
3          ^~~~~~~~~~~~~~~

Suggested fix:

 0diff --git a/configure.ac b/configure.ac
 1index 1f85dd3a99d..b43a29c7fb8 100644
 2--- a/configure.ac
 3+++ b/configure.ac
 4@@ -1255,7 +1255,7 @@ if test x$use_pkgconfig = xyes; then
 5         BITCOIN_QT_CHECK([PKG_CHECK_MODULES([QR], [libqrencode], [have_qrencode=yes], [have_qrencode=no])])
 6       fi
 7       if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench != xnonononono; then
 8-        PKG_CHECK_MODULES([EVENT], [libevent],, [AC_MSG_ERROR(libevent not found.)])
 9+        PKG_CHECK_MODULES([EVENT], [libevent], [use_libevent=yes], [AC_MSG_ERROR(libevent not found.)])
10         if test x$TARGET_OS != xwindows; then
11           PKG_CHECK_MODULES([EVENT_PTHREADS], [libevent_pthreads],, [AC_MSG_ERROR(libevent_pthreads not found.)])
12         fi
13@@ -1275,7 +1275,7 @@ if test x$use_pkgconfig = xyes; then
14 else
15 
16   if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench != xnonononono; then
17-    AC_CHECK_HEADER([event2/event.h],, AC_MSG_ERROR(libevent headers missing),)
18+    AC_CHECK_HEADER([event2/event.h], [use_libevent=yes], AC_MSG_ERROR(libevent headers missing),)
19     AC_CHECK_LIB([event],[main],EVENT_LIBS=-levent,AC_MSG_ERROR(libevent missing))
20     if test x$TARGET_OS != xwindows; then
21       AC_CHECK_LIB([event_pthreads],[main],EVENT_PTHREADS_LIBS=-levent_pthreads,AC_MSG_ERROR(libevent_pthreads missing))
22@@ -1529,6 +1529,7 @@ AM_CONDITIONAL([ENABLE_QT_TESTS],[test x$BUILD_TEST_QT = xyes])
23 AM_CONDITIONAL([ENABLE_BENCH],[test x$use_bench = xyes])
24 AM_CONDITIONAL([USE_QRCODE], [test x$use_qr = xyes])
25 AM_CONDITIONAL([USE_LCOV],[test x$use_lcov = xyes])
26+AM_CONDITIONAL([USE_LIBEVENT],[test x$use_libevent = xyes])
27 AM_CONDITIONAL([GLIBC_BACK_COMPAT],[test x$use_glibc_compat = xyes])
28 AM_CONDITIONAL([HARDEN],[test x$use_hardening = xyes])
29 AM_CONDITIONAL([ENABLE_SSE42],[test x$enable_sse42 = xyes])
30diff --git a/src/Makefile.am b/src/Makefile.am
31index 8c927f330b8..c38a87a1e4b 100644
32--- a/src/Makefile.am
33+++ b/src/Makefile.am
34@@ -523,9 +523,12 @@ libbitcoin_util_a_SOURCES = \
35   util/strencodings.cpp \
36   util/string.cpp \
37   util/time.cpp \
38-  util/url.cpp \
39   $(BITCOIN_CORE_H)
40 
41+if USE_LIBEVENT
42+libbitcoin_util_a_SOURCES += util/url.cpp
43+endif
44+
45 if GLIBC_BACK_COMPAT
46 libbitcoin_util_a_SOURCES += compat/glibc_compat.cpp
47 AM_LDFLAGS += $(COMPAT_LDFLAGS)

luke-jr commented at 11:38 am on March 30, 2020: member

wallet/rpcwallet.cpp calls urlDecode, which will presumably give a new linker error when building bitcoin-wallet without libevent?

ryanofsky commented at 1:01 pm on March 30, 2020: member

wallet/rpcwallet.cpp calls urlDecode, which will presumably give a new linker error when building bitcoin-wallet without libevent?

Yes, I didn’t mention it, but for the future I do want bitcoin-wallet to be able to optionally depend on libevent so it can listen for RPC requests (https://github.com/ryanofsky/bitcoin/blob/ipc-export/doc/multiprocess.md#next-steps).

If it’s important for bitcoin-wallet to be built without libevent, then something needs to break the link dependency between bitcoin-wallet.cpp and urlDecode. Your PR #18469 does this by breaking the dependency at the RegisterWalletRPCCommands symbol, but I would do it at the GetWalletNameFromJSONRPCRequest symbol instead so bitcoin-wallet.cpp isn’t cut off entirely from rpcwallet.

Right now I am worried about code duplication in bitcoin-wallet, and concerned that it if gains more offline functionality it is going to turn into a third wallet interface duplicating functionality already implemented in the two current interfaces (RPC and GUI). This concern would go away if bitcoin-wallet could be a simple shim around rpcwallet methods that are tweaked to work offline (https://github.com/bitcoin/bitcoin/pull/15307#issuecomment-461480203, #13926 (comment))

Any case I believe simplest possible fix for gentoo bitcoin-tx problem is one from my comment #18465 (comment). Simplest fix for bitcoin-wallet link error without libevent is to just require libevent to build bitcoin-wallet. Second simplest fix would be to tweak GetWalletNameFromJSONRPCRequest to return false when urlDecode function isn’t available. Lots of ways to do this, you can make urlDecode a function pointer, use a HAVE_LIBEVENT macro, or make rule. I can post something later if it would be helpful.

ryanofsky commented at 1:38 pm on March 30, 2020: member

Fix for bitcoin-wallet as well as bitcoin-tx:

 0diff --git a/configure.ac b/configure.ac
 1index 1f85dd3a99d..05ab0dd1e55 100644
 2--- a/configure.ac
 3+++ b/configure.ac
 4@@ -1255,7 +1255,9 @@ if test x$use_pkgconfig = xyes; then
 5         BITCOIN_QT_CHECK([PKG_CHECK_MODULES([QR], [libqrencode], [have_qrencode=yes], [have_qrencode=no])])
 6       fi
 7       if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench != xnonononono; then
 8-        PKG_CHECK_MODULES([EVENT], [libevent],, [AC_MSG_ERROR(libevent not found.)])
 9+        PKG_CHECK_MODULES([EVENT], [libevent],
10+          [AC_DEFINE(USE_LIBEVENT, 1, [Define this symbol to enable libevent dependency])],
11+          [AC_MSG_ERROR(libevent not found.)])
12         if test x$TARGET_OS != xwindows; then
13           PKG_CHECK_MODULES([EVENT_PTHREADS], [libevent_pthreads],, [AC_MSG_ERROR(libevent_pthreads not found.)])
14         fi
15@@ -1275,7 +1277,9 @@ if test x$use_pkgconfig = xyes; then
16 else
17 
18   if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench != xnonononono; then
19-    AC_CHECK_HEADER([event2/event.h],, AC_MSG_ERROR(libevent headers missing),)
20+    AC_CHECK_HEADER([event2/event.h],
21+      [AC_DEFINE(USE_LIBEVENT, 1, [Define this symbol to enable libevent dependency])],
22+      AC_MSG_ERROR(libevent headers missing),)
23     AC_CHECK_LIB([event],[main],EVENT_LIBS=-levent,AC_MSG_ERROR(libevent missing))
24     if test x$TARGET_OS != xwindows; then
25       AC_CHECK_LIB([event_pthreads],[main],EVENT_PTHREADS_LIBS=-levent_pthreads,AC_MSG_ERROR(libevent_pthreads missing))
26diff --git a/src/util/url.cpp b/src/util/url.cpp
27index e42d93bce8a..ae15815f0f7 100644
28--- a/src/util/url.cpp
29+++ b/src/util/url.cpp
30@@ -4,6 +4,10 @@
31 
32 #include <util/url.h>
33 
34+#include <config/bitcoin-config.h>
35+
36+#if USE_LIBEVENT
37+
38 #include <event2/http.h>
39 #include <stdlib.h>
40 #include <string>
41@@ -19,3 +23,11 @@ std::string urlDecode(const std::string &urlEncoded) {
42     }
43     return res;
44 }
45+
46+UrlDecodeFn* const g_url_decode = urlDecode;
47+
48+#else
49+
50+UrlDecodeFn* const g_url_decode = nullptr;
51+
52+#endif
53diff --git a/src/util/url.h b/src/util/url.h
54index e9ea2ab765a..4c5da762c31 100644
55--- a/src/util/url.h
56+++ b/src/util/url.h
57@@ -7,6 +7,8 @@
58 
59 #include <string>
60 
61-std::string urlDecode(const std::string &urlEncoded);
62+using UrlDecodeFn = std::string(const std::string& url_encoded);
63+UrlDecodeFn urlDecode;
64+extern UrlDecodeFn* const g_url_decode;
65 
66 #endif // BITCOIN_UTIL_URL_H
67diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
68index 5d34e592dbe..901d5ce9d4d 100644
69--- a/src/wallet/rpcwallet.cpp
70+++ b/src/wallet/rpcwallet.cpp
71@@ -77,9 +77,9 @@ bool HaveKey(const SigningProvider& wallet, const CKey& key)
72 
73 bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string& wallet_name)
74 {
75-    if (request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) {
76+    if (g_url_decode && request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) {
77         // wallet endpoint was used
78-        wallet_name = urlDecode(request.URI.substr(WALLET_ENDPOINT_BASE.size()));
79+        wallet_name = g_url_decode(request.URI.substr(WALLET_ENDPOINT_BASE.size()));
80         return true;
81     }
82     return false;

luke-jr commented at 3:34 pm on March 30, 2020: member

IMO a build of bitcoin-wallet shouldn’t change based on whether or not it was built with bitcoind/bitcoin-qt at the same time?

ryanofsky commented at 5:31 pm on March 30, 2020: member

IMO a build of bitcoin-wallet shouldn’t change based on whether or not it was built with bitcoind/bitcoin-qt at the same time?

Good point, it seems like a nice property to avoid changing. You could use:

  0diff --git a/configure.ac b/configure.ac
  1index 1f85dd3a99d..b43a29c7fb8 100644
  2--- a/configure.ac
  3+++ b/configure.ac
  4@@ -1255,7 +1255,7 @@ if test x$use_pkgconfig = xyes; then
  5         BITCOIN_QT_CHECK([PKG_CHECK_MODULES([QR], [libqrencode], [have_qrencode=yes], [have_qrencode=no])])
  6       fi
  7       if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench != xnonononono; then
  8-        PKG_CHECK_MODULES([EVENT], [libevent],, [AC_MSG_ERROR(libevent not found.)])
  9+        PKG_CHECK_MODULES([EVENT], [libevent], [use_libevent=yes], [AC_MSG_ERROR(libevent not found.)])
 10         if test x$TARGET_OS != xwindows; then
 11           PKG_CHECK_MODULES([EVENT_PTHREADS], [libevent_pthreads],, [AC_MSG_ERROR(libevent_pthreads not found.)])
 12         fi
 13@@ -1275,7 +1275,7 @@ if test x$use_pkgconfig = xyes; then
 14 else
 15 
 16   if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench != xnonononono; then
 17-    AC_CHECK_HEADER([event2/event.h],, AC_MSG_ERROR(libevent headers missing),)
 18+    AC_CHECK_HEADER([event2/event.h], [use_libevent=yes], AC_MSG_ERROR(libevent headers missing),)
 19     AC_CHECK_LIB([event],[main],EVENT_LIBS=-levent,AC_MSG_ERROR(libevent missing))
 20     if test x$TARGET_OS != xwindows; then
 21       AC_CHECK_LIB([event_pthreads],[main],EVENT_PTHREADS_LIBS=-levent_pthreads,AC_MSG_ERROR(libevent_pthreads missing))
 22@@ -1529,6 +1529,7 @@ AM_CONDITIONAL([ENABLE_QT_TESTS],[test x$BUILD_TEST_QT = xyes])
 23 AM_CONDITIONAL([ENABLE_BENCH],[test x$use_bench = xyes])
 24 AM_CONDITIONAL([USE_QRCODE], [test x$use_qr = xyes])
 25 AM_CONDITIONAL([USE_LCOV],[test x$use_lcov = xyes])
 26+AM_CONDITIONAL([USE_LIBEVENT],[test x$use_libevent = xyes])
 27 AM_CONDITIONAL([GLIBC_BACK_COMPAT],[test x$use_glibc_compat = xyes])
 28 AM_CONDITIONAL([HARDEN],[test x$use_hardening = xyes])
 29 AM_CONDITIONAL([ENABLE_SSE42],[test x$enable_sse42 = xyes])
 30diff --git a/src/Makefile.am b/src/Makefile.am
 31index 8c927f330b8..c38a87a1e4b 100644
 32--- a/src/Makefile.am
 33+++ b/src/Makefile.am
 34@@ -523,9 +523,12 @@ libbitcoin_util_a_SOURCES = \
 35   util/strencodings.cpp \
 36   util/string.cpp \
 37   util/time.cpp \
 38-  util/url.cpp \
 39   $(BITCOIN_CORE_H)
 40 
 41+if USE_LIBEVENT
 42+libbitcoin_util_a_SOURCES += util/url.cpp
 43+endif
 44+
 45 if GLIBC_BACK_COMPAT
 46 libbitcoin_util_a_SOURCES += compat/glibc_compat.cpp
 47 AM_LDFLAGS += $(COMPAT_LDFLAGS)
 48diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
 49index 6982eaab61d..3c773fb64db 100644
 50--- a/src/bitcoin-cli.cpp
 51+++ b/src/bitcoin-cli.cpp
 52@@ -15,6 +15,7 @@
 53 #include <util/strencodings.h>
 54 #include <util/system.h>
 55 #include <util/translation.h>
 56+#include <util/url.h>
 57 
 58 #include <functional>
 59 #include <memory>
 60@@ -29,6 +30,7 @@
 61 #include <compat/stdin.h>
 62 
 63 const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
 64+UrlDecodeFn* const URL_DECODE = urlDecode;
 65 
 66 static const char DEFAULT_RPCCONNECT[] = "127.0.0.1";
 67 static const int DEFAULT_HTTP_CLIENT_TIMEOUT=900;
 68diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp
 69index 7f1a4a114ba..76152a81d84 100644
 70--- a/src/bitcoin-wallet.cpp
 71+++ b/src/bitcoin-wallet.cpp
 72@@ -11,11 +11,13 @@
 73 #include <logging.h>
 74 #include <util/system.h>
 75 #include <util/translation.h>
 76+#include <util/url.h>
 77 #include <wallet/wallettool.h>
 78 
 79 #include <functional>
 80 
 81 const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
 82+UrlDecodeFn* const URL_DECODE = nullptr;
 83 
 84 static void SetupWalletToolArgs()
 85 {
 86diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
 87index e284dce0d5d..f26eb45fcea 100644
 88--- a/src/bitcoind.cpp
 89+++ b/src/bitcoind.cpp
 90@@ -20,10 +20,12 @@
 91 #include <util/system.h>
 92 #include <util/threadnames.h>
 93 #include <util/translation.h>
 94+#include <util/url.h>
 95 
 96 #include <functional>
 97 
 98 const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
 99+UrlDecodeFn* const URL_DECODE = urlDecode;
100 
101 static void WaitForShutdown(NodeContext& node)
102 {
103diff --git a/src/qt/main.cpp b/src/qt/main.cpp
104index 3dfd9e850ef..607cf9f9760 100644
105--- a/src/qt/main.cpp
106+++ b/src/qt/main.cpp
107@@ -5,6 +5,7 @@
108 #include <qt/bitcoin.h>
109 
110 #include <util/translation.h>
111+#include <util/url.h>
112 
113 #include <QCoreApplication>
114 
115@@ -15,5 +16,6 @@
116 extern const std::function<std::string(const char*)> G_TRANSLATION_FUN = [](const char* psz) {
117     return QCoreApplication::translate("bitcoin-core", psz).toStdString();
118 };
119+UrlDecodeFn* const URL_DECODE = urlDecode;
120 
121 int main(int argc, char* argv[]) { return GuiMain(argc, argv); }
122diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
123index d684b977876..b7f5dcfe43b 100644
124--- a/src/test/util/setup_common.cpp
125+++ b/src/test/util/setup_common.cpp
126@@ -27,12 +27,14 @@
127 #include <util/string.h>
128 #include <util/time.h>
129 #include <util/translation.h>
130+#include <util/url.h>
131 #include <validation.h>
132 #include <validationinterface.h>
133 
134 #include <functional>
135 
136 const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
137+UrlDecodeFn* const URL_DECODE = nullptr;
138 
139 FastRandomContext g_insecure_rand_ctx;
140 /** Random context to get unique temp data dirs. Separate from g_insecure_rand_ctx, which can be seeded from a const env var */
141diff --git a/src/util/url.h b/src/util/url.h
142index e9ea2ab765a..be9f1c9e8a7 100644
143--- a/src/util/url.h
144+++ b/src/util/url.h
145@@ -7,6 +7,8 @@
146 
147 #include <string>
148 
149-std::string urlDecode(const std::string &urlEncoded);
150+using UrlDecodeFn = std::string(const std::string& url_encoded);
151+UrlDecodeFn urlDecode;
152+extern UrlDecodeFn* const URL_DECODE;
153 
154 #endif // BITCOIN_UTIL_URL_H
155diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
156index 5d34e592dbe..9dd684a94c0 100644
157--- a/src/wallet/rpcwallet.cpp
158+++ b/src/wallet/rpcwallet.cpp
159@@ -77,9 +77,9 @@ bool HaveKey(const SigningProvider& wallet, const CKey& key)
160 
161 bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string& wallet_name)
162 {
163-    if (request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) {
164+    if (URL_DECODE && request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) {
165         // wallet endpoint was used
166-        wallet_name = urlDecode(request.URI.substr(WALLET_ENDPOINT_BASE.size()));
167+        wallet_name = URL_DECODE(request.URI.substr(WALLET_ENDPOINT_BASE.size()));
168         return true;
169     }
170     return false;

ryanofsky referenced this in commit e432309c64 on Apr 2, 2020

laanwj added this to the milestone 0.20.0 on Apr 2, 2020

ryanofsky referenced this in commit 0660119ac3 on Apr 3, 2020

ryanofsky commented at 7:18 pm on April 3, 2020: member

Would recommend using #18504 to resolve this issue. #18504 fixes the reported bitcoin-tx compile error by adding a conditional build rule for src/url.cpp, and fixes the other issues raised by luke-jr above: avoiding bitcoin-wallet link error, building identical bitcoin-wallet binary when libevent is or isn’t found.

Alternate approach in draft PR #18469 is probably not an ideal fix for this issue. It complicates chain client interface in a way that does not seem to make sense (though could be my own ignorance), and adds new static library that doesn’t seem to line up with source code organization (https://github.com/bitcoin/bitcoin/issues/15732), and might make it difficult to add more offline wallet functionality to bitcoin wallet (https://github.com/bitcoin/bitcoin/issues/18465#issuecomment-605984805)

MarcoFalke closed this on Apr 10, 2020

MarcoFalke referenced this in commit 4eb1eeb02c on Apr 10, 2020

sidhujag referenced this in commit 32d6ead51a on Apr 13, 2020

janus referenced this in commit 951e97777e on Nov 5, 2020

PhotoshiNakamoto referenced this in commit f7bcb3d341 on Dec 11, 2021

DrahtBot locked this on Feb 15, 2022

bitcoin-tx (and probably others) fails to build without libevent #18465