💬 ryanofsky commented on pull request "build: Enable ENABLE_IPC option by default":
(https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3197824813)
Updated 732c134bfc1208377f449e5956e01dd8b78d73d9 -> 5a34156ab65ac7460c26141bb29651477e000d19 ([`pr/ipc-default.3`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-default.3) -> [`pr/ipc-default.4`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-default.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-default.3..pr/ipc-default.4) applying fanquake's suggestion https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3197036114 to update valgrind jobs, and sjors suggest
...
(https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3197824813)
Updated 732c134bfc1208377f449e5956e01dd8b78d73d9 -> 5a34156ab65ac7460c26141bb29651477e000d19 ([`pr/ipc-default.3`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-default.3) -> [`pr/ipc-default.4`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-default.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-default.3..pr/ipc-default.4) applying fanquake's suggestion https://github.com/bitcoin/bitcoin/pull/33190#issuecomment-3197036114 to update valgrind jobs, and sjors suggest
...
💬 janb84 commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2283046276)
This branch / patch works for me on Nix on macOS. Hint, do not forget to `import shutil` when applying the patch below.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2283046276)
This branch / patch works for me on Nix on macOS. Hint, do not forget to `import shutil` when applying the patch below.
💬 sipa commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2283047618)
@ryanofsky Thanks! Will address later today.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2283047618)
@ryanofsky Thanks! Will address later today.
💬 brunoerg commented on pull request "fuzz: enhance wallet_fees by mocking mempool stuff":
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2283103326)
> Is the 256-byte value used at all? If not, seems easier to just hardcode a value (like 0 or 1 or 2), or use a deterministic random comtext?
Good catch. I don't think it's used at all, we can simply use a hardcoded value instead of spending part of the buffer with it.
(https://github.com/bitcoin/bitcoin/pull/33210#discussion_r2283103326)
> Is the 256-byte value used at all? If not, seems easier to just hardcode a value (like 0 or 1 or 2), or use a deterministic random comtext?
Good catch. I don't think it's used at all, we can simply use a hardcoded value instead of spending part of the buffer with it.
💬 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
...
(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.
(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.
(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.
(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
...
(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
...
(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)
(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
...
(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
...
(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
...
(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.
(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?
(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)
(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
(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.
(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.
(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.