💬 Sjors commented on pull request "descriptors: Allow `H` as a hardened indicator":
(https://github.com/bitcoin/bitcoin/pull/32788#issuecomment-2999109559)
> you could encode an xpub into a format like bech32, encoded into QR alphanumeric mode and save ~15% over a base58 encoded xpub in QR binary mode

I think it would be nice in general to have a bech32m encoding for extended keys, because part of the reason why descriptors look weird is the mix of bech32 and base58.

✅ polespinasa closed a pull request: "wallet, rpc: remove settxfee and paytxfee"
(https://github.com/bitcoin/bitcoin/pull/32138)

💬 polespinasa commented on pull request "wallet, rpc: remove settxfee and paytxfee":
(https://github.com/bitcoin/bitcoin/pull/32138#issuecomment-2999139610)
Will re-open closer to 31.0 release

💬 willcl-ark commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#issuecomment-2999233449)
> No need to mention the warning for the blocksdir since it just a folder in the datadir; It's whats being checked in [ismaelsadeeq@ddb02a6](https://github.com/ismaelsadeeq/bitcoin/commit/ddb02a6beb0ec892c18b43a55d53dc2daf11e555)

Ok, we now check if blocksdir is inside datadir, and removed the long URL from the warning.

💬 maflcko commented on pull request "test: allow all functional tests to be run or skipped with --usecli":
(https://github.com/bitcoin/bitcoin/pull/32290#issuecomment-2999413010)
only change is some nits

re-ACK 666016e56b28b77f798dc85c767b95c1ca0abfae 🔀

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment:
...

👍 dergoegge approved a pull request: "node: cap `-maxmempool` and `-dbcache` values for 32-bit"
(https://github.com/bitcoin/bitcoin/pull/32530#pullrequestreview-2952838460)
Code review ACK 9f8e7b0b3b787b873045a4a8194e77d0b0a2b3b6

The chosen limits seem reasonable to me.

🤔 maflcko reviewed a pull request: "rpc: use CScheduler for HTTPRPCTimer"
(https://github.com/bitcoin/bitcoin/pull/32796#pullrequestreview-2952901140)
lgtm ACK 466722cbe5d6c9ddf35d1590b749d19ec115684e 🎱

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 466722cbe5d6c9dd
...

💬 maflcko commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2163404769)
```suggestion
explicit HTTPRPCTimerInterface(const std::any& context)
: m_context{*Assert(std::any_cast<NodeContext*>(context))}
```

💬 maflcko commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2163412918)
if this turns out problematic (intermittently fail), one could also try `mockscheduler`.

💬 maflcko commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2163408031)
nit: Could probably assert early on construction, rather than when a timer is created, but either is equally fine.

👍 l0rinc approved a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2952512677)
Can you please update the PR descripton now that there's now `UNCONDITIONAL_ALWAYS category`

I'm mostly ok with the change, I'm testing if it introduces any slowdown and would prefer making the tests a bit more typesafe and compact

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163198581)
Seems to me the first two sentences contradict each other now: "log unconditionally, unless it doesn't fulfill the rate limiting condition"

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163176717)
nit: I find mixing C++20 code with C API a bit clunky, if you touch again, consider either comparing their `std::string_view` wrappers or maybe even adding such a helper to `string.h` for `const char*` values.

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163211212)
Does this "prevent" rate limiting or just allows one more megabyte?

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163209427)
That's a reasonable assumption - if a bug could trigger debug logging, we'd likely have bigger problems I guess

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163285496)
👍 nice and clear test

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163179624)
I understand when the time is split off from the measured code to make sure it's accurate - but I don't see how that's the case here, consider inlining it:
```suggestion
m_limiter.m_last_reset = NodeClock::now();
```

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163230657)
this seems a bit hacky to me - this is basically a parameterized test where we implicitly verify the inputs produce the desired outputs, line-by-line. It could make sense to store it in a way that indicates more clearly which outputs we're expecting for which inputs.

We do need separate `::current()` calls to capture that the lines are assigned properly, we could add those to the parameterized test values as well.

I also find it a bit awkward that we're concatenating the result of a `tfm::
...

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163313166)
I find this confusing (either the name or the value), by default debug logs aren't displayed...

💬 l0rinc commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2163309586)
I'm not a fan of dead comments, could we rather extract it to a named variable, something like `CSipHasher uniform(0, 0)` instead of explaining code with text?

----


nit: Maybe my formatter is off, but do we really need to push the formatted lines so far out?