💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476890547)
(see above)

💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476890802)
Correct, but I wanted to keep this commit limited to just member fields, see https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476860710. Happy to revert this hunk and leave if for a follow-up.

💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476890904)
> Not sure I understand why `GetBlockWeight` is `int64_t`

Generally, signed arithmetic types are preferred, when the values are later on used in arithmetic expressions. (Though, I don't want to re-hash the discussion here and just focus on moving 32-bit to 64-bit unsigned arithmetic)



> but `MAX_BLOCK_WEIGHT` and `MAX_BLOCK_SERIALIZED_SIZE` are only `unsigned int`

The type is sufficient to hold the value (also enforced at compile time by the compiler) and the type will promote itself
...

💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476890984)
Correct, but I wanted to keep this commit limited to just member fields, see https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476860710. Happy to revert this hunk and leave if for a follow-up.

💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476891089)
> Can we migrate the constant to 64 bits as well?

Yes, happy to push that. (See my above comment why 64-bit is preferable)

💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476891157)
Yes, a signed UniValue can be different here, when an overflow occurs. It would be represented as a string that starts with the `-` char.

However, I don't think an overflow can occur, as explained in the commit message.

💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476903348)
Yeah, happy to push. I can see it go either way, because those return values are not primarily used in consensus, but rather try to mimic the std lib containers interface.

💬 maflcko commented on issue "RFC: Do we want to support fuzzing on MacOS?":
(https://github.com/bitcoin/bitcoin/issues/33731#issuecomment-3466734703)
My suggestion would be similar to how valgrind is handled, which doesn't really work unless the dev picks a tested Linux distro, arch, and compiler, with compiler settings:

https://github.com/bitcoin/bitcoin/blob/72511fd02e72b74be11273e97bd7911786a82e54/contrib/valgrind.supp#L15-L16

In that case, I'd expect that the `doc/fuzzing.md` would only list the latest release of macOS and the exact steps (with compiler version) to reproduce there. If someone is on an older macOS, they can check the old
...

📝 maflcko opened a pull request: "doc: Document valgrind aarch64 clang workaround"
(https://github.com/bitcoin/bitcoin/pull/33742)
Fixes https://github.com/bitcoin/bitcoin/issues/29635

The upstream bug will likely not be fixed any time soon, so it seems better to document the workaround and refer to the upstream bug.

This also allows to use any clang version (instead of requiring 16), going forward.

💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#issuecomment-3466897433)
Other candidates could be: `common/ compat/ primitives/ script/ support/ util/ zmq/`

💬 ismaelsadeeq commented on pull request "node: add `BlockTemplate{Cache, Manager}`":
(https://github.com/bitcoin/bitcoin/pull/33421#issuecomment-3466996399)
> I do think while the code changes seem good, it would be good to have more clarity about the cases where cache hits are expected and this cache is most likely to be useful. This seems like something that could be mentioned in the BlockTemplateCache documentation. I know the issue #33389 mentioned a number of cases but it's unclear if these would be helped by the caching implemented here or if more changes like sharing templates with different options would be required.
>
> If you have more
...

💬 fanquake 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-3467100580)
It seems a bit premature to be doing code review or merging anything here, until some discussion has played out in #33684 in regards to approach, and implementation details (can you link to it from the PR description). There still seem to be high-level questions that need answering/agreement on.

💬 fanquake commented on issue "Async Payjoin":
(https://github.com/bitcoin/bitcoin/issues/33684#issuecomment-3467103998)
Given that this is adding new features to the GUI. Is the plan to add them to the "legacy" GUI, the new QML GUI, or implement Payjoin into both? cc @hebasto

📝 maflcko opened a pull request: "fuzz: refactor memcpy to std::ranges::copy to work around ubsan warn"
(https://github.com/bitcoin/bitcoin/pull/33743)
Using std::ranges::copy from the C++ standard library has a few benefits here:

* It has the additional benefit of being a bit more type safe and document the byte cast explicitly.
* The compiler will likely optimize it to the same asm, but performance doesn't really matter here anyway.
* It works around an UB-Sanitizer bug, when the source range is empty.

Fixes https://github.com/bitcoin/bitcoin/issues/33643

💬 maflcko commented on pull request "fuzz: don't pass (maybe) nullptr to memcpy()":
(https://github.com/bitcoin/bitcoin/pull/33644#discussion_r2477298479)
Looks like this fell off the radar. It is a fuzz blocker for 31.0, so I went ahead and pushed my alternative fix in https://github.com/bitcoin/bitcoin/pull/33743

💬 fanquake commented on pull request "fuzz: don't pass (maybe) nullptr to memcpy()":
(https://github.com/bitcoin/bitcoin/pull/33644#discussion_r2477301763)
Ok, closing this for now.

✅ fanquake closed a pull request: "fuzz: don't pass (maybe) nullptr to memcpy()"
(https://github.com/bitcoin/bitcoin/pull/33644)

💬 fanquake commented on pull request "fuzz: refactor memcpy to std::ranges::copy to work around ubsan warn":
(https://github.com/bitcoin/bitcoin/pull/33743#issuecomment-3467141145)
cc @dergoegge @marcofleon

💬 maflcko commented on issue "fuzz: connman fuzz target: runtime error: null pointer passed as argument 2, which is declared to never be null":
(https://github.com/bitcoin/bitcoin/issues/33643#issuecomment-3467153921)
Just to copy the background details here, mentioned earlier:

Apart from https://www.open-std.org/JTC1/SC22/WG14/www/docs/n3466.pdf , see also https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3322.pdf , which says:

> Modify 7.26.1p3:
Where an argument declared as size_t n specifies the length of the array for a function, n can
have the value zero on a call to that function. Unless explicitly stated otherwise in the
description of a particular function in this subclause, pointer arguments on su
...

💬 fanquake commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2477333320)
Thanks, looks better now.