💬 murchandamus commented on pull request "policy: restrict all TRUC (v3) transactions to 10KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1607176526)
<s>I don’t understand the test here. My understanding was that TRUC transactions would be generally limited to 10 kvB, and TRUC transactions with a parent would be limited to 1 kvB. It seems to me that they should together be allowed to weigh up to 11 kvB, so it is no clear to me why this comment reads "but parent's descendant limit is exceeded".</s>

Perhaps clarify here in the comment that there is a custom descendant limit in play:
```suggestion
# Parent and child are within v3 li
...

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#issuecomment-2121088705)
Am I right that after this change (or alternatives which let BatchWrite just iterate dirty/fresh entries), it would be possible to (perhaps dramatically) reduce the periodic flush time, so that after IBD, we basically always have a mostly non-dirty cache?

🤔 furszy reviewed a pull request: "indexes: Don't wipe indexes again when continuing a prior reindex"
(https://github.com/bitcoin/bitcoin/pull/30132#pullrequestreview-2067063831)
Looking good in a first glance. It would be nice to add some coverage for it. Maybe assert that certain logs are not present during init? Like the "Wiping LevelDB in <index_path>" one.

💬 hebasto commented on pull request "build: Remove `--enable-threadlocal`":
(https://github.com/bitcoin/bitcoin/pull/30137#issuecomment-2121304986)
> Concept ACK.

I forgot to mention that it would be great to ditch it before CMake :)

👍 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?