build, ci: Fix linking bitcoin-chainstate.exe to bitcoinkernel.dll on Windows #31158

1. 
   hebasto commented at 8:07 pm on October 25, 2024: member

   On the master branch @ 9a7206a34e3179d7280de98a818a13374178ee7b, linking bitcoin-chainstate.exe to bitcoinkernel.dll fails:

   0> cmake -B build --preset vs2022-static -DBUILD_UTIL_CHAINSTATE=ON -DBUILD_KERNEL_LIB=ON -DBUILD_SHARED_LIBS=ON
   1> cmake --build build --config Release -t bitcoin-chainstate
   2...
   3LINK : fatal error LNK1181: cannot open input file 'kernel\Release\bitcoinkernel.lib' [C:\Users\hebasto\source\repos\bitcoin\build-static\src\bitcoin-chainstate.vcxproj]
   

   This PR fixes this issue and enables the same scenario in the “Win64 native” CI job.

   The first commit is necessary; otherwise, linking fails and produces a different error:

   0bitcoin-chainstate.obj : error LNK2019: unresolved external symbol "class AnnotatedMixin<class std::recursive_mutex> cs_main" (?cs_main@@3V?$AnnotatedMixin@Vrecursive_mutex@std@@@@A) referenced in function main [D:\a\bitcoin\bitcoin\build\src\bitcoin-chainstate.vcxproj]
   1D:\a\bitcoin\bitcoin\build\src\Release\bitcoin-chainstate.exe : fatal error LNK1120: 1 unresolved externals [D:\a\bitcoin\bitcoin\build\src\bitcoin-chainstate.vcxproj]
   

2. hebasto added the label Windows on Oct 25, 2024
3. hebasto added the label Build system on Oct 25, 2024
4. 
   DrahtBot commented at 8:07 pm on October 25, 2024: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   Concept ACK	theuni

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #31221 (ci: Split out native fuzz jobs for macOS and windows (take 2) by dergoegge)

   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.

5. hebasto renamed this:
   build, ci: Fix linking `bitcoin-chainstate.exe` to `bticoinkernel.dll` on Windows
   build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows
   on Oct 25, 2024
6. hebasto force-pushed on Oct 25, 2024
7. in .github/workflows/ci.yml:185 in ca9554f037 outdated

   181@@ -182,7 +182,7 @@ jobs:
   182 
   183       - name: Generate build system
   184         run: |
   185-          cmake -B build --preset vs2022-static -DCMAKE_TOOLCHAIN_FILE="$env:VCPKG_INSTALLATION_ROOT\scripts\buildsystems\vcpkg.cmake" -DBUILD_GUI=ON -DWITH_BDB=ON -DWITH_MINIUPNPC=ON -DWITH_ZMQ=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DWERROR=ON
   186+          cmake -B build --preset vs2022-static -DCMAKE_TOOLCHAIN_FILE="$env:VCPKG_INSTALLATION_ROOT\scripts\buildsystems\vcpkg.cmake" -DBUILD_GUI=ON -DWITH_BDB=ON -DWITH_MINIUPNPC=ON -DWITH_ZMQ=ON -DBUILD_BENCH=ON -DBUILD_FUZZ_BINARY=ON -DBUILD_UTIL_CHAINSTATE=ON -DBUILD_KERNEL_LIB=ON -DBUILD_SHARED_LIBS=ON -DWERROR=ON
   

   ---

   ---

   

   TheCharlatan commented at 8:21 pm on October 25, 2024:

   Do you want to take this opportunity to also add a better description to the test name? Maybe something like: Win64 native, VS 2022, no depends, libbitcoinkernel?

   ---

   

   hebasto commented at 8:29 pm on October 25, 2024:

   Thanks! “libbitcoinkernel” taken; it seems awkward to mention “depends”, which is a non-Windows thing, on Windows :)

   ---

   

   TheCharlatan commented at 8:34 pm on October 25, 2024:

   Ah, I was thinking of mentioning it because of the macOS build, but I guess we can have native macOS and depends, so it makes sense there, but as you say, does not make sense for msvc.
8. hebasto force-pushed on Oct 25, 2024
9. in src/kernel/cs_main.h:20 in bc8efd1998 outdated

   16@@ -17,6 +17,6 @@
   17  * The transaction pool has a separate lock to allow reading from it and the
   18  * chainstate at the same time.
   19  */
   20-extern RecursiveMutex cs_main;
   21+inline RecursiveMutex cs_main;
   

   ---

   ---

   

   TheCharlatan commented at 1:43 pm on October 26, 2024:

   I checked if this could also be a problem for other extern symbols that are currently not used in bitcoin-chainstate, but still part of the kernel library. For example in src/validation.h we have extern const std::vector<std::string> CHECKLEVEL_DOC;, and it looks like using it is indeed problematic too if it were used externally: https://github.com/TheCharlatan/bitcoin/actions/runs/11531552497/job/32102377337#step:10:425. There are a bunch of other such cases, where I’m not entirely sure how to handle them. Could just ignore them for now, but it is a bit annoying that the library might contain symbols that you couldn’t just use.

   EDIT: I guess this is another argument for having a clear list of exports from the lib.

10. DrahtBot added the label Needs rebase on Oct 28, 2024
11. refactor: Inline `cs_main` definition 4a1accd1df
12. cmake, kernel: Enable `WINDOWS_EXPORT_ALL_SYMBOLS` property

    Temporarily enable `WINDOWS_EXPORT_ALL_SYMBOLS` in CMake to export all
    symbols on Windows until a dedicated header with `__declspec(dllexport)`
    directives is available.

    b8bbf7b6e9
13. ci: Build bitcoinkernel.dll and bitcoin-chainstate.exe on Windows fc3a37d6c3
14. hebasto force-pushed on Oct 28, 2024
15. 
    hebasto commented at 12:24 pm on October 28, 2024: member

    Rebased due to the conflict with the merged #31130.
16. DrahtBot removed the label Needs rebase on Oct 28, 2024
17. 
    theuni commented at 7:18 pm on October 28, 2024: member

    Concept ACK and code review ACK. This needs a more useful commit message though.

    I wasn’t familiar with this c++17 functionality, and at first glance it looked scary to me (as though we might get multiple cs_mains instantiated). According to cppreference though, this is a reasonable (if not more correct/modern) approach.

    This stackoverflow question/answers also covers our use-case exactly.

    Agree with @TheCharlatan that we should track down other cases as well.

18. 
    theuni commented at 7:26 pm on October 28, 2024: member

    Wait, I take that back. It seems like we could still get 2 separate cs_mains here: one in the lib and the other in the binary. ACK retracted while I investigate.
19. 
    fanquake commented at 10:05 am on October 29, 2024: member

    Generally I would prefer if we weren’t making source code changes to fix (currently edge-case) Windows linking errors; especially if there are more complete fixes we could be making (even if later-on).

    It’d be good if all commits explained the problem, and fix. i.e the first commit 4a1accd1df31cba778167b6a3ff9783ffedb46ab refactor: Inline cs_main definition has no explanation of what the problem/fix is, and doesn’t mention that the change is only needed to fix a build issue when using MSVC, other than “linking fails” in the PR description. I also don’t think this should be a labeled “refactor”.

Contributors
hebasto DrahtBot TheCharlatan theuni fanquake

Labels
Windows Build system