🤔 enirox001 reviewed a pull request: "wallet: Remove isminetypes"
(https://github.com/bitcoin/bitcoin/pull/32523#pullrequestreview-3136742267)
re-ACK be776a1
(https://github.com/bitcoin/bitcoin/pull/32523#pullrequestreview-3136742267)
re-ACK be776a1
💬 luke-jr commented on pull request "depends: remove xinerama extension from libxcb":
(https://github.com/bitcoin/bitcoin/pull/33217#issuecomment-3206473104)
> (only used for windowing over multiple screens support?)
Deprecated by and incompatible with XRandR which was introduced in ~2001.
(https://github.com/bitcoin/bitcoin/pull/33217#issuecomment-3206473104)
> (only used for windowing over multiple screens support?)
Deprecated by and incompatible with XRandR which was introduced in ~2001.
💬 ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288193651)
In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
As mentioned https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2283314830, would be curious to know if force-system-libcapnp=True works for people, since it seems wasteful to download and build a dependency that we already know must be installed (or the test would be skipped).
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288193651)
In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
As mentioned https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2283314830, would be curious to know if force-system-libcapnp=True works for people, since it seems wasteful to download and build a dependency that we already know must be installed (or the test would be skipped).
💬 ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288166616)
In commit "test: add is_ipc_enabled() and skip_if_no_ipc() functions" (eae32d1a9b5c62707de6afd9832d40de859dd42d)
Not important, but might be better to rename enabled to compiled here to be more similar to surrounding functions which check ENABLE_XXX but are called is_xxx_compiled. (I think your PR actually did this originally but I didn't notice the pattern it was following)
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288166616)
In commit "test: add is_ipc_enabled() and skip_if_no_ipc() functions" (eae32d1a9b5c62707de6afd9832d40de859dd42d)
Not important, but might be better to rename enabled to compiled here to be more similar to surrounding functions which check ENABLE_XXX but are called is_xxx_compiled. (I think your PR actually did this originally but I didn't notice the pattern it was following)
💬 ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288233217)
In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
I might describe this as # Create a remote thread on the server for the IPC calls to be executed in
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288233217)
In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
I might describe this as # Create a remote thread on the server for the IPC calls to be executed in
👍 ryanofsky approved a pull request: "Add functional test for IPC interface"
(https://github.com/bitcoin/bitcoin/pull/33201#pullrequestreview-3136603264)
Code review ACK 088dc2c486af0ad6803d919d8114ab29d3cd652f. Looks great! Left some comments that you should feel free to ignore. I am shocked and amazed about how much more friendly the python async API is compared to the c++ API, but I guess coroutines can bring similar benefits to c++. It is also nice how this tests covers different scenarios with waitNext()
(https://github.com/bitcoin/bitcoin/pull/33201#pullrequestreview-3136603264)
Code review ACK 088dc2c486af0ad6803d919d8114ab29d3cd652f. Looks great! Left some comments that you should feel free to ignore. I am shocked and amazed about how much more friendly the python async API is compared to the c++ API, but I guess coroutines can bring similar benefits to c++. It is also nice how this tests covers different scenarios with waitNext()
💬 ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288203120)
In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
Not important, but for easier review and to avoid conflicts in the CI files, it may make sense to split this commit into two commits: one which adds the interface_ipc.py test and allows running it on systems with pycapnp installed, and another commit before or after that installs pycapnp in CI.
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288203120)
In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
Not important, but for easier review and to avoid conflicts in the CI files, it may make sense to split this commit into two commits: one which adds the interface_ipc.py test and allows running it on systems with pycapnp installed, and another commit before or after that installs pycapnp in CI.
💬 ryanofsky commented on pull request "Add functional test for IPC interface":
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288227488)
In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
This is probably fine in practice, but it does seem possible for this to fail if the `capnp` binary isn't on the path because `shutil.which` will return None. It might be better to write this as:
````python
if capnp_bin := shutil.which("capnp"):
# Add the system cap'nproto path so include/capnp/c++.capnp can be found.
capnp_dir = Path(shutil.which("capnp")).parent.parent / "include"
...
(https://github.com/bitcoin/bitcoin/pull/33201#discussion_r2288227488)
In commit "ci: add functional test for IPC interface" (088dc2c486af0ad6803d919d8114ab29d3cd652f)
This is probably fine in practice, but it does seem possible for this to fail if the `capnp` binary isn't on the path because `shutil.which` will return None. It might be better to write this as:
````python
if capnp_bin := shutil.which("capnp"):
# Add the system cap'nproto path so include/capnp/c++.capnp can be found.
capnp_dir = Path(shutil.which("capnp")).parent.parent / "include"
...
💬 Crypt-iQ commented on pull request "[29.x] Backport logging ratelimiting":
(https://github.com/bitcoin/bitcoin/pull/33225#issuecomment-3206481506)
Post-merge ACK 0022e25333a8eabf79c0341f94cf06db36e32f4f
Only change is release-notes. Tested that rate-limiting works as expected.
(https://github.com/bitcoin/bitcoin/pull/33225#issuecomment-3206481506)
Post-merge ACK 0022e25333a8eabf79c0341f94cf06db36e32f4f
Only change is release-notes. Tested that rate-limiting works as expected.
💬 fanquake commented on pull request "[29.x] Backport logging ratelimiting":
(https://github.com/bitcoin/bitcoin/pull/33225#discussion_r2288289304)
Ok, we can adjust this pre-final.
(https://github.com/bitcoin/bitcoin/pull/33225#discussion_r2288289304)
Ok, we can adjust this pre-final.
💬 glozow commented on pull request "doc: truc packages allow sub min feerate transactions":
(https://github.com/bitcoin/bitcoin/pull/33220#issuecomment-3206570179)
LGTM
(https://github.com/bitcoin/bitcoin/pull/33220#issuecomment-3206570179)
LGTM
💬 maflcko commented on issue "RPC sendmany first (dummy, empty string) argument is not optional":
(https://github.com/bitcoin/bitcoin/issues/33182#issuecomment-3206575653)
> ...how do you make a first argument optional, anyway?
You can use named args, by using the `-named=1` option:
```
$ ./bld-cmake/bin/bitcoin-cli -named=1 sendmany amounts='{"hi":1}'
```
This shows that the arg is correctly marked as optional.
What am I missing?
(https://github.com/bitcoin/bitcoin/issues/33182#issuecomment-3206575653)
> ...how do you make a first argument optional, anyway?
You can use named args, by using the `-named=1` option:
```
$ ./bld-cmake/bin/bitcoin-cli -named=1 sendmany amounts='{"hi":1}'
```
This shows that the arg is correctly marked as optional.
What am I missing?
💬 1ma commented on pull request "[29.x] 33106 backport and final changes for rc2":
(https://github.com/bitcoin/bitcoin/pull/33226#issuecomment-3206599181)
What is the criteria for backporting PRs to point releases? Sub 1 s/vB standardization is certainly not a bugfix.
(https://github.com/bitcoin/bitcoin/pull/33226#issuecomment-3206599181)
What is the criteria for backporting PRs to point releases? Sub 1 s/vB standardization is certainly not a bugfix.
✅ pinheadmz closed an issue: "RPC sendmany first (dummy, empty string) argument is not optional"
(https://github.com/bitcoin/bitcoin/issues/33182)
(https://github.com/bitcoin/bitcoin/issues/33182)
💬 pinheadmz commented on issue "RPC sendmany first (dummy, empty string) argument is not optional":
(https://github.com/bitcoin/bitcoin/issues/33182#issuecomment-3206632314)
ah using `-named=1` is my answer thanks.
(https://github.com/bitcoin/bitcoin/issues/33182#issuecomment-3206632314)
ah using `-named=1` is my answer thanks.
💬 janb84 commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3206654682)
> Please take a step back and just think like a user. v29 shipped with `bitcoind`. They'll now see `bitcoin`, `bitcoind`, and `bitcoin-node`. Even if their behavior doesn't have to change at all, anyone could be forgiven for being confused by that.
>
@theuni could it be that the context you are describing is the `<<buid-dir>>/bin` directory and not the result of `cmake --install ` result that splits binaries in `${CMAKE_PREFIX_PATH}/bin` and `${CMAKE_PREFIX_PATH}/libexec` (as per #31679)?
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3206654682)
> Please take a step back and just think like a user. v29 shipped with `bitcoind`. They'll now see `bitcoin`, `bitcoind`, and `bitcoin-node`. Even if their behavior doesn't have to change at all, anyone could be forgiven for being confused by that.
>
@theuni could it be that the context you are describing is the `<<buid-dir>>/bin` directory and not the result of `cmake --install ` result that splits binaries in `${CMAKE_PREFIX_PATH}/bin` and `${CMAKE_PREFIX_PATH}/libexec` (as per #31679)?
...
⚠️ fanquake opened an issue: "tracing: issue running `contrib/tracing/log_utxos.bt`"
(https://github.com/bitcoin/bitcoin/issues/33227)
Using master (9cf7b3d90c76fd299e156d9615e42b938ee884b2) and running `bpftrace contrib/tracing/log_utxos.bt -v`:
```bash
# Linux fedora-32gb-hel1-3 6.17.0-0.rc1.250812g53e760d89498.18.fc44.aarch64 #1 SMP PREEMPT_DYNAMIC Tue Aug 12 19:11:45 UTC 2025 aarch64 GNU/Linux
bpftrace contrib/tracing/log_utxos.bt -v
Attaching 4 probes...
ERROR: Error loading BPF program for BEGIN_1.
Kernel error log:
processed 103 insns (limit 1000000) max_states_per_insn 0 total_states 2 peak_states 2 mark_read 1
ERRO
...
(https://github.com/bitcoin/bitcoin/issues/33227)
Using master (9cf7b3d90c76fd299e156d9615e42b938ee884b2) and running `bpftrace contrib/tracing/log_utxos.bt -v`:
```bash
# Linux fedora-32gb-hel1-3 6.17.0-0.rc1.250812g53e760d89498.18.fc44.aarch64 #1 SMP PREEMPT_DYNAMIC Tue Aug 12 19:11:45 UTC 2025 aarch64 GNU/Linux
bpftrace contrib/tracing/log_utxos.bt -v
Attaching 4 probes...
ERROR: Error loading BPF program for BEGIN_1.
Kernel error log:
processed 103 insns (limit 1000000) max_states_per_insn 0 total_states 2 peak_states 2 mark_read 1
ERRO
...
💬 theuni commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3206701996)
> @theuni Thanks for stating your preference. If I am following, you would like to:
It seems (as usual :p) that we're talking past each other.
Above, I described what would've been, in my opinion, the ideal features for v30. That would've been a bitcoind with build-time opt-out for IPC. That build-time opt-out would've added `-ipcbind` to bitcoind. It would've been default-on, so that all devs would've encountered IPC as part of their build process, or turned it off manually. By default, t
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-3206701996)
> @theuni Thanks for stating your preference. If I am following, you would like to:
It seems (as usual :p) that we're talking past each other.
Above, I described what would've been, in my opinion, the ideal features for v30. That would've been a bitcoind with build-time opt-out for IPC. That build-time opt-out would've added `-ipcbind` to bitcoind. It would've been default-on, so that all devs would've encountered IPC as part of their build process, or turned it off manually. By default, t
...
🤔 maflcko reviewed a pull request: "test: use local `CBlockIndex` in block read hash mismatch check"
(https://github.com/bitcoin/bitcoin/pull/33154#pullrequestreview-3136954211)
lgtm ACK cb173b8e939d63821a966d0d76b299f20742c619
(https://github.com/bitcoin/bitcoin/pull/33154#pullrequestreview-3136954211)
lgtm ACK cb173b8e939d63821a966d0d76b299f20742c619
💬 maflcko commented on pull request "test: use local `CBlockIndex` in block read hash mismatch check":
(https://github.com/bitcoin/bitcoin/pull/33154#discussion_r2288403957)
nit: Could use the `m_node.chainman->GetMutex()` alias for new code.
(https://github.com/bitcoin/bitcoin/pull/33154#discussion_r2288403957)
nit: Could use the `m_node.chainman->GetMutex()` alias for new code.