🤔 furszy reviewed a pull request: "interfaces: enable cancelling running `waitNext` calls"
(https://github.com/bitcoin/bitcoin/pull/33676#pullrequestreview-3384985314)
Code ACK dcb56fd4cb59e6857c110dd87019459989dc1ec3

nit:
is there any way you could verify that there is someone waiting before setting `interrupt_wait`? It seems fragile to be able to interrupt a procedure that hasn't started.

💬 torkelrogstad commented on issue "Failure to bind to ZMQ addresses is swallowed in debug logs":
(https://github.com/bitcoin/bitcoin/issues/33715#issuecomment-3452842787)
> > 2025-10-27T17:12:31Z [zmq] Error: Failed to bind address, msg: Address already in use
>
> This was in your log ... ?

Yes, when I enabled debug. Before enabling debug I was very confused as to why I wasn't seeing anything with `getzmqnotifications`.

> Do you think bitcoin should fail to initialize if it cant bind? (same as rpc?)

Yes, ideally. As I said above, I think failing hard is best, and then a clear non-debug log entry is the second best solution.

💬 polespinasa commented on pull request "refactor: rename `fees.{h,cpp}` to `fees/block_policy_estimator.{h,cpp}`":
(https://github.com/bitcoin/bitcoin/pull/33218#issuecomment-3452870905)
re ACK 1a7fb5eeeef3575c1e7c27915c9b98695191299d

Just small rebase and small nits

🤔 glozow reviewed a pull request: "test: descriptor: cover invalid multi/multi_a cases"
(https://github.com/bitcoin/bitcoin/pull/32504#pullrequestreview-3385170617)
ACK 58e55b17e632dbd4425dd64825b087f242ac4b7b

Read BIP 383, 387 and the relevant code, mutation tested to see this is new coverage.

🚀 glozow merged a pull request: "test: descriptor: cover invalid multi/multi_a cases"
(https://github.com/bitcoin/bitcoin/pull/32504)

💬 theStack commented on pull request "test: add functional test for `TestShell` (matching doc example)":
(https://github.com/bitcoin/bitcoin/pull/33546#discussion_r2466884577)
Thanks for the reviews! Leaving these suggestions about either introducing new `TEST_FRAMEWORK_...` constants or extending existing ones as topic for a different PR, since I'm not sure what is the right approach here. E.g. one could even argue in the other direction and suggest to just remove the `TEST_FRAMEWORK_UNIT_TESTS` constant, as it's only used once in the list and doesn't seem to serve any deeper purpose, unless I'm missing something.

> I think this counts as unit test? Could just go
...

💬 l0rinc commented on pull request "random: clarify where the environ extern is needed":
(https://github.com/bitcoin/bitcoin/pull/33714#discussion_r2466889461)
If it's only needed on Mac, wouldn't:
```suggestion
#ifdef __APPLE__
```
be better here?

💬 furszy commented on pull request "interfaces: enable cancelling running `waitNext` calls":
(https://github.com/bitcoin/bitcoin/pull/33676#issuecomment-3453042451)
> nit:
> is there any way you could verify that there is someone waiting before setting interrupt_wait? It seems fragile to be able to interrupt a procedure that hasn't started.

Quick idea; could provide an optional flag. So the value exists whenever someone is waiting on it and is destroyed (`nullopt`) when there is no one doing it.

```diff
Index: src/node/interfaces.cpp
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
========================
...

🤔 ryanofsky reviewed a pull request: "wallet: re-activate "AmountWithFeeExceedsBalance" error"
(https://github.com/bitcoin/bitcoin/pull/25269#pullrequestreview-3384873041)
Code review ACK 98b7f1061ff104cdffebe1c2968c9f829d1a72f9 for the second commit, but I am unsure about the first commit (see below). I started reviewing this because https://github.com/bitcoin-core/gui/pull/807 depends on it, but this seems like a nice change independently that provides a clearer error message and fixes a GUI feature that was accidentally broken in #20640.

💬 ryanofsky commented on pull request "wallet: re-activate "AmountWithFeeExceedsBalance" error":
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r2466651540)
In commit "wallet: fix, make 'total_effective_amount' optional actually optional" (568753018c5ffcddf4612464cd53502fa559f6fa)

I wonder if this change requires updates to any code that might be assuming `total_effective_amount` is not null like: https://github.com/bitcoin/bitcoin/blob/56e9703968e26353fd4663e07a7bba198a272d12/src/wallet/spend.cpp#L226

Also it's not really clear what the connection is between this change in the rest of the PR. It would help to mention some motivation in the co
...

💬 ryanofsky commented on pull request "wallet: re-activate "AmountWithFeeExceedsBalance" error":
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r2466837909)
In commit "wallet: introduce "tx amount exceeds balance when fees are included" error" (98b7f1061ff104cdffebe1c2968c9f829d1a72f9)

Is it safe to assume GetEffectiveTotalAmount() returns not null here after previous commit? Might be better to use value_or

💬 ryanofsky commented on pull request "wallet: re-activate "AmountWithFeeExceedsBalance" error":
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r2466870484)
re: https://github.com/bitcoin/bitcoin/pull/25269#discussion_r2114901397

> Furthermore, if it were possible to reach this with SFFO, I think the error message would be confusing and it would actually be better to return "Insufficent Funds", so this whole check could probably be guarded by an `if (m_subtract_fee_outputs)` to skip it when SFFO is on.

I don't think I follow the logic behind this but it's possible I'm misunderstanding. If user is trying to send less money than their balance, a
...

💬 ryanofsky commented on pull request "wallet: re-activate "AmountWithFeeExceedsBalance" error":
(https://github.com/bitcoin/bitcoin/pull/25269#discussion_r2466700754)
In commit "wallet: introduce "tx amount exceeds balance when fees are included" error" (98b7f1061ff104cdffebe1c2968c9f829d1a72f9)

Can this error message include amount of fees that was in the [original error message](https://github.com/bitcoin/bitcoin/blob/56e9703968e26353fd4663e07a7bba198a272d12/src/qt/sendcoinsdialog.cpp#L746). It seems like knowing how much the wallet is trying to spend could help the user address the problem. It looks like equivalent amount would be `selection_target - re
...

⚠️ kosuodhmwa reopened an issue: "ZMQ problem: 'ss -ltnp | grep 28332' and 'ss -ltnp | grep 28333' does not give me an answer (i need it for LND)"
(https://github.com/bitcoin/bitcoin/issues/33710)
### Is there an existing issue for this?

- [x] I have searched the existing issues

### Current behaviour

Compile process:

```
[GIT stuff here]
apt install -y cmake
cd ~/bitcoin
mkdir build
cd build
apt install -y capnproto libcapnp-dev
cmake .. -DBUILD_BITCOIN_WALLET=ON -DENABLE_ZMQ=ON -DBUILD_TESTING=OFF -DBUILD_BENCH=OFF
make -j$(nproc)
```


start process:

```
root@debian12-btc-node:~# cat ./start-btcd.sh
set -x;
clear;
cd ~/;
#cd bitcoin/src;
cd bitcoin;
cd build;
cd bin;
./bitcoind | g
...

💬 kosuodhmwa commented on issue "ZMQ problem: 'ss -ltnp | grep 28332' and 'ss -ltnp | grep 28333' does not give me an answer (i need it for LND)":
(https://github.com/bitcoin/bitcoin/issues/33710#issuecomment-3453094799)
cmake script content:

```
root@debian12-btc-node:~# cat ./cmake-btcd.sh
cd ~/bitcoin/build;

# rm -rf *;

cmake .. \
-DBUILD_BITCOIN_WALLET=ON \
-DENABLE_ZMQ=ON \
-DZeroMQ_INCLUDE_DIR=/usr/include \
-DZeroMQ_LIBRARY=/usr/lib/x86_64-linux-gnu/libzmq.so \
-DBUILD_TESTING=OFF \
-DBUILD_BENCH=OFF \
-DCMAKE_VERBOSE_MAKEFILE=ON

exit 0;
root@debian12-btc-node:~#
```

When i execute it:

```
root@debian12-btc-node:~# ./cmake-btcd.sh
-- Could NOT find Doxygen (missing: DOXYGEN_EXECUTABLE
...

👍 ryanofsky approved a pull request: "refactor: interfaces, make 'createTransaction' less error-prone "
(https://github.com/bitcoin-core/gui/pull/807#pullrequestreview-3385281340)
Code review ACK 0b92ee2bc7a75074651e4688912e8e3c7de06a5a. Change looks good but needs rebase, and it would be good to update commit description and say it depends on #25269 (first commit of this PR is from #25269 and that would be good to know up front.)

💬 ryanofsky commented on pull request "refactor: interfaces, make 'createTransaction' less error-prone ":
(https://github.com/bitcoin-core/gui/pull/807#discussion_r2466945454)
In commit "refactor: interfaces, make 'createTransaction' less error-prone" (0b92ee2bc7a75074651e4688912e8e3c7de06a5a)

Might be better to replace `int()` with `static_cast<int>()`

💬 achow101 commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#issuecomment-3453170678)
ACK 8b892d41fdeb5756fd83f6050f27a170338d260a

💬 willcl-ark commented on pull request "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2":
(https://github.com/bitcoin/bitcoin/pull/33185#issuecomment-3453183249)
My guix build:

```
❯ uname -m; find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
x86_64
3de417ac1dade848d8b1609adf72c0faefcaf5acf03199259aa3aeb83e6a863f guix-build-59c4898994bd/output/aarch64-linux-gnu/SHA256SUMS.part
c6eda45fce2b34940ea53caa2acf0c1436122f89b85c0bc727834360f78a40f5 guix-build-59c4898994bd/output/aarch64-linux-gnu/bitcoin-59c4898994bd-aarch64-linux-gnu-debug.tar.gz
41584667134bfc5c35851d5005692c447d3fc5
...

💬 willcl-ark commented on pull request "guix: update time-machine to 5cb84f2013c5b1e48a7d0e617032266f1e6059e2":
(https://github.com/bitcoin/bitcoin/pull/33185#discussion_r2467017701)
I guess this means our bins won't work on Ubuntu 22.04 LTS any more as they only have [2.35](https://repology.org/project/glibc/versions)?

LTS for that version persists until 2027 and we'd probably want to update before then anyway, so perhaps it's no issue.