🤔 theuni reviewed a pull request: "build: Remove `--enable-threadlocal`"
(https://github.com/bitcoin/bitcoin/pull/30137#pullrequestreview-2066875638)
Concept ACK

🤔 mzumsande reviewed a pull request: "validation: sync chainstate to disk after syncing to tip"
(https://github.com/bitcoin/bitcoin/pull/15218#pullrequestreview-2066847859)
Concept ACK

While this one-time sync after IBD should help in some situations, I'm not sure that it completely resolves https://github.com/bitcoin/bitcoin/issues/11600 (I encountered this PR while looking into possible improvements to `ReplayBlocks()`)
After all, there are several other situations in which a crash / unclean shutdown could lead to extensive replays (e.g. during IBD) that this PR doesn't address.

💬 mzumsande commented on pull request "validation: sync chainstate to disk after syncing to tip":
(https://github.com/bitcoin/bitcoin/pull/15218#discussion_r1607166628)
Should the original scheduling be conditional on `IsInitialBlockDownload() == true`? Seems like the goal of this PR is to trigger flush if we transition out of IBD, but not so much after restarting an almost or completely synced node. Currently, it will sync 1 minute after restart (with the second call to `SyncCoinsTipAfterChainSync`, because for the first one after 30s `last_chain_height` is still -1).

🤔 murchandamus reviewed a pull request: "policy: restrict all TRUC (v3) transactions to 10KvB"
(https://github.com/bitcoin/bitcoin/pull/29873#pullrequestreview-2066839394)
ACK 0583aeef4f2b09f35547f95f1fe21dc14738b65d

Nit in PR title and second commit message:
Uppercase _K_ kilo refers to 1024¹ per the JEDEC standard, the symbol of the metric prefix _kilo_ for 1000¹ is lowercase _k_ (presumably because the symbol _K_ was already used for Kelvin in the SI system). Since you are limiting TRUC transactions to 10,000 vB, I think you mean "kvB" instead of "KvB".

💬 murchandamus commented on pull request "policy: restrict all TRUC (v3) transactions to 10KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#discussion_r1607161860)
Typo:
```suggestion
// descendant limit descendants. The transaction also cannot be v3,
// as its topology restrictions do not allow a second child.
```

💬 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