💬 fanquake commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#issuecomment-3197934560)
https://github.com/bitcoin/bitcoin/actions/runs/17047602183/job/48327342413?pr=33201#step:8:3379:
```bash
stderr:
Traceback (most recent call last):
File "/Users/runner/work/bitcoin/bitcoin/ci/scratch/build-aarch64-apple-darwin23.6.0/test/functional/interface_ipc.py", line 161, in <module>
IPCInterfaceTest(__file__).main()
~~~~~~~~~~~~~~~~^^^^^^^^^^
File "/Users/runner/work/bitcoin/bitcoin/test/functional/test_framework/test_framework.py", line 186, in __init__
self.set_t
...

💬 brunoerg commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2283133265)
Done.

💬 josibake commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2283137376)
Sorry, didn’t mean to imply this breaks things if present, rather that adding this line works because it allows accessing the bundled version. This is why I recommended in a separate comment that our docs prompt the user to always install from source using the bundled version when installing pycapnp.

Longer term, I am confident we can drop all of this from the functional test in favour of having CMake pipe through the correct info to the config file.

📝 Crypt-iQ opened a pull request: "test: modify logging_filesize_rate_limit params"
(https://github.com/bitcoin/bitcoin/pull/33211)
Change `time_window` from 20s to 1h so `Reset` is not accidentally called if the test takes a while.

Change `num_lines` from 1024 to 10 since `LogRateLimiter` is parameterized and does not require logging 1MiB of data.

💬 ryanofsky commented on pull request "build: Enable ENABLE_IPC option by default":
(https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3198025822)
re: https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3196869655

> I think https://github.com/bitcoin/bitcoin/pull/31802 should be seen as a successor to this PR, regardless of which release it goes into.

@sipa if you are saying you see this PR as a prerequisite to #31802, that seems ok, but I would be interested to know why.

As far as I can tell, there haven't been any other features in bitcoin core enabled by default in non-depends builds but disabled by default by default in
...

💬 fanquake commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3198034390)
Note that when this is merged, `oss-fuzz` will need fixing, to either opt-out of multiprocess, or configure depends to use the pre-built Clang for native package compilation (as Ubuntu 20.04 ships with GCC 9). This isn't an issue currently, as we don't build native packages (NO_QT=1), but with multiprocess on by default, we'll build `native_capnp`:
```bash
/src/bitcoin-core/depends/work/download/native_capnp-1.2.0/capnproto-cxx-1.2.0.tar.gz.temp: OK
Extracting native_capnp...
/src/bitcoin-co
...

🚀 fanquake merged a pull request: "ipc: Handle unclean shutdowns better"
(https://github.com/bitcoin/bitcoin/pull/32345)

💬 TheCharlatan commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3198178581)
Updated 7bcb122e6e55339f25238a44433cc5aadc4526f1 -> 3a34e006918490eb968c7b3312693e8a27fa3fde ([kernelApi_58](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_58) -> [kernelApi_59](https://github.com/TheCharlatan/bitcoin/tree/kernelApi_59), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelApi_58..kernelApi_59))

* Implemented owned delegates as lined out in @purpleKarrot's [blog post](https://purplekarrot.github.io/btck/design/delegates.html) - though for now without error
...

💬 achow101 commented on pull request "build: Enable ENABLE_IPC option by default":
(https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3198197406)
Thinking on this further, I think I agree that there isn't a good reason to be making this change. When building from source, we already don't enable several optional features, including the GUI, that we ship in the release binaries. Since the switch to cmake, having the dependencies installed doesn't enable those features automatically, they still need to be enabled explicitly during configuration. Developers should probably be using the dev-mode preset, or similar, which enables a bunch of the
...

💬 ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2283314830)
Thanks, when I saw this I wasn't aware what a ["bundled"](https://github.com/capnproto/pycapnp/blob/a89eb0dee6c67049bf2f6b875e51dd3f71a6abd1/setup.py#L146-L151) capnproto was but agree it makes sense to use it if the python extension includes it.

I guess I'd be curious what would happen if the docs were changed to use `force-system-libcapnp=True` instead of `force-bundled-libcapnp=True`? It seems like since the c++ code already requires capnproto to be built, maybe it's wasteful for the instr
...

🤔 achow101 reviewed a pull request: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802#pullrequestreview-3129705803)
I disagree with defaulting `ENABLE_IPC=ON` since that makes capnproto almost a required dependency. Instead of it being an optional feature that you can turn on, it now becomes and optional feature that you can turn off, and if you just do a build with the defaults, capnproto will need to be installed.

I think it's ok to have the default source build not match the release builds or the depends builds since we already do that by excluding optional features like the GUI, or ZMQ, etc.

💬 achow101 commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#discussion_r2283319242)
In 16bce9ac4cd02b2c8308f370bf39d70f9421e69b "build: depends makes libmultiprocess by default"

For the other packages that depends may build, there is a similar if-else block right about this, with a comment that says something about needing to use `MATCHES` rather than `STREQUAL`. That seems like something that should be done here?

👋 Crypt-iQ's pull request is ready for review: "test: modify logging_filesize_rate_limit params"
(https://github.com/bitcoin/bitcoin/pull/33211)

💬 Crypt-iQ commented on pull request "test: modify logging_filesize_rate_limit params":
(https://github.com/bitcoin/bitcoin/pull/33211#issuecomment-3198335063)
cc @stickies-v

🤔 achow101 reviewed a pull request: "index: Fix coinstats overflow"
(https://github.com/bitcoin/bitcoin/pull/30469#pullrequestreview-3129757943)
053ac55eb569be6b49c5249a7d8c0eeaa149a18b seems like it could cause incorrect total statistics if somehow we ever have the situation where we are appending a block that doesn't build on the index's current block hash.

This behavior does match what we do today with the running total members, but it feels

Although that sounds like it should really be a fatal error.

💬 achow101 commented on pull request "index: Fix coinstats overflow":
(https://github.com/bitcoin/bitcoin/pull/30469#discussion_r2283353869)
In b8c5bf71a4be4756740fb7b8a6a580153b007475 "index, refactor: DRY coinbase check"

This seems like a bug fix that should be in it's own commit.

Should this be decrementing `m_total_unspendable_amount` and `m_total_unspendables_bip30` to reverse what the similar code in `CustomAppend` does? This doesn't really matter once those members are removed.

💬 Crypt-iQ commented on issue "ci: failure in `logging_tests`":
(https://github.com/bitcoin/bitcoin/issues/33195#issuecomment-3198343891)
I've opened https://github.com/bitcoin/bitcoin/pull/33211 to fix this. It changes the time window to 1h which should be more than enough. Also, it reduces `num_lines` quite a bit as suggested by @stickies-v above.

> It seems the 835th iteration got corrupted (scroll down a bit)

Interesting... now I'm curious why it was corrupted? But I'll let that be for now.

💬 achow101 commented on pull request "headerssync: Preempt unrealistic unit test behavior":
(https://github.com/bitcoin/bitcoin/pull/32579#discussion_r2277678459)
In 232485d2486f08221203224212d5451a47eb4fc8 "refactor(test): Break up headers_sync_chainwork_tests"

Our preferred pattern for test cases within unit tests is to define them with `BOOST_AUTO_TEST_CASE` rather than functions like these.

💬 ryanofsky commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3198354810)
> I disagree with defaulting `ENABLE_IPC=ON` since that makes capnproto almost a required dependency. Instead of it being an optional feature that you can turn on, it now becomes and optional feature that you can turn off, and if you just do a build with the defaults, capnproto will need to be installed.

Yes, with the current version of this PR, ENABLE_IPC acts like ENABLE_WALLET: it's an optional feature that is default-on in both depends and non-depends builds.

I didn't realize that othe
...

📝 mzumsande opened a pull request: "index: Don't commit state in BaseIndex::Rewind "
(https://github.com/bitcoin/bitcoin/pull/33212)
The committed state of an index should never be ahead of the flushed chainstate.
Otherwise, in the case of an unclean shutdown, the blocks necessary to revert
from the prematurely committed state are not be available, which would corrupt the coinstatsindex in particular.
Instead, the index state will be committed with the next ChainStateFlushed notification.

Fixes #33208