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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31158.

Reviews

See the guideline for information on the review process.

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:

• #31961 (Require sqlite when building the wallet by Sjors)
• #31665 (build: Make config warnings fatal if -DWCONFIGURE_ERROR=ON by davidgumberg)
• #31507 ([POC] build: Use clang-cl to build on Windows natively by hebasto)
• #31176 (ci: Test cross-built Windows executables on Windows natively by hebasto)
• #30595 (kernel: Introduce initial C header API by TheCharlatan)
• #28710 (Remove the legacy wallet and BDB dependency 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.

Rebased due to the conflict with the merged #31130.

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.

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.

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 4a1accd 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".

Rebased. The commit messages have been amended.

@theuni

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.

That's an interesting observation. Could you elaborate on how you think the two separate cs_main instances might arise?

If there's a relevant section in the documentation or standard that you believe applies, pointing it out would be helpful for clarity.

Rebased. The commit messages have been amended.

@theuni

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.

That's an interesting observation. Could you elaborate on how you think the two separate cs_main instances might arise?

If there's a relevant section in the documentation or standard that you believe applies, pointing it out would be helpful for clarity.

clang 21 (git) has introduced a new warning for this!

And turns out, yeah, it is problematic for us with this PR:

/home/cory/dev/bitcoin/src/kernel/cs_main.h:20:23: warning: 'cs_main' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage [-Wunique-object-duplication] 20 | inline RecursiveMutex cs_main;

master currently does not have this issue.

also (unrelated to this PR, but of interest)

/home/cory/dev/bitcoin/src/support/lockedpool.h:224:31: warning: 'init_flag' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage [-Wunique-object-duplication] 224 | static std::once_flag init_flag;

/home/cory/dev/bitcoin/src/node/miner.h:175:42: warning: 'm_last_block_num_txs' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage [-Wunique-object-duplication] 175 | inline static std::optional<int64_t> m_last_block_num_txs{};

/home/cory/dev/bitcoin/src/node/miner.h:176:42: warning: 'm_last_block_weight' may be duplicated when built into a shared library: it is mutable, has hidden visibility, and external linkage [-Wunique-object-duplication] 176 | inline static std::optional<int64_t> m_last_block_weight{};

(maybe more)

This warning shows for bitcoind when REDUCE_EXPORTS is on, but not for the kernel because it currently overrides that.

I have verified that this does work though:

__attribute__ ((visibility ("default"))) inline RecursiveMutex cs_main;

But I'm not sure if that's what we want to be doing.

Need to think this through some more.

@theuni

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.

To clarify this discussion, which binary you are referring to—bitcoind or bitcoin-chainstate? I assume the latter, as the former uses the static kernel library.

From the docs

The module definition file will be passed to the linker causing all symbols to be exported from the .dll. For global data symbols, __declspec(dllimport) must still be used when compiling against the code in the .dll. All other function symbols will be automatically exported and imported by callers

See bold text. I suspect that's the real issue here, and that inlining is the opposite of what we want.

Mind sharing a link?

If the binary does not share dependencies with the shared library, it should be safe.

The binary has to find cs_main somewhere though.

Right. bitcoin-chainstate finds it in libbitcoinkernel.so.

Yes. But when inlining, it's free to generate its own as opposed to using the kernel's. Which is the opposite of what we want.

Mind sharing a link?

Sorry, I meant to link there. Updated.

Give me a few min, I'm testing over here :)

Please give https://github.com/theuni/bitcoin/commits/pr31158/ a shot. It's not meant to be robust, but it should illustrate the fix (using dllimport for bitcoin-chainstate.exe).

Please give https://github.com/theuni/bitcoin/commits/pr31158/ a shot. It's not meant to be robust, but it should illustrate the fix (using dllimport for bitcoin-chainstate.exe).

CI fails: https://github.com/theuni/bitcoin/actions/runs/13163570761/job/36738155052

Please give https://github.com/theuni/bitcoin/commits/pr31158/ a shot. It's not meant to be robust, but it should illustrate the fix (using dllimport for bitcoin-chainstate.exe).

CI fails: https://github.com/theuni/bitcoin/actions/runs/13163570761/job/36738155052

Yeah, the above is only enough to test bitcoin-chainstate.exe. In reality, the condition in the header would need to be something like:
#elif defined(MSC_VER) && !defined(STATIC_LIBBITCOINKERNEL) && !defined(BITCOIN_INTERNAL_BUILD), which would get defined for everything that's not bitcoin-chainstate.

Converting to a draft after today's offline discussion with @theuni.

@theuni

Please give https://github.com/theuni/bitcoin/commits/pr31158/ a shot. It's not meant to be robust, but it should illustrate the fix (using dllimport for bitcoin-chainstate.exe).

Thank you for your insight!

This PR has been reworked based on your branch. The inline variables are avoided.

Thank you @hebasto! We had a pretty long discussion about this. I think that hebasto's approach is probably fine and would never be an issue in the real world. I'll admit that I'm being overly paranoid and probably wrong about the guarantees that linkers provide...

But still, I'm not alone in my paranoia. For example, Google has banned non-constexpr inline header variables for Chromium out of similar concerns.

This reddit thread also contains a discussion from which illustrates some of my concerns and made me nervous.

For reference, the original proposal was:

-extern RecursiveMutex cs_main;
+inline RecursiveMutex cs_main;

And here's how the binaries changed in Linux:

extern (as-in current master):

 $ nm -C src/kernel/libbitcoinkernel.so | grep cs_main
00000000003b3028 D cs_main

 $ nm -C  src/bitcoin-chainstate | grep cs_main
                 U cs_main

header-inline (this PR's original proposal):

 $ nm -C src/kernel/libbitcoinkernel.so | grep cs_main
00000000003b3010 V cs_main
00000000003b35f0 V guard variable for cs_main

 $ nm -C  src/bitcoin-chainstate | grep cs_main
000000000000e010 V cs_main
000000000000e048 V guard variable for cs_main

Notice that the symbols, now weak, now exist in the shared lib as well as the binary. The runtime loader should pick one to use on both sides of the binary boundary, but technically the link/load behaviors here are platform-specific.

The root cause of the link failure being addressed here is that CMake's visibility machinery handles functions but not variables. So there are basically 3 potential solutions:

• A c++17-style inline variable as hebasto originally proposed here. I admit I'm being overly paranoid, but I'd rather not do that.
• Give cs_main hidden visiblility and add a get_csmain() getter function for bitcon-chainstate to use. That would work but would mean that bitcoin-chainstate's code would diverge from the rest of the codebase. (assuming we didn't do a global replacement, which would be a HUGE hammer)
• Add our own visibility header for exported variables, as hebasto has done here in the updated impl of this PR. This means no changes to how the symbols look, other than a strong guarantee that MSVC is importing as opposed to duplicating.

I pretty strongly prefer the 3rd as it's minimally invasive and leaves no potential for regressions in bitcoind/kernel.

Will review in detail.

Give cs_main hidden visiblility and add a get_csmain() getter function for bitcon-chainstate to use. That would work but would mean that bitcoin-chainstate's code would diverge from the rest of the codebase. (assuming we didn't do a global replacement, which would be a HUGE hammer)

(Probably out-of-scope for this pull, but there is already a getter for cs_main: chainman.GetMutex(), which can be used today, and can make it easier to turn cs_main from a global variable into a member field of chainman in the future. Though, that doesn't work for all places today and a global getter get_csmain would still be needed for now.)

Ran into this linker error when attempting to test #31161 on Windows yesterday.
Thanks for working on this, good to see linker warnings as errors enabled in CI!

The kernel library exposing cs_main instead of exposing functions that automatically take the correct locks makes me want to cry. But we have to deal with where we are.

Concept ACK.

🐙 This pull request conflicts with the target branch and needs rebase.

I'm not sure about the current approach of this change. I think we could go the way of manually exporting symbols in our c++ code, but then I would call into question the need for this project to ship a C header in the first place. Having two symbol-exporting headers, one for our internal code, and one for a potential future C header seems very confusing.

Also, as maflcko already said, we could just not directly use these globals from code that links to the shared library. If I understand the problem correctly, in the case of cs_main the required change is very small, and taking a similar approach for the other ones (if we ever run into them) seems reasonable.

I'm not sure about the current approach of this change.

I agree. There's no rush or need to do anything here, and doing something just to "make things green/compile" seems like the wrong choice, rather than making some good design decisions.

Converted to a draft for now.

⌛ There hasn't been much activity lately and the patch still needs rebase. What is the status here?

• Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
• Is it no longer relevant? ➡️ Please close.
• Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.