💬 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
...
(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
(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
(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.
(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?
(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)
(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)
(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.
(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
...
(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.
(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)
(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.
(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.
(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
...
(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.
(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/`
(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
...
(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.
(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
(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
(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
(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