👍 cbergqvist approved a pull request: "Make it possible to disable Tor binds and abort startup on bind failure"
(https://github.com/bitcoin/bitcoin/pull/22729#pullrequestreview-2066219143)
ACK d0c8109dd0f2c8ca3ee742f0a0f11911553e0a62

Was worried that the UI might fail to show the MessageBoxes about bind errors in `CConnman::Bind()` now that `CConnman::InitBinds()` fails on bind errors instead of continuing. But MessageBoxes are modal due to `MSG_ERROR = (ICON_ERROR | BTN_OK | MODAL)` and I manually verified it works fine in *bitcoin-qt* by attempting to bind to a port occupied by another process on my machine.

Ran functional `--extended` tests with one single *expected* test
...

💬 cbergqvist commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1607299718)
The `self.expected` variable name really confused me since it includes command line inputs used to *steer* the test, but it was named prior to this PR. Maybe improve the situation with some slightly clearer names in the loop?
```python
for i, (args, expected_services) in enumerate(self.expected):
self.log.info(f"Checking listening ports of node {i} with {args}")
```

💬 cbergqvist commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1606777724)
nit: Introducing style mismatch on adjacent lines - mixing single and double quotes in the two `addr_to_hex()` calls.

💬 cbergqvist commented on pull request "Make it possible to disable Tor binds and abort startup on bind failure":
(https://github.com/bitcoin/bitcoin/pull/22729#discussion_r1607158855)
nit: Using `''.format()` instead of `f''` as recommended in https://github.com/bitcoin/bitcoin/blob/master/test/functional/README.md#style-guidelines

> Use f'{x}' for string formatting in preference to '{}'.format(x) or '%s' % x.

Prior entries use the recommended style apart from `"`-quotes.

💬 hebasto commented on pull request "[DO NOT MERGE] cmake: Migrate CI scripts to CMake-based build system -- WIP":
(https://github.com/bitcoin/bitcoin/pull/29790#issuecomment-2121349899)
This branch has been rebased on top of the https://github.com/hebasto/bitcoin/pull/202.

I've noticed two issues so far:
1. `Cannot open mapping file '/ci_container_base/ci/scratch/build-/contrib/devtools/iwyu/bitcoin.core.imp': No such file or directory.`
2. Missed disabling `EXPORT_COMPILE_COMMANDS` property for targets in the `crc32c` subtree.

💬 hebasto commented on pull request "kernel: Remove batchpriority from kernel library":
(https://github.com/bitcoin/bitcoin/pull/30083#issuecomment-2121359133)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/202.

💬 hebasto commented on pull request "build: swap cctools otool for llvm-objdump":
(https://github.com/bitcoin/bitcoin/pull/29739#issuecomment-2121359541)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/202.

💬 hebasto commented on pull request "kernel: Remove key module from kernel library":
(https://github.com/bitcoin/bitcoin/pull/29252#issuecomment-2121360201)
Ported to the CMake-based build system in https://github.com/hebasto/bitcoin/pull/202.

💬 hebasto commented on issue "CMake-based build system tracking issue":
(https://github.com/bitcoin/bitcoin/issues/28607#issuecomment-2121368098)
The main part of the https://github.com/bitcoin/bitcoin/issues/28607#issue-1929913410 has all checkboxes checked.

Thanks to all reviewers and testers!

Keep working :)

💬 sipa commented on pull request "net: Replace libnatpmp with built-in PCP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2121393948)
Trying this with `-pcp=1` on my home internet connection:
```
2024-05-20T23:28:41.625432Z [net] pcp: gateway [IPv4]: 192.168.1.1
2024-05-20T23:28:41.625469Z [net] pcp: Requesting port mapping for addr 0.0.0.0 port 8333 from gateway 192.168.1.1
2024-05-20T23:28:41.625529Z [net] pcp: Internal address after connect: 192.168.1.254
2024-05-20T23:28:41.626388Z [net] pcp: Received response of 8 bytes: 008100010046cfb7
2024-05-20T23:28:41.626404Z [net:warning] pcp: Response too small
2024-05-20T2
...

💬 kristapsk commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1607472804)
@ajtowns Good idea, can add test simple way without adding much more code. 84e03ab48e0c832ab2c0d8e6babb6563a0ee92dc

📝 maflcko opened a pull request: "ci: Add mising -Wno-error=maybe-uninitialized to armhf task"
(https://github.com/bitcoin/bitcoin/pull/30144)

💬 maflcko commented on pull request "cli: Detect port errors in rpcconnect and rpcport":
(https://github.com/bitcoin/bitcoin/pull/29521#discussion_r1607698133)
Fixed in https://github.com/bitcoin/bitcoin/pull/30144

💬 maflcko commented on pull request "rpc: Avoid getchaintxstats invalid results":
(https://github.com/bitcoin/bitcoin/pull/29720#issuecomment-2121826810)
The CI failure can be ignored during review. It is fixed in https://github.com/bitcoin/bitcoin/pull/30144

💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1607712689)
style-wise, I wonder if it makes sense to call wait_until and find_conn 6 times, when all logic could be put into a single function, which will be passed as a `lambda` to wait_until once.

I guess this would mean slightly cleaner code, but would make it harder to see which part of the single function returned `False` when a failure happens. Though, debug logs could be added to help with that, if needed.

What do you think?

💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#discussion_r1607713755)
nit: Could add a comment to where the ID is set, to clarify that it is now required for `connect_nodes` to work?

💬 maflcko commented on pull request "test: improve robustness of connect_nodes()":
(https://github.com/bitcoin/bitcoin/pull/30118#issuecomment-2121847793)
> This approach is fragile, as any of the peers involved in the process can drop, lose, or
> create a connection at any step, causing subsequent `wait_until` checks to stall indefinitely
> even when the peers in question were connected successfully.

I'd say the tests should avoid racy code and deterministically execute the test code line-by-line, but enforcing this in `connect_nodes` is the wrong approach. So Concept ACK.

💬 josibake commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1607767150)
Updated to remove the `extra` argument.

💬 vasild commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#discussion_r1607790271)
Hmm, maybe we can keep track of whether we have have successfully managed to connect to at least one `.onion` address via the `-proxy`, then we can assume that is a Tor proxy? How does that sound?

💬 virtu commented on pull request "tracing: network connection tracepoints":
(https://github.com/bitcoin/bitcoin/pull/25832#issuecomment-2121965779)
LGTM! ACK [8109319](https://github.com/bitcoin/bitcoin/commit/8109319e102c41d3aeed0ecfbc3a0e23b7fea807)