💬 theuni commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#discussion_r1946935711)
The meaning of `BITCOINKERNEL_STATIC` here is overloaded. Just to differentiate the use-cases, I'd prefer to have a 3rd define here:

`#elif defined(_WIN32) && !defined(BITCOIN_BUILD_INTERNAL) && !defined(BITCOINKERNEL_STATIC)`

`BITCOINKERNEL_STATIC`: I'm a user of the kernel building against a static kernel.
`BITCOIN_BUILD_INTERNAL`: I'm a component of Core who's not building against the kernel at all, I just happen to be seeing this header.

The former should stay as-is for bitcoin-cha
...

🤔 ryanofsky reviewed a pull request: "multiprocess: Add libmultiprocess git subtree"
(https://github.com/bitcoin/bitcoin/pull/31741#pullrequestreview-2602352724)
Rebased 99b7b857d3b8b9ce9bd7501e2480583369740c55 -> 4ca2462bf516846c5552378345e243bc819553a3 ([`pr/subtree.8`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.8) -> [`pr/subtree.9`](https://github.com/ryanofsky/bitcoin/commits/pr/subtree.9), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/subtree.8-rebase..pr/subtree.9)) to fix conflict with #31800. Also applied review suggestions from vasild, renamed Libmultiprocess_EXTERNAL_MPGEN cmake option to MPGEN_EXECUTABLE as suggested
...

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1946903444)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1942670069

Thanks, updated

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1946892750)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1942473099

Thanks, fixed

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1946893285)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1942473641

Agree, changed to suggested name

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1946902878)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1942612485

Thanks, applied both patches.

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1946901978)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1942477051

Agree that's a little clearer and more consistent with the documentation, so updated.

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1946892486)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1942468885

Thanks, fixed

👍 instagibbs approved a pull request: "TxOrphanage: account for size of orphans and count announcements"
(https://github.com/bitcoin/bitcoin/pull/31810#pullrequestreview-2602431093)
reACK 2a279acd89fa37fec94c66dde7408a3a9627f49a

💬 instagibbs commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1946936618)
nit: this could be moved completely outside the loops for the same checking power

💬 Sjors commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2643619428)
Getting a bunch of CI failings in #30975:

```
error: no member named 'format' in namespace 'std'
result.append(std::format("\\{:02x}", c));
```

e.g. macOS native: https://github.com/bitcoin/bitcoin/actions/runs/13205261715/job/36866826238?pr=30975

ARM, libbitcoinkernel, previous releases jobs:
```
17:54:12.396] [ 14%] Building CXX object CMakeFiles/mputil.dir/src/mp/util.cpp.o
[17:54:12.396] /ci_container_base/depends/work/build/arm-linux-gnueabihf/native_libmulti
...

💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1946946833)
Isn't it already out of the loop?

💬 glozow commented on pull request "TxOrphanage: account for size of orphans and count announcements":
(https://github.com/bitcoin/bitcoin/pull/31810#discussion_r1946951492)
Oh nvm, there are 2 loops, yes

💬 theuni commented on pull request "doc: swap CPPFLAGS for APPEND_CPPFLAGS":
(https://github.com/bitcoin/bitcoin/pull/31819#issuecomment-2643641231)
@purpleKarrot It's an artifact of a few things:
- The migration from autotools to CMake. autotools made CPPFLAGS a first-class citizen whereas CMake expects users to just pass them in as CXXFLAGS. `APPEND_CPPFLAGS` could probably be rolled into `APPEND_CXXFLAGS`, we just maintain the split because it can be a helpful distinction.
- In many cases `CXXFLAGS` or `CMAKE_CXX_FLAGS` or `CMAKE_CXX_FLAGS_BUILDTYPE` will work, but not always. CMake provides no way to _append_ CXXFLAGS, only to prepend
...

💬 ryanofsky commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1946960980)
re: https://github.com/bitcoin/bitcoin/pull/31741#discussion_r1942419081

> > So if I've been hacking in my local libmultiprocess dir (as I have today), and I do a depends build, it should fail and yell at me that the sources don't match the agreed-upon hash.
>
> Doesn't that apply to any source as well, not only libmultiprocess dir? Should depends build yell if the tree has local mods?

The PR has changed since this comment was written, but the idea is for the depends build to detect if
...

📝 Christewart opened a pull request: "tests: Add witness commitment if we have a witness transaction in `FullBlockTest.update_block()`"
(https://github.com/bitcoin/bitcoin/pull/31823)
This is useful for test cases where we want to test logic invalid blocks that contain witness transactions. If we don't add the witness commitment as per [BIP141](https://github.com/bitcoin/bips/blob/master/bip-0141.mediawiki#user-content-Commitment_structure), blocks will be rejected with the error [`Block mutated`](https://github.com/bitcoin/bitcoin/blob/fb0ada982a73687520c43b8fde480fa5d456f3e1/src/validation.cpp#L4180).

This change was needed in https://github.com/ajtowns/bitcoin/pull/13 w
...

✅ l0rinc closed an issue: "Proposal: Adopt simple SPDX-License-Identifier Across Bitcoin Codebase"
(https://github.com/bitcoin/bitcoin/issues/29445)

💬 l0rinc commented on pull request "fixing-link-Update dependencies.md":
(https://github.com/bitcoin/bitcoin/pull/31822#issuecomment-2643688638)
The old link successfully redirects me to the new one, what's the purpose of changing it?

💬 purpleKarrot commented on pull request "doc: swap CPPFLAGS for APPEND_CPPFLAGS":
(https://github.com/bitcoin/bitcoin/pull/31819#issuecomment-2643689227)
> CMake provides no way to append `CXXFLAGS`, ...

Not sure I understand. On the first run, CMake initializes `CMAKE_CXX_FLAGS` by prepending `CMAKE_CXX_FLAGS_INIT` with the environment variable `CXXFLAGS`. See: https://cmake.org/cmake/help/latest/variable/CMAKE_LANG_FLAGS_INIT.html

In other words, `CMAKE_CXX_FLAGS_INIT` is appended to `CXXFLAGS`. But you say it provides no way to append to `CXXFLAGS`?

💬 theuni commented on pull request "doc: swap CPPFLAGS for APPEND_CPPFLAGS":
(https://github.com/bitcoin/bitcoin/pull/31819#issuecomment-2643704920)
I didn't mean CXXFLAGS the env var, I meant CXXFLAGS the concept (as-in, what ultimately gets passed to the compiler). There are just some things that either we set internally or that CMake sets that are impossible to override without affecting something else (by using a None build type for example).`-Ox` is one of them.

So we provide these vars with guaranteed append semantics.