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
See also: https://bugs.gentoo.org/715268
It could be moved to bitcoin_server (src/rpc/url or so)?
I suspect that will make problems for bitcoin-wallet tool
I can reproduce this on ubuntu with:
sudo apt remove libevent-dev
./configure --disable-util-cli --disable-util-wallet --with-daemon=no --with-gui=no --disable-tests --disable-bench
make
Expected behavior: successful build of bitcoin-tx
Actual behavior: compile error
CXX util/libbitcoin_util_a-url.o
util/url.cpp:7:10: fatal error: event2/http.h: No such file or directory
#include <event2/http.h>
^~~~~~~~~~~~~~~
Suggested fix:
diff --git a/configure.ac b/configure.ac
index 1f85dd3a99d..b43a29c7fb8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1255,7 +1255,7 @@ if test x$use_pkgconfig = xyes; then
BITCOIN_QT_CHECK([PKG_CHECK_MODULES([QR], [libqrencode], [have_qrencode=yes], [have_qrencode=no])])
fi
if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench != xnonononono; then
- PKG_CHECK_MODULES([EVENT], [libevent],, [AC_MSG_ERROR(libevent not found.)])
+ PKG_CHECK_MODULES([EVENT], [libevent], [use_libevent=yes], [AC_MSG_ERROR(libevent not found.)])
if test x$TARGET_OS != xwindows; then
PKG_CHECK_MODULES([EVENT_PTHREADS], [libevent_pthreads],, [AC_MSG_ERROR(libevent_pthreads not found.)])
fi
@@ -1275,7 +1275,7 @@ if test x$use_pkgconfig = xyes; then
else
if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench != xnonononono; then
- AC_CHECK_HEADER([event2/event.h],, AC_MSG_ERROR(libevent headers missing),)
+ AC_CHECK_HEADER([event2/event.h], [use_libevent=yes], AC_MSG_ERROR(libevent headers missing),)
AC_CHECK_LIB([event],[main],EVENT_LIBS=-levent,AC_MSG_ERROR(libevent missing))
if test x$TARGET_OS != xwindows; then
AC_CHECK_LIB([event_pthreads],[main],EVENT_PTHREADS_LIBS=-levent_pthreads,AC_MSG_ERROR(libevent_pthreads missing))
@@ -1529,6 +1529,7 @@ AM_CONDITIONAL([ENABLE_QT_TESTS],[test x$BUILD_TEST_QT = xyes])
AM_CONDITIONAL([ENABLE_BENCH],[test x$use_bench = xyes])
AM_CONDITIONAL([USE_QRCODE], [test x$use_qr = xyes])
AM_CONDITIONAL([USE_LCOV],[test x$use_lcov = xyes])
+AM_CONDITIONAL([USE_LIBEVENT],[test x$use_libevent = xyes])
AM_CONDITIONAL([GLIBC_BACK_COMPAT],[test x$use_glibc_compat = xyes])
AM_CONDITIONAL([HARDEN],[test x$use_hardening = xyes])
AM_CONDITIONAL([ENABLE_SSE42],[test x$enable_sse42 = xyes])
diff --git a/src/Makefile.am b/src/Makefile.am
index 8c927f330b8..c38a87a1e4b 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -523,9 +523,12 @@ libbitcoin_util_a_SOURCES = \
util/strencodings.cpp \
util/string.cpp \
util/time.cpp \
- util/url.cpp \
$(BITCOIN_CORE_H)
+if USE_LIBEVENT
+libbitcoin_util_a_SOURCES += util/url.cpp
+endif
+
if GLIBC_BACK_COMPAT
libbitcoin_util_a_SOURCES += compat/glibc_compat.cpp
AM_LDFLAGS += $(COMPAT_LDFLAGS)
wallet/rpcwallet.cpp calls urlDecode, which will presumably give a new linker error when building bitcoin-wallet without libevent?
wallet/rpcwallet.cppcallsurlDecode, which will presumably give a new linker error when buildingbitcoin-walletwithout 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.
Fix for bitcoin-wallet as well as bitcoin-tx:
diff --git a/configure.ac b/configure.ac
index 1f85dd3a99d..05ab0dd1e55 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1255,7 +1255,9 @@ if test x$use_pkgconfig = xyes; then
BITCOIN_QT_CHECK([PKG_CHECK_MODULES([QR], [libqrencode], [have_qrencode=yes], [have_qrencode=no])])
fi
if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench != xnonononono; then
- PKG_CHECK_MODULES([EVENT], [libevent],, [AC_MSG_ERROR(libevent not found.)])
+ PKG_CHECK_MODULES([EVENT], [libevent],
+ [AC_DEFINE(USE_LIBEVENT, 1, [Define this symbol to enable libevent dependency])],
+ [AC_MSG_ERROR(libevent not found.)])
if test x$TARGET_OS != xwindows; then
PKG_CHECK_MODULES([EVENT_PTHREADS], [libevent_pthreads],, [AC_MSG_ERROR(libevent_pthreads not found.)])
fi
@@ -1275,7 +1277,9 @@ if test x$use_pkgconfig = xyes; then
else
if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench != xnonononono; then
- AC_CHECK_HEADER([event2/event.h],, AC_MSG_ERROR(libevent headers missing),)
+ AC_CHECK_HEADER([event2/event.h],
+ [AC_DEFINE(USE_LIBEVENT, 1, [Define this symbol to enable libevent dependency])],
+ AC_MSG_ERROR(libevent headers missing),)
AC_CHECK_LIB([event],[main],EVENT_LIBS=-levent,AC_MSG_ERROR(libevent missing))
if test x$TARGET_OS != xwindows; then
AC_CHECK_LIB([event_pthreads],[main],EVENT_PTHREADS_LIBS=-levent_pthreads,AC_MSG_ERROR(libevent_pthreads missing))
diff --git a/src/util/url.cpp b/src/util/url.cpp
index e42d93bce8a..ae15815f0f7 100644
--- a/src/util/url.cpp
+++ b/src/util/url.cpp
@@ -4,6 +4,10 @@
#include <util/url.h>
+#include <config/bitcoin-config.h>
+
+#if USE_LIBEVENT
+
#include <event2/http.h>
#include <stdlib.h>
#include <string>
@@ -19,3 +23,11 @@ std::string urlDecode(const std::string &urlEncoded) {
}
return res;
}
+
+UrlDecodeFn* const g_url_decode = urlDecode;
+
+#else
+
+UrlDecodeFn* const g_url_decode = nullptr;
+
+#endif
diff --git a/src/util/url.h b/src/util/url.h
index e9ea2ab765a..4c5da762c31 100644
--- a/src/util/url.h
+++ b/src/util/url.h
@@ -7,6 +7,8 @@
#include <string>
-std::string urlDecode(const std::string &urlEncoded);
+using UrlDecodeFn = std::string(const std::string& url_encoded);
+UrlDecodeFn urlDecode;
+extern UrlDecodeFn* const g_url_decode;
#endif // BITCOIN_UTIL_URL_H
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index 5d34e592dbe..901d5ce9d4d 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -77,9 +77,9 @@ bool HaveKey(const SigningProvider& wallet, const CKey& key)
bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string& wallet_name)
{
- if (request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) {
+ if (g_url_decode && request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) {
// wallet endpoint was used
- wallet_name = urlDecode(request.URI.substr(WALLET_ENDPOINT_BASE.size()));
+ wallet_name = g_url_decode(request.URI.substr(WALLET_ENDPOINT_BASE.size()));
return true;
}
return false;
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?
IMO a build of
bitcoin-walletshouldn'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:
diff --git a/configure.ac b/configure.ac
index 1f85dd3a99d..b43a29c7fb8 100644
--- a/configure.ac
+++ b/configure.ac
@@ -1255,7 +1255,7 @@ if test x$use_pkgconfig = xyes; then
BITCOIN_QT_CHECK([PKG_CHECK_MODULES([QR], [libqrencode], [have_qrencode=yes], [have_qrencode=no])])
fi
if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench != xnonononono; then
- PKG_CHECK_MODULES([EVENT], [libevent],, [AC_MSG_ERROR(libevent not found.)])
+ PKG_CHECK_MODULES([EVENT], [libevent], [use_libevent=yes], [AC_MSG_ERROR(libevent not found.)])
if test x$TARGET_OS != xwindows; then
PKG_CHECK_MODULES([EVENT_PTHREADS], [libevent_pthreads],, [AC_MSG_ERROR(libevent_pthreads not found.)])
fi
@@ -1275,7 +1275,7 @@ if test x$use_pkgconfig = xyes; then
else
if test x$build_bitcoin_cli$build_bitcoind$bitcoin_enable_qt$use_tests$use_bench != xnonononono; then
- AC_CHECK_HEADER([event2/event.h],, AC_MSG_ERROR(libevent headers missing),)
+ AC_CHECK_HEADER([event2/event.h], [use_libevent=yes], AC_MSG_ERROR(libevent headers missing),)
AC_CHECK_LIB([event],[main],EVENT_LIBS=-levent,AC_MSG_ERROR(libevent missing))
if test x$TARGET_OS != xwindows; then
AC_CHECK_LIB([event_pthreads],[main],EVENT_PTHREADS_LIBS=-levent_pthreads,AC_MSG_ERROR(libevent_pthreads missing))
@@ -1529,6 +1529,7 @@ AM_CONDITIONAL([ENABLE_QT_TESTS],[test x$BUILD_TEST_QT = xyes])
AM_CONDITIONAL([ENABLE_BENCH],[test x$use_bench = xyes])
AM_CONDITIONAL([USE_QRCODE], [test x$use_qr = xyes])
AM_CONDITIONAL([USE_LCOV],[test x$use_lcov = xyes])
+AM_CONDITIONAL([USE_LIBEVENT],[test x$use_libevent = xyes])
AM_CONDITIONAL([GLIBC_BACK_COMPAT],[test x$use_glibc_compat = xyes])
AM_CONDITIONAL([HARDEN],[test x$use_hardening = xyes])
AM_CONDITIONAL([ENABLE_SSE42],[test x$enable_sse42 = xyes])
diff --git a/src/Makefile.am b/src/Makefile.am
index 8c927f330b8..c38a87a1e4b 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -523,9 +523,12 @@ libbitcoin_util_a_SOURCES = \
util/strencodings.cpp \
util/string.cpp \
util/time.cpp \
- util/url.cpp \
$(BITCOIN_CORE_H)
+if USE_LIBEVENT
+libbitcoin_util_a_SOURCES += util/url.cpp
+endif
+
if GLIBC_BACK_COMPAT
libbitcoin_util_a_SOURCES += compat/glibc_compat.cpp
AM_LDFLAGS += $(COMPAT_LDFLAGS)
diff --git a/src/bitcoin-cli.cpp b/src/bitcoin-cli.cpp
index 6982eaab61d..3c773fb64db 100644
--- a/src/bitcoin-cli.cpp
+++ b/src/bitcoin-cli.cpp
@@ -15,6 +15,7 @@
#include <util/strencodings.h>
#include <util/system.h>
#include <util/translation.h>
+#include <util/url.h>
#include <functional>
#include <memory>
@@ -29,6 +30,7 @@
#include <compat/stdin.h>
const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+UrlDecodeFn* const URL_DECODE = urlDecode;
static const char DEFAULT_RPCCONNECT[] = "127.0.0.1";
static const int DEFAULT_HTTP_CLIENT_TIMEOUT=900;
diff --git a/src/bitcoin-wallet.cpp b/src/bitcoin-wallet.cpp
index 7f1a4a114ba..76152a81d84 100644
--- a/src/bitcoin-wallet.cpp
+++ b/src/bitcoin-wallet.cpp
@@ -11,11 +11,13 @@
#include <logging.h>
#include <util/system.h>
#include <util/translation.h>
+#include <util/url.h>
#include <wallet/wallettool.h>
#include <functional>
const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+UrlDecodeFn* const URL_DECODE = nullptr;
static void SetupWalletToolArgs()
{
diff --git a/src/bitcoind.cpp b/src/bitcoind.cpp
index e284dce0d5d..f26eb45fcea 100644
--- a/src/bitcoind.cpp
+++ b/src/bitcoind.cpp
@@ -20,10 +20,12 @@
#include <util/system.h>
#include <util/threadnames.h>
#include <util/translation.h>
+#include <util/url.h>
#include <functional>
const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+UrlDecodeFn* const URL_DECODE = urlDecode;
static void WaitForShutdown(NodeContext& node)
{
diff --git a/src/qt/main.cpp b/src/qt/main.cpp
index 3dfd9e850ef..607cf9f9760 100644
--- a/src/qt/main.cpp
+++ b/src/qt/main.cpp
@@ -5,6 +5,7 @@
#include <qt/bitcoin.h>
#include <util/translation.h>
+#include <util/url.h>
#include <QCoreApplication>
@@ -15,5 +16,6 @@
extern const std::function<std::string(const char*)> G_TRANSLATION_FUN = [](const char* psz) {
return QCoreApplication::translate("bitcoin-core", psz).toStdString();
};
+UrlDecodeFn* const URL_DECODE = urlDecode;
int main(int argc, char* argv[]) { return GuiMain(argc, argv); }
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index d684b977876..b7f5dcfe43b 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -27,12 +27,14 @@
#include <util/string.h>
#include <util/time.h>
#include <util/translation.h>
+#include <util/url.h>
#include <validation.h>
#include <validationinterface.h>
#include <functional>
const std::function<std::string(const char*)> G_TRANSLATION_FUN = nullptr;
+UrlDecodeFn* const URL_DECODE = nullptr;
FastRandomContext g_insecure_rand_ctx;
/** Random context to get unique temp data dirs. Separate from g_insecure_rand_ctx, which can be seeded from a const env var */
diff --git a/src/util/url.h b/src/util/url.h
index e9ea2ab765a..be9f1c9e8a7 100644
--- a/src/util/url.h
+++ b/src/util/url.h
@@ -7,6 +7,8 @@
#include <string>
-std::string urlDecode(const std::string &urlEncoded);
+using UrlDecodeFn = std::string(const std::string& url_encoded);
+UrlDecodeFn urlDecode;
+extern UrlDecodeFn* const URL_DECODE;
#endif // BITCOIN_UTIL_URL_H
diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp
index 5d34e592dbe..9dd684a94c0 100644
--- a/src/wallet/rpcwallet.cpp
+++ b/src/wallet/rpcwallet.cpp
@@ -77,9 +77,9 @@ bool HaveKey(const SigningProvider& wallet, const CKey& key)
bool GetWalletNameFromJSONRPCRequest(const JSONRPCRequest& request, std::string& wallet_name)
{
- if (request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) {
+ if (URL_DECODE && request.URI.substr(0, WALLET_ENDPOINT_BASE.size()) == WALLET_ENDPOINT_BASE) {
// wallet endpoint was used
- wallet_name = urlDecode(request.URI.substr(WALLET_ENDPOINT_BASE.size()));
+ wallet_name = URL_DECODE(request.URI.substr(WALLET_ENDPOINT_BASE.size()));
return true;
}
return false;
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)
Milestone
0.20.0