📝 maflcko opened a pull request: " refactor: Avoid UB in SHA3_256::Write "
(https://github.com/bitcoin/bitcoin/pull/31655)
It is UB to apply a distance to a pointer or iterator further than the
end itself, even if the distance is (partially) revoked later on.

Fix the issue by advancing the data pointer at most to the end.

This fix is required to adopt C++ safe buffers https://github.com/bitcoin/bitcoin/issues/31272.

Also included is a somewhat unrelated commit.

💬 maflcko commented on pull request "refactor: Avoid UB in SHA3_256::Write":
(https://github.com/bitcoin/bitcoin/pull/31655#issuecomment-2590857397)
I tested with g++-14 and the resulting `bitcoind` was unchanged, so I tagged this as "refactor". However, this may not hold for every compiler and build setting.

💬 sipa commented on pull request "refactor: Avoid UB in SHA3_256::Write":
(https://github.com/bitcoin/bitcoin/pull/31655#issuecomment-2590878790)
utACK fabeca3458b38a3d8930cb0cbc866388c3f120f1

💬 hebasto commented on pull request "depends, NetBSD: Fix `bdb` package compilation with GCC-14":
(https://github.com/bitcoin/bitcoin/pull/31613#issuecomment-2590898164)
> It looks like `string.h` includes `strings.h` which defines these functions, but only if `_NETBSD_SOURCE` is defined....
>
> So agreed with @maflcko, let's just add a patch to include `strings.h` for netbsd.

The `strsep` function declaration is [guarded by `_NETBSD_SOURCE`](https://github.com/NetBSD/src/blob/c147cae3a562b7215a69630b1e3f3aaed09cd1dd/include/string.h#L97-L112) as well::
```c
#if defined(_NETBSD_SOURCE)
#include <strings.h> /* for backwards-compatibility */
__BEGIN_DEC
...

💬 sipa commented on pull request "consensus: Remove checkpoints (take 2)":
(https://github.com/bitcoin/bitcoin/pull/31649#discussion_r1915464618)
> part of the block index, or exposed via RPC

I don't believe either of these to be the case.

📝 yancyribbens opened a pull request: "test: Add expected result assertions"
(https://github.com/bitcoin/bitcoin/pull/31656)
This is a trivial addition to the test suit, however it shouldn't be required to add debug statements and manually run the tests if someone needs to know the results of this test.

👍 hodlinator approved a pull request: "refactor: Check translatable format strings at compile-time"
(https://github.com/bitcoin/bitcoin/pull/31061#pullrequestreview-2550849478)
re-ACK fa51381790fe19e37e04a01a39cc94a00dcc441c

Just resolved conflict with #31616 since my last review.

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