💬 TheCharlatan commented on pull request "kernel: Introduce C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#issuecomment-3466427632)
> Are the assert statements in src/kernel/bitcoinkernel.cpp really necessary, or could they be replaced with runtime validation?

Replacing them with validation and returning an error works too. However that does not solve the problem of handling assertions within the code, since we have many places within validation that assert on an expected invariant. They are supposed to guard against mistakes from the programmer. Have you tried stubbing out the assertion code / header in a similar way to
...

🤔 maflcko reviewed a pull request: "rpc: Optionally print feerates in sat/vb"
(https://github.com/bitcoin/bitcoin/pull/33741#pullrequestreview-3397729983)
I guess it is a bit verbose to have each RPC annotated with a type optional arg, but there probably isn't an easier solution.

There could be a global option to toggle the default, to improve the UX.

Though, please don't use floating point types. For monetary values, a fixed point type should be used.

💬 maflcko commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2476651745)
Please don't encode default values in how the code is written. This makes it impossible to change the default values without rewriting the code logic.

Also, could use `self.(Maybe)Arg<bool>` instead?

💬 maflcko commented on pull request "rpc: Optionally print feerates in sat/vb":
(https://github.com/bitcoin/bitcoin/pull/33741#discussion_r2476689034)
this is wrong, you can't use `double` to denote monetary amounts. `double` is a floating point type, which can not represent decimal values accurately.

For example, if `GetFeePerK()` returns `CAmount{665714}`, the Univalue will be `665.7140000000001`, which is confusing at best, but also wrong.

💬 l0rinc commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3466597361)
Thanks, looks like a missed a few manual undos.
Reverted the 3 remaining files that seem related to the cluster mempool work and verified that it merges cleanly with https://github.com/bitcoin/bitcoin/pull/33629 and https://github.com/bitcoin/bitcoin/pull/33591

💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476860710)
Good question. I wanted to keep it limited to member fields (and their constructors) only. Otherwise, there'd be way too many function calls to touch. Happy to do those in a follow-up. Also, happy to drop the `src/node/blockstorage.cpp` changes from this commit, to keep it more focussed on just member fields.

💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476885390)
> This value will be returned and converted to a `size_t` again

Correct, but this is safe, because the range was checked in the `if` below.

Generally, the idea is that 32-bits is just not really enough to do size accounting (even on 32-bit architectures), because some code paths may accumulate sizes over time, or multiply them with a factor, all of which may or may not overflow the limited 32-bit value.

So using 64-bit arithmetic is overall safer, and more future proof, because current
...

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

💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476889544)
Yeah, I can go that way, but I think using 64-bit by default and explicitly casting after the range has been properly checked seems safer, see also https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476885390

💬 maflcko commented on pull request "refactor: Return uint64_t from GetSerializeSize":
(https://github.com/bitcoin/bitcoin/pull/33724#discussion_r2476889665)
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_r2476889813)
I wanted to keep this one focussed on the kernel. Wallet changes can be done in a follow-up, if that is fine?

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

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