💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2127131414)
I've carved this out into a `NeedsRateLimiting` function and have kept the logic of `FormatLogStrInPlace` mostly unchanged. The reason I had the original change is because `FormatLogStrInPlace` inserts a timestamp at the beginning of `str` and I wanted the prepended rate-limiting logs to occur _before_ the timestamp. With the new change, the rate-limiting logs are first in `str` followed by the time-stamp. I think now the logs look a little wonky, but I like the code change better. I could also
...

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2127135142)
Both the map and set are now cleared if `LogRateLimiter` determines that the logging window should be reset.

👍 brunoerg approved a pull request: "doc: update tor docs to use correct bitcoind binary path"
(https://github.com/bitcoin/bitcoin/pull/32679#pullrequestreview-2897685988)
ACK b42ba6f806cda1e771ca9a0d57cc9716d1110a04

💬 w0xlt commented on pull request "wallet, rpc: Change `OutputType` items from `string` into compile-time constants `string_view`":
(https://github.com/bitcoin/bitcoin/pull/32432#issuecomment-2940974930)
> Not sure what you mean by "won't work". What is the exact diff to reproduce the compile failure or error?

If you try to concatenate the RPC description with any of the output type string variables, you will get an empty string. The reason seems to be that they are not yet initialized when this operation occurs.

Using string_view solves this, as it ensures that the output types are already initialized at this point.

But other suggestions on how we can use output type variables in the R
...

💬 davidgumberg commented on pull request "doc: update tor docs to use correct bitcoind binary path":
(https://github.com/bitcoin/bitcoin/pull/32679#issuecomment-2940987359)
I do sort of see both points here, I think the question to answer is who this documentation is for, there being three groups:

1. Those building from source
2. Those downloading releases and unzipping them somewhere, e.g.: `~/Downloads/bitcoin-29.0/`
3. Those who have Bitcoin Core in their `PATH`, probably installed from a package manager.

💬 theuni commented on pull request "thread-safety: fix annotations with REVERSE_LOCK":
(https://github.com/bitcoin/bitcoin/pull/32465#issuecomment-2940988167)
> To be clear, I all think the changes in this PR are improvements, even the last commit annotating REVERSE_LOCK, so I'd be happy to merge see this PR merged.
>
> I'm just saying the last commit changing and annotating REVERSE_LOCK could be unnecessary if we decide to remove it later, and think we should remove it for reasons discussed above.

Understood, and points taken.

Currently playing around with your `WITH_UNLOCK` idea to see what that looks like. I'll report back.

🤔 ryanofsky reviewed a pull request: "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`"
(https://github.com/bitcoin/bitcoin/pull/32532#pullrequestreview-2897452412)
Concept ACK 5b11b8706f14e0545f7e07c7c029e24811dc6c9d. I haven't finished reviewing but these changes seem very straightforward and the new test and benchmark coverage seems nice. Left some review comments below

💬 ryanofsky commented on pull request "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2127041222)
In commit "bench: measure `CheckBlock` speed separately from serialization" (f7b161ba72ee4b219f9ccae50ee350eb1092cd07)

It seem a bit fragile to set these flags individually far away from the CBlock definition, because more flags could be added and the benchmark could show results that are not meaningful. Might be good to to add a `ResetChecked` or similar method to the class definition and call it here instead.

💬 ryanofsky commented on pull request "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2127057166)
In commit "bench: measure `SigOpsBlock` speeds separately" (1e614fb9daa1baf1cef37d66ff30179a174ced99)

Not sure but would it maybe make sense for the GetLegacySigOpCount to take a block instead of transaction to deduplicate code running before and during the benchmark and make sure it is consistent?

Also it seems like this duplicating the [tx_verify](https://github.com/bitcoin/bitcoin/blob/a5e98dc3ae63fe8ee689917db440954206893a9f/src/consensus/tx_verify.cpp#L112) function. Might be good to
...

💬 ryanofsky commented on pull request "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2127115488)
In commit "refactor: add extra-fast checks for all standard script types" (c87c7b02796d83f5c4d8a7472a2cd13df30e6a05)

This change seems all right but I'd find it much easier to review if it could be split up to move just one `CScript::Is*` method per-commit. Otherwise it is easy to confuse the different method names and bodies because they look very similar, and verifying the c87c7b02796d83f5c4d8a7472a2cd13df30e6a05 diff requires jumping around and searching for one method at a time instead of
...

💬 ryanofsky commented on pull request "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2127193014)
In commit "refactor: add extra-fast checks for all standard script types" (c87c7b02796d83f5c4d8a7472a2cd13df30e6a05)

Any particular reason for writing 35 instead of CPubKey::COMPRESSED_SIZE + 2 like previous code?

💬 ryanofsky commented on pull request "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2127176949)
In commit "test: add `SigOpCount` edge‑cases & known‑template coverage" (461779ae4903d8e86500fe5ef4d73009d80ee311)

Could consider expanding commit message a bit to say how the new tests compare to existing tests. I think:

- GetLegacySigOpCountErrors is adding new coverage for malformed scripts that weren't checked before?
- GetLegacySigOpCountStandardScripts is adding more organized and complete coverage, but not really new coverage?

And I know you tend to push back against adding cod
...

💬 ryanofsky commented on pull request "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2127196120)
In commit "refactor: add extra-fast checks for all standard script types" (c87c7b02796d83f5c4d8a7472a2cd13df30e6a05)

This is dropping the `script[1] == 0x02 || script[1] == 0x03))` check?

💬 ryanofsky commented on pull request "refactor: extract `DecodePushData` from `GetScriptOp` and `GetLegacySigOpCount`":
(https://github.com/bitcoin/bitcoin/pull/32532#discussion_r2127198025)
In commit "refactor: add extra-fast checks for all standard script types" (c87c7b02796d83f5c4d8a7472a2cd13df30e6a05)

This is dropping the `script[1] == 0x04` check?

💬 w0xlt commented on pull request "[Draft/POC] Add secp256k1-based HPKE (Hybrid Public Key Encryption) For Payjoin v2":
(https://github.com/bitcoin/bitcoin/pull/32617#issuecomment-2941016820)
> at least BIP77 contains only the two modes "Base" and "Auth" explicitly (and no mentions of "PSK"):

Good catch. I still need to verify whether `PSK` is used by OHTTP in the Payjoin v2 protocol, but since the https://github.com/payjoin/bitcoin-hpke project used by PDK implements and has test vectors for `PSK`, I added the `PSK` and `AuthPSK` modes here as well.

💬 ismaelsadeeq commented on pull request "doc: update tor docs to use correct bitcoind binary path":
(https://github.com/bitcoin/bitcoin/pull/32679#issuecomment-2941094419)
> I lean a little towards removing the ./ prefix from the commands here to cater to group 3, whom I assume are average users.

Done, I also see thats the pattern in i2p docs.
https://github.com/bitcoin/bitcoin/compare/b42ba6f806cda1e771ca9a0d57cc9716d1110a04..a8765f2e5dae63d666e9757d8ea160db57b38fcc

✅ brunoerg closed a pull request: "wallet: sqlite: there is no need to have exclusive locking when mocking"
(https://github.com/bitcoin/bitcoin/pull/32632)

💬 brunoerg commented on pull request "wallet: sqlite: there is no need to have exclusive locking when mocking":
(https://github.com/bitcoin/bitcoin/pull/32632#issuecomment-2941121201)
> Does this materially change anything for performance? If not, I'd rather keep it as is for simplicity.

Yes, but it's hard to measure, especially because mocking is used for testing. Anyway, we can keep as is.

🤔 janb84 reviewed a pull request: "doc: update tor docs to use bitcoind binary from path"
(https://github.com/bitcoin/bitcoin/pull/32679#pullrequestreview-2897867316)
ACK a8765f2e5dae63d666e9757d8ea160db57b38fcc

Can imagine that the ./ is confusing for some people, removing is the best option to keep the documentation "generic".
(Although this can be a typical item that can get reintroduced in a new PR later on)

💬 willcl-ark commented on pull request "guix: warn and abort when SOURCE_DATE_EPOCH is set":
(https://github.com/bitcoin/bitcoin/pull/32678#issuecomment-2941252182)
My guix hashes

```
src/core/bitcoin on  source-date-epoch [$] via △ v3.31.6 via 🐍 v3.12.10 via ❄️ impure (nix-shell-env) took 52m9s
❯ find guix-build-$(git rev-parse --short=12 HEAD)/output/ -type f -print0 | env LC_ALL=C sort -z | xargs -r0 sha256sum
13c339d8340af9456ad236a76bdd7faaed0f694849e6ad15da328d17d0c87ede guix-build-5c4a0f8009ce/output/aarch64-linux-gnu/SHA256SUMS.part
afc24aab271148828178b89379a4e518664415163601c7628e93d9cf53616314 guix-build-5c4a0f8009ce/output/aarch64-li
...