💬 maflcko commented on pull request "refactor: Avoid UB in SHA3_256::Write":
(https://github.com/bitcoin/bitcoin/pull/31655#issuecomment-2591011413)
This can be tested with a diff:


```diff
diff --git a/src/crypto/sha3.cpp b/src/crypto/sha3.cpp
index 770500bfe2..6eedba3ad6 100644
--- a/src/crypto/sha3.cpp
+++ b/src/crypto/sha3.cpp
@@ -103,8 +103,10 @@ void KeccakF(uint64_t (&st)[25])
}
}

-SHA3_256& SHA3_256::Write(Span<const unsigned char> data)
+#include <span>
+SHA3_256& SHA3_256::Write(Span<const unsigned char> old)
{
+std::span data{old};
if (m_bufsize && m_bufsize + data.size() >= sizeof(m_buffer)) {

...

👍 ryanofsky approved a pull request: "kernel: Move kernel-related cache constants to kernel cache"
(https://github.com/bitcoin/bitcoin/pull/31483#pullrequestreview-2550610710)
Code review ACK 922aa7a25650bdfd8428198f171f88915af1ffa0. Changes since last review were clamping -dbcache cache value instead of returning an error if it is too large, making more constants use bytes instead of mib, replacing MiBToBytes with SaturatingLeftShift, adding MiB literal suffix. Would maybe consider squashing the last two commits as it seems a little to safer to rename the constants at the same time as changing their units instead of separately.

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915373211)
In commit "util: Add integer left shift helpers" (https://github.com/bitcoin/bitcoin/commit/a85c07a891f76cd9e606aa83c0671506b1173f30)

Could consider dropping this line. Code is correct without it and it seems like a distraction. Or, if it is important to check type of T for some reason, presumably that reason would also apply in the `SaturatingLeftShift` function below and this same check should be done there before checking for `input < 0`.

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915356827)
In commit "util: Add integer left shift helpers" (a85c07a891f76cd9e606aa83c0671506b1173f30)

Could consider adding a fuzz test too. Maybe:

```c++
namespace {
//! Test overflow operations for type T using a wider type, W, to verify results.
template <typename T, typename W>
void TestOverflow(FuzzedDataProvider& fuzzed_data_provider)
{
constexpr auto min{std::numeric_limits<T>::min()};
constexpr auto max{std::numeric_limits<T>::max()};
static_assert(min >= std::numeric_lim
...

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915527924)
In commit "init: Use size_t consistently for cache sizes" (922aa7a25650bdfd8428198f171f88915af1ffa0)

Note: Passing max `size_t` value to `min<uint64_t>` could be broken in the theoretical case where `size_t` type is wider than `uint64_t`. If we had a [`saturate_cast`](https://en.cppreference.com/w/cpp/numeric/saturate_cast) function the code be simplified to avoid this type of problem:

```c++
if (std::optional<int64_t> db_cache = args.GetIntArg("-dbcache")) {
total_cache = st
...

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915387311)
In commit "util: Add integer left shift helpers" (https://github.com/bitcoin/bitcoin/commit/a85c07a891f76cd9e606aa83c0671506b1173f30)

"overflow or too high shift value" seem like the same thing so it is confusing to refer to them as different cases. Phrasing is also potentially ambiguous. Would maybe suggest `@return (input * 2^shift) or nullopt if it would not fit in the return type.`

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915394447)
In commit "util: Add integer left shift helpers" (https://github.com/bitcoin/bitcoin/commit/a85c07a891f76cd9e606aa83c0671506b1173f30)

Again would suggest something shorter and less ambiguous about meaning of left shift like `@return (input * 2^shift) clamped to fit between the lowest and highest representable values of the type T`

👍 ryanofsky approved a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2550968005)
Code review ACK fa51381790fe19e37e04a01a39cc94a00dcc441c. Just rebased and fixed minor conflict since last review.

💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915573360)
I had a compiler complain about signed to unsigned integer comparison without it.

💬 pablomartin4btc commented on pull request "refactor: Use our own implementation of urlDecode":
(https://github.com/bitcoin/bitcoin/pull/29904#discussion_r1915579276)
Having chatted with you in #31194 I do agree with your thoughts.

I've tested this PR adding these 2 checks (comparing it with the libevent uri decoder [output](https://github.com/bitcoin/bitcoin/issues/31194#issuecomment-2585885945)), similar to what you did in your [branch](https://github.com/pinheadmz/bitcoin/blob/4084f6f739412acea0df7c93a2d31bdeeadef9bb/src/test/httpserver_tests.cpp#L110-L111) (sorry to be late on this review):

```
BOOST_CHECK_EQUAL(UrlDecode("%3Fp1=v1%20&p1=v2%20"
...

🚀 glozow merged a pull request: "test: avoid internet traffic"
(https://github.com/bitcoin/bitcoin/pull/31646)

🚀 glozow merged a pull request: "doc: Archive 28.1 release notes"
(https://github.com/bitcoin/bitcoin/pull/31630)

💬 luke-jr commented on pull request "[28.x] Backports":
(https://github.com/bitcoin/bitcoin/pull/31648#issuecomment-2591115647)
#31623 is a clean merge to both branches, so no need for a cherry-pick here

💬 TheBlueMatt commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2591123833)
I was referencing the comment at https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2450593825 The changes proposed do not accomplish point 1, 3, or 4, only point 2. Point 2 is possible more important than 1 or 3, but 4 is also quite important.

💬 Zero-1729 commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1915613633)
It'd be good to clarify here that it is recommended to use `APFS` instead.

```suggestion
- **MacOS**: The exFAT filesystem should not be used, it is recommended to use the APFS filesystem instead. There have been multiple reports of database corruption when using exFAT on MacOS for Bitcoin Core. This appears to be due to filesystem-level issues with exFAT on MacOS. See [Issue #31454](https://github.com/bitcoin/bitcoin/issues/31454) for more details.
```

💬 Zero-1729 commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1915615158)
I'd be good to add a recommendation here, too, so the user is aware of possible immediate solutions.

```suggestion
InitWarning(strprintf(_("Specified %s \"%s\" is exFAT which is known to have intermittent corruption problems on MacOS, use APFS instead. "
```

💬 TheCharlatan commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915615707)
Thanks, but I get a crash immediately:
```
test/fuzz/overflow.cpp:36 TestOverflow: Assertion `check(widen(i) << shift) == CheckedLeftShift(i, j)' failed.
```
Does this correction make sense to you?
```git
diff --git a/src/test/fuzz/overflow.cpp b/src/test/fuzz/overflow.cpp
index defaf9926d..d29273eb6e 100644
--- a/src/test/fuzz/overflow.cpp
+++ b/src/test/fuzz/overflow.cpp
@@ -27,13 +27,13 @@ void TestOverflow(FuzzedDataProvider& fuzzed_data_provider)

const T i = fuzzed_data_p
...

💬 sipa commented on issue "Mining Interface doesn't allow for Bitcoin Core to create blocks when it wants":
(https://github.com/bitcoin/bitcoin/issues/31109#issuecomment-2591132620)
@TheBlueMatt I don't follow. A "wait until you have something for me" interface is push-based, and accomplishes all points as far as I can tell.

💬 ryanofsky commented on pull request "kernel: Move kernel-related cache constants to kernel cache":
(https://github.com/bitcoin/bitcoin/pull/31483#discussion_r1915622193)
Thanks, the bottom part of that correction (shifting by `shift` instead of `j`) makes sense but the top part changing `std::numeric_limits<W>::digits ` to `std::numeric_limits<T>::digits` reduces the coverage of the fuzz test by reducing the range of `shift` so I think would not be a good change.

🤔 Zero-1729 reviewed a pull request: "util: detect and warn when using exFAT on MacOS"
(https://github.com/bitcoin/bitcoin/pull/31453#pullrequestreview-2551060227)
Concept ACK

I encountered this error late last year as well. Thank you for working on this. I think having the warning reported is a good measure.

I've dropped a few comments above.

I tested the patch on an M1 MBP running MacOS v15.2 with an exFAT drive (83839a35f07055dd03924b5bdab46bf31df33b35):

```
$ ./build/src/qt/bitcoin-qt --datadir=/Volumes/Little\ Test/

Warning: Specified data directory "/Volumes/Little Test" is exFAT which is known to have intermittent corruption problems
...