💬 theuni commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1607107513)
To be clear, the only reason for passing `extra` here is so that it can be reserved?

If so, that's not very intuitive and seems kinda like overkill. Seems like always reserving an extra 6 bytes fine, even if they don't end up getting used?

💬 paplorinc commented on pull request "refactor: Model the bech32 charlimit as an Enum":
(https://github.com/bitcoin/bitcoin/pull/30047#discussion_r1607125130)
Yes, it's either 0 or 6.
I agree that we can likely always add them the end, amended my original PR: https://github.com/bitcoin/bitcoin/pull/29607/files#diff-f146300624c06d2e08aadf500952294148a1785edd6ff2e8b50f13b2c08255edR317

💬 edilmedeiros commented on pull request "contrib/signet/miner: increase miner search space":
(https://github.com/bitcoin/bitcoin/pull/30130#issuecomment-2120983038)
Thanks for the review, @AngusP. Your suggestion did the trick, indeed.

<details>
<summary>Log from mining two blocks. The first exhausted the grinder search space. The second didn't.</summary>

```bash
2024-05-20 15:05:53 INFO Mined block at height 10142; next in -464h55m33s (mine)
2024-05-20 15:05:53 DEBUG Calling bitcoin-cli: ['/Users/jose.edil/2-development/bitcoin/bitcoin-core/src/bitcoin-cli', '-datadir=/Users/jose.edil/2-development/bitcoin/signet-mining-experiments/signet-fix-ha
...

💬 edilmedeiros commented on issue "contrib/signet/miner: grind will fail for high difficulty chain":
(https://github.com/bitcoin/bitcoin/issues/30102#issuecomment-2120986786)
@1ma, since you seem to have a high difficulty signet chain running, may I ask for a review in #30130?

💬 ajtowns commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#discussion_r1607131131)
Just adding the true check in `mempool_datacarrier` seems like it would be easy?

💬 AngusP commented on pull request "contrib/signet/miner: increase miner search space":
(https://github.com/bitcoin/bitcoin/pull/30130#discussion_r1607133058)
Ah yes, I feel I should've remembered that 😆 it's usually considered correct-est to use the `%` formatting in log messages because that formatted string is only actually created if the message will be logged based on log-level, vs `f"` or `.format` or `" % ...` which are always created. Others may consider that premature optimisation if string-formatting due to logging has negligible perf impact 😉

💬 luke-jr commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1607136024)
This feels overengineered. Why can't we stick to fs::perms internally and just map the string in a function?

💬 sipa commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#issuecomment-2121018386)
Benchmarks on my Ryzen 5950X system:

| ns/op | op/s | err% | total | benchmark
|--------------------:|--------------------:|--------:|----------:|:----------
| 2,373.94 | 421,240.11 | 0.1% | 1.10 | `LinearizeNoIters16TxWorstCase`
| 7,530.22 | 132,798.26 | 0.0% | 1.07 | `LinearizeNoIters32TxWorstCase`
| 16,585.34 | 60,294.20 | 0.1% | 1.10 | `LinearizeNoIters48TxWorstCase`
...

💬 murchandamus commented on pull request "policy: restrict all TRUC (v3) transactions to 10KvB":
(https://github.com/bitcoin/bitcoin/pull/29873#issuecomment-2121032237)
Concept ACK on making the TRUC transaction vsize limit 10 kvB.

🤔 theuni reviewed a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2066845490)
Concept ACK. Nice.

💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1607175446)
Any reason to not make `max_size_bytes` a `CSignatureCache` constructor param? That way `InitSignatureCache` could go away.

🤔 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 :)