💬 sipa commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827806882)
@l0rinc That doesn't test anything. `sizeof()` is a compile-time property, that just depends on the arguments' type. Since `empty_msg` and `msg_with_type` are the same type, the check will always be true.
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827806882)
@l0rinc That doesn't test anything. `sizeof()` is a compile-time property, that just depends on the arguments' type. Since `empty_msg` and `msg_with_type` are the same type, the check will always be true.
💬 ryanofsky commented on pull request "build: Make G_FUZZING constexpr, require -DBUILD_FOR_FUZZING=ON to fuzz":
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2454859628)
The use-case which I think this breaks and should be supported is the ability to use fuzzing in a normal build instead of a dedicated build.
As an analogy, if you want use a debugger, the best place to use it is in a dedicated debug build, but you should also be able to generate debug symbols and use debuggers with some limitations in release builds. Or if you want to check memory safety, the best way to do it may be to run MSan or valgrind in dedicated builds, but it should also be possible
...
(https://github.com/bitcoin/bitcoin/pull/31191#issuecomment-2454859628)
The use-case which I think this breaks and should be supported is the ability to use fuzzing in a normal build instead of a dedicated build.
As an analogy, if you want use a debugger, the best place to use it is in a dedicated debug build, but you should also be able to generate debug symbols and use debuggers with some limitations in release builds. Or if you want to check memory safety, the best way to do it may be to run MSan or valgrind in dedicated builds, but it should also be possible
...
💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827825362)
Lol, that was a stupid mistake, lemme' try again:
```C++
BOOST_AUTO_TEST_CASE(sso_size_test)
{
auto is_sso{[](const std::string& s, const CSerializedNetMsg& msg) {
const auto msg_start = reinterpret_cast<const char*>(&msg);
const auto msg_end = msg_start + sizeof(msg);
return msg_start <= s.data() && s.data() <= msg_end;
}};
const CSerializedNetMsg empty_msg;
BOOST_CHECK(is_sso(empty_msg.m_type, empty_msg));
CSerializedNetMsg msg_with_type;
msg_with_
...
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827825362)
Lol, that was a stupid mistake, lemme' try again:
```C++
BOOST_AUTO_TEST_CASE(sso_size_test)
{
auto is_sso{[](const std::string& s, const CSerializedNetMsg& msg) {
const auto msg_start = reinterpret_cast<const char*>(&msg);
const auto msg_end = msg_start + sizeof(msg);
return msg_start <= s.data() && s.data() <= msg_end;
}};
const CSerializedNetMsg empty_msg;
BOOST_CHECK(is_sso(empty_msg.m_type, empty_msg));
CSerializedNetMsg msg_with_type;
msg_with_
...
✅ knst closed an issue: "guix: failure on Kubuntu 24-10: error: mount: mount "none" on "/tmp/guix-directory.VEMlin": Permission denied"
(https://github.com/bitcoin/bitcoin/issues/31202)
(https://github.com/bitcoin/bitcoin/issues/31202)
💬 knst commented on issue "guix: failure on Kubuntu 24-10: error: mount: mount "none" on "/tmp/guix-directory.VEMlin": Permission denied":
(https://github.com/bitcoin/bitcoin/issues/31202#issuecomment-2454882552)
Removing app-armor and re-installing guix + reboot helped, thanks for quick help!
P.S. I guess I'm moving to Manjaro; a broken AppArmor package out of the box for 2 continuous releases is disappointing.
(https://github.com/bitcoin/bitcoin/issues/31202#issuecomment-2454882552)
Removing app-armor and re-installing guix + reboot helped, thanks for quick help!
P.S. I guess I'm moving to Manjaro; a broken AppArmor package out of the box for 2 continuous releases is disappointing.
💬 instagibbs commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1827857362)
Oof, this was making it a lot harder to hit the various cases! Fixed!
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1827857362)
Oof, this was making it a lot harder to hit the various cases! Fixed!
💬 l0rinc commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#issuecomment-2454914258)
> I don't understand why M1 Macs would have more ops after
My understand is that when we've reused the hash as input, it could only run a single instance since each one depended on the previous one. The new way is more predictable and clang often optimizes more aggressively, I guess it unrolls the instances. I can investigate if you think it's important.
> refresh the results anyway after latest push
Update the description, thanks.
(https://github.com/bitcoin/bitcoin/pull/30349#issuecomment-2454914258)
> I don't understand why M1 Macs would have more ops after
My understand is that when we've reused the hash as input, it could only run a single instance since each one depended on the previous one. The new way is more predictable and clang often optimizes more aggressively, I guess it unrolls the instances. I can investigate if you think it's important.
> refresh the results anyway after latest push
Update the description, thanks.
💬 Abdulkbk commented on pull request "Improve lockunspent validation for vout":
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2454922674)
> Every use of `getInt` represents a whole number. This is not special. If you disagree, it would be good to explain and to provide an example. Also, What real-world end-user-facing issue is this trying to solve?
>
> Tend toward NACK for now, unless there is a reason.
I am new to the codebase and learned that every use of `getInt` represents a whole number after you mentioned it.
For the end users facing this issue, one example I can give, those using `bitcoin-cli lockunspent`, may enco
...
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2454922674)
> Every use of `getInt` represents a whole number. This is not special. If you disagree, it would be good to explain and to provide an example. Also, What real-world end-user-facing issue is this trying to solve?
>
> Tend toward NACK for now, unless there is a reason.
I am new to the codebase and learned that every use of `getInt` represents a whole number after you mentioned it.
For the end users facing this issue, one example I can give, those using `bitcoin-cli lockunspent`, may enco
...
💬 dergoegge commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1827872630)
Done. Let me know if you're not happy with the wording.
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1827872630)
Done. Let me know if you're not happy with the wording.
💬 dergoegge commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1827873746)
Thanks, removed the ExpandDescriptor bench as well. Will comment on how I picked the benches soon.
(https://github.com/bitcoin/bitcoin/pull/31153#discussion_r1827873746)
Thanks, removed the ExpandDescriptor bench as well. Will comment on how I picked the benches soon.
📝 marcofleon opened a pull request: "fuzz: Fix difficulty target generation in `p2p_headers_presync`"
(https://github.com/bitcoin/bitcoin/pull/31213)
In the `p2p_headers_presync` fuzz target, this assertion failed:
```
assert(total_work < chainman.MinimumChainWork());
```
The test previously used `ConsumeIntegralInRange` to generate header difficulty targets within a hardcoded range. The fuzzer found specific values in that range that correspond to very low thresholds due to how [`SetCompact`][setcompact-link] works. The total work of a long enough test chain ended up exceeding `MinimumChainWork`.
Fix this by adding a new `ConsumeAri
...
(https://github.com/bitcoin/bitcoin/pull/31213)
In the `p2p_headers_presync` fuzz target, this assertion failed:
```
assert(total_work < chainman.MinimumChainWork());
```
The test previously used `ConsumeIntegralInRange` to generate header difficulty targets within a hardcoded range. The fuzzer found specific values in that range that correspond to very low thresholds due to how [`SetCompact`][setcompact-link] works. The total work of a long enough test chain ended up exceeding `MinimumChainWork`.
Fix this by adding a new `ConsumeAri
...
💬 instagibbs commented on pull request "fuzz: Fix difficulty target generation in `p2p_headers_presync`":
(https://github.com/bitcoin/bitcoin/pull/31213#issuecomment-2454954020)
To be clear, `nBits` is "deterministic" in that the value, given the chain history, is static, right?
(https://github.com/bitcoin/bitcoin/pull/31213#issuecomment-2454954020)
To be clear, `nBits` is "deterministic" in that the value, given the chain history, is static, right?
💬 sipa commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827884684)
@laanwj If you were to actually incorporate that into this PR, be aware you'll need to use `!std::greater{}(a, b)` instead of `a <= b` when comparing unrelated pointers like this (using `<=` for that is technically unspecified behavior, though it works fine in every real compiler IIRC).
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827884684)
@laanwj If you were to actually incorporate that into this PR, be aware you'll need to use `!std::greater{}(a, b)` instead of `a <= b` when comparing unrelated pointers like this (using `<=` for that is technically unspecified behavior, though it works fine in every real compiler IIRC).
💬 maflcko commented on pull request "Improve lockunspent validation for vout":
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2454965521)
> For the end users facing this issue, one example I can give, those using `bitcoin-cli lockunspent`, may encounter confusion when they un/intentionally pass a floating-point vout and receive the error "JSON integer out of range."
If the error message is confusing, it would be a reason to improve the error message. However, this applies to all RPCs and changing a single RPC seems inconsistent and confusing.
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2454965521)
> For the end users facing this issue, one example I can give, those using `bitcoin-cli lockunspent`, may encounter confusion when they un/intentionally pass a floating-point vout and receive the error "JSON integer out of range."
If the error message is confusing, it would be a reason to improve the error message. However, this applies to all RPCs and changing a single RPC seems inconsistent and confusing.
💬 dergoegge commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2454986180)
I'm not sure if using the priority levels is a good alternative to deleting the benches. Afaict, the priority level was introduced to avoid running slow benchmarks in the CI as apposed to indicating how "important" they are. As a result, we're not catching issues with these tests until someone executes them (e.g. https://github.com/bitcoin/bitcoin/issues/29061#issuecomment-1852140217) and I'd prefer not to expand on that.
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2454986180)
I'm not sure if using the priority levels is a good alternative to deleting the benches. Afaict, the priority level was introduced to avoid running slow benchmarks in the CI as apposed to indicating how "important" they are. As a result, we're not catching issues with these tests until someone executes them (e.g. https://github.com/bitcoin/bitcoin/issues/29061#issuecomment-1852140217) and I'd prefer not to expand on that.
💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1827906033)
you're right, relatively harmless, but pushed a change which means it won't try package evaluation unless `txns_package_eval` is larger than size 1.
(https://github.com/bitcoin/bitcoin/pull/31096#discussion_r1827906033)
you're right, relatively harmless, but pushed a change which means it won't try package evaluation unless `txns_package_eval` is larger than size 1.
💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2455019735)
rebased on master due to test conflict
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2455019735)
rebased on master due to test conflict
💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827932382)
> Lol, that was a stupid mistake, lemme' try again:
i'm kind of hestitant to add a test like that because i'm not sure we want to require this behavior from the C++ library. It's perfectly valid to compile bitcoind with a library that doesn't do it, it should just take it into account.
> If you were to actually incorporate that into this PR, be aware you'll need to use !std::greater{}(a, b) instead of a <= b
pointer arithmetic is so scary nowadays...
Would this be safe?
```
bool da
...
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827932382)
> Lol, that was a stupid mistake, lemme' try again:
i'm kind of hestitant to add a test like that because i'm not sure we want to require this behavior from the C++ library. It's perfectly valid to compile bitcoind with a library that doesn't do it, it should just take it into account.
> If you were to actually incorporate that into this PR, be aware you'll need to use !std::greater{}(a, b) instead of a <= b
pointer arithmetic is so scary nowadays...
Would this be safe?
```
bool da
...
💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827934472)
Thanks for the hints, @sipa!
According to https://en.cppreference.com/w/cpp/utility/compare/compare_three_way, this also seems safe to use:
```c++
return (msg_start <=> s.data()) <= 0 && (s.data() <=> msg_end) <= 0;
```
or
```c++
return (msg_start <=> s.data()) != std::strong_ordering::greater
&& (s.data() <=> msg_end) != std::strong_ordering::greater;
```
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827934472)
Thanks for the hints, @sipa!
According to https://en.cppreference.com/w/cpp/utility/compare/compare_three_way, this also seems safe to use:
```c++
return (msg_start <=> s.data()) <= 0 && (s.data() <=> msg_end) <= 0;
```
or
```c++
return (msg_start <=> s.data()) != std::strong_ordering::greater
&& (s.data() <=> msg_end) != std::strong_ordering::greater;
```
💬 sipa commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827938447)
@laanwj My understanding is that that is correct (see https://en.cppreference.com/w/cpp/language/operator_comparison#Pointer_total_order; `std::greater{}` constructs an object of type `std::greater<void>`).
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827938447)
@laanwj My understanding is that that is correct (see https://en.cppreference.com/w/cpp/language/operator_comparison#Pointer_total_order; `std::greater{}` constructs an object of type `std::greater<void>`).