build: Do not include server symbols in wallet

MarcoFalke commented at 10:15 pm on June 19, 2020: member

This reverts a hacky workaround from commit b83cc0f, which only happens to work due to compiler optimizations. Then, it actually fixes the linker error.

The underlying problem is that the wallet includes symbols from the server (ui_interface), which usually results in linker failures. Though, in this specific case the linker failures have not been observed (unless -O0) because our compilers were smart enough to strip unused symbols.

Fix the underlying problem by creating a new header-only with the needed symbol and move ui_interface to node to clarify that this is part of libbitcoin_server.

MarcoFalke added the label Build system on Jun 19, 2020

MarcoFalke force-pushed on Jun 19, 2020

DrahtBot commented at 3:37 am on June 20, 2020: 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:

#19306 (refactor: Replace RecursiveMutex with Mutex in CTxMemPool by hebasto)
#19184 (Overhaul transaction request logic by sipa)
#19183 ([WIP DONOTMERGE] Replace boost with C++17 by MarcoFalke)
#19064 (refactor: Cleanup thread ctor calls by hebasto)
#18923 (wallet: Never schedule MaybeCompactWalletDB when -flushwallet is off by MarcoFalke)
#18077 (net: Add NAT-PMP port forwarding support by hebasto)
#17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)
#15937 (Add loadwallet and createwallet load_on_startup options by ryanofsky)
#15454 (Remove the automatic creation and loading of the default wallet by achow101)

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.

hebasto commented at 6:04 am on June 20, 2020: member

Approach ACK, tested with GCC 7.5.0 – works as expected.

The underlying problem is that the wallet includes symbols from the server (ui_interface), which usually results in linker failures.

Trying to get the complete understanding of linker failures. Could you elaborate or point out further reading the reasons of linker failures.

From my current understanding, if a symbol is exported, it could be linked to, no?

in src/util/ui_change_type.h:1 in fa2303bed0 outdated

0@@ -0,0 +1,15 @@
1+// Copyright (c) 2012-2020 The Bitcoin Core developers

hebasto commented at 6:27 am on June 20, 2020:

nit:

0// Copyright (c) 2020 The Bitcoin Core developers

MarcoFalke commented at 10:05 am on June 20, 2020:

The enum has been added in 2012

in src/qt/bitcoin.cpp:32 in faa4d7cb6e outdated

28@@ -29,12 +29,12 @@
29 
30 #include <interfaces/handler.h>
31 #include <interfaces/node.h>
32+#include <node/ui_interface.h>

hebasto commented at 6:55 am on June 20, 2020:

It seems qt/bitcoin.cpp does not use any symbols from node/ui_interface.h directly.

MarcoFalke commented at 10:12 am on June 20, 2020:

thx, removed

in src/qt/splashscreen.cpp:15 in faa4d7cb6e outdated

11@@ -12,9 +12,9 @@
12 #include <interfaces/handler.h>
13 #include <interfaces/node.h>
14 #include <interfaces/wallet.h>
15+#include <node/ui_interface.h>

hebasto commented at 7:03 am on June 20, 2020:

It seems qt/splashscreen.cpp does not use any symbols from node/ui_interface.h directly.

MarcoFalke commented at 10:12 am on June 20, 2020:

thx, removed

MarcoFalke commented at 10:12 am on June 20, 2020: member

Trying to get the complete understanding of linker failures. Could you elaborate or point out further reading the reasons of linker failures.

bitcoin-wallet tool: Drop libbitcoin_server.a dependency #15639
Move-only: Pull wallet code out of libbitcoin_server #15638

MarcoFalke force-pushed on Jun 20, 2020

MarcoFalke commented at 10:13 am on June 20, 2020: member

Removed unused includes as requested by @hebasto

hebasto approved

hebasto commented at 10:56 am on June 20, 2020: member

ACK fa30d36c5a94cd2f71185162b577e05ab9adf717, tested on Linux Mint 19.3 (x86_64) with both GCC 7.5.0 and Clang 6.0.0, using -O0 compiler option.

practicalswift commented at 9:03 am on June 22, 2020: contributor

Concept ACK

Thanks for fixing! This -O0 gotcha has annoyed me (mildly) for a while :)

laanwj commented at 12:38 pm on June 24, 2020: member

This looks reasonable. It’s important to be consistent about which libraries are allowed to use which symbols. Concept ACK.

DrahtBot added the label Needs rebase on Jun 24, 2020

MarcoFalke force-pushed on Jun 24, 2020

DrahtBot removed the label Needs rebase on Jun 24, 2020

Revert "Fix link error with --enable-debug"

This reverts commit b83cc0fc94df99f0334430e63e8c9fa6ae3790e1.

fa0f6c58c1

wallet: Do not include server symbols

ui_interface is in libbitcoin_server and can not be included in the
wallet because the wallet does not link with server symbols.

fac96e6450

qt: Remove unused includes fa72ca6a9d

MarcoFalke force-pushed on Jun 27, 2020

scripted-diff: Move ui_interface to the node lib

-BEGIN VERIFY SCRIPT-

 # Move files
 git mv src/ui_interface.h                                          src/node/ui_interface.h
 git mv src/ui_interface.cpp                                        src/node/ui_interface.cpp
 sed -i -e 's/BITCOIN_UI_INTERFACE_H/BITCOIN_NODE_UI_INTERFACE_H/g' src/node/ui_interface.h

 # Adjust includes and makefile
 sed -i -e 's|ui_interface|node/ui_interface|g' $(git grep -l ui_interface)

 # Sort includes
 git diff -U0 | clang-format-diff -p1 -i -v

-END VERIFY SCRIPT-

cccc2784a3

build: Sort Makefile.am after renaming file fa4695da4c

ci: Install fixed version of clang-format for linters faca73000f

MarcoFalke force-pushed on Jun 27, 2020

Sjors commented at 6:47 pm on June 29, 2020: member

ACK faca730

MarcoFalke commented at 12:10 pm on June 30, 2020: member

@hebasto Mind to re-ACK?

hebasto approved

hebasto commented at 3:58 pm on June 30, 2020: member

re-ACK faca73000fa8975c28f6be8be01957c1ae94ea62, since the previous review:

rebased
added “ci: Install fixed version of clang-format for linters” commit (related to #19095)

laanwj commented at 1:37 pm on July 1, 2020: member

ACK faca73000fa8975c28f6be8be01957c1ae94ea62

laanwj merged this on Jul 1, 2020

laanwj closed this on Jul 1, 2020

MarcoFalke deleted the branch on Jul 1, 2020

Fabcien referenced this in commit 978bd5b488 on Dec 11, 2020

DrahtBot locked this on Feb 15, 2022

build: Do not include server symbols in wallet #19331

Conflicts