💬 edilmedeiros commented on pull request "contrib/signet/miner: increase miner search space":
(https://github.com/bitcoin/bitcoin/pull/30130#discussion_r1607049322)
Thanks, that did the job indeed.

💬 luke-jr commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1607069420)
`%s`

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

💬 ajtowns commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1607094214)
Rather than adding an error return in the constructor here, wouldn't it be better to change our cuckoo cache to clamp its size when given too large a value? ie, `InitScriptExecutionCache(135000000)` is treated as `134,217,727` instead? The value passed in is only used as an approximate target in the first place. Historical context is #25527.

<details>
<summary>(example patch)</summary>

```diff
--- a/src/cuckoocache.h
+++ b/src/cuckoocache.h
@@ -360,16 +360,16 @@ public:
* struct
...

💬 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.
```