💬 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>`).
💬 marcofleon commented on pull request "fuzz: Fix difficulty target generation in `p2p_headers_presync`":
(https://github.com/bitcoin/bitcoin/pull/31213#issuecomment-2455038583)
> To be clear, `nBits` is "deterministic" in that the value, given the chain history, is static, right?
Hmm I'm not entirely sure what you're asking. `nBits` does always start with the genesis block value, yes. Then it'll either change or stay the same according to the fuzzer. Is that what you meant?
(https://github.com/bitcoin/bitcoin/pull/31213#issuecomment-2455038583)
> To be clear, `nBits` is "deterministic" in that the value, given the chain history, is static, right?
Hmm I'm not entirely sure what you're asking. `nBits` does always start with the genesis block value, yes. Then it'll either change or stay the same according to the fuzzer. Is that what you meant?
💬 l0rinc commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827943096)
> 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.
I understood the memory calculations will be incorrect if this would fail - is that not the case?
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827943096)
> 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.
I understood the memory calculations will be incorrect if this would fail - is that not the case?
💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827945717)
https://en.cppreference.com/w/cpp/container/vector uses
```
template<
class T,
class Allocator = [std::allocator](http://en.cppreference.com/w/cpp/memory/allocator)<T>
> class vector
```
so let's go with those names
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827945717)
https://en.cppreference.com/w/cpp/container/vector uses
```
template<
class T,
class Allocator = [std::allocator](http://en.cppreference.com/w/cpp/memory/allocator)<T>
> class vector
```
so let's go with those names
💬 maflcko commented on issue "intermittent issue in wallet_upgradewallet.py: AssertionError: bdb magic does not match bdb btree magic":
(https://github.com/bitcoin/bitcoin/issues/31210#issuecomment-2455067581)
Steps to reproduce on a fresh Ubuntu 24.04 on that machine:
`export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./b-c && cd b-c && apt install build-essential cmake pkg-config libevent-dev libboost-dev libsqlite3-dev libdb++-dev python3 -y && cmake -B ./bld-cmake -DBUILD_TESTS=OFF -DWITH_BDB=ON -DENABLE_WALLET=ON -DBUILD_GUI=OFF -DBUILD_FUZZ_BINARY=OFF -DBUILD_BENCH=OFF -DWIT
...
(https://github.com/bitcoin/bitcoin/issues/31210#issuecomment-2455067581)
Steps to reproduce on a fresh Ubuntu 24.04 on that machine:
`export DEBIAN_FRONTEND=noninteractive && apt update && apt install curl wget htop git vim ccache -y && git clone https://github.com/bitcoin/bitcoin.git --depth=1 ./b-c && cd b-c && apt install build-essential cmake pkg-config libevent-dev libboost-dev libsqlite3-dev libdb++-dev python3 -y && cmake -B ./bld-cmake -DBUILD_TESTS=OFF -DWITH_BDB=ON -DENABLE_WALLET=ON -DBUILD_GUI=OFF -DBUILD_FUZZ_BINARY=OFF -DBUILD_BENCH=OFF -DWIT
...
💬 laanwj commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827968959)
> I understood the memory calculations will be incorrect if this would fail - is that not the case?
Yes, unless we add the logic to `memusage.h`. i guess i've always seen memusage more as a "best effort estimate" than something that needs to be correct to the byte, because it's so hard to guarantee across implementations, we don't really want to extend our scope to C++ library implementation specifics. But i dunno.
BTW: It looks like currently. memusage.h doesn't handle `std::string` (nor
...
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827968959)
> I understood the memory calculations will be incorrect if this would fail - is that not the case?
Yes, unless we add the logic to `memusage.h`. i guess i've always seen memusage more as a "best effort estimate" than something that needs to be correct to the byte, because it's so hard to guarantee across implementations, we don't really want to extend our scope to C++ library implementation specifics. But i dunno.
BTW: It looks like currently. memusage.h doesn't handle `std::string` (nor
...
📝 hebasto opened a pull request: "guix: Do not set `C{PLUS}_INCLUDE_PATH` variables for Darwin builds"
(https://github.com/bitcoin/bitcoin/pull/31214)
When cross-building for macOS, the `C{PLUS}_INCLUDE_PATH` environment variables modify the default include directories for both native GCC and cross Clang compilers, which is overkill and undesirable for the latter.
This change avoids setting the `C{PLUS}_INCLUDE_PATH` environment variables and instead sets the required `-isystem` flags for the native compiler explicitly.
Additionally, this PR helps avoid non-trivial [patching](https://github.com/bitcoin/bitcoin/blob/add3067e766db6f6732836
...
(https://github.com/bitcoin/bitcoin/pull/31214)
When cross-building for macOS, the `C{PLUS}_INCLUDE_PATH` environment variables modify the default include directories for both native GCC and cross Clang compilers, which is overkill and undesirable for the latter.
This change avoids setting the `C{PLUS}_INCLUDE_PATH` environment variables and instead sets the required `-isystem` flags for the native compiler explicitly.
Additionally, this PR helps avoid non-trivial [patching](https://github.com/bitcoin/bitcoin/blob/add3067e766db6f6732836
...
💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1827978920)
Please see https://github.com/bitcoin/bitcoin/pull/31214 as a more clean (as I believe) alternative.
(https://github.com/bitcoin/bitcoin/pull/30997#discussion_r1827978920)
Please see https://github.com/bitcoin/bitcoin/pull/31214 as a more clean (as I believe) alternative.
💬 Abdulkbk commented on pull request "Improve lockunspent validation for vout":
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2455105308)
> > 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.
In that case I will convert this PR to draft and work on figuring a way to
...
(https://github.com/bitcoin/bitcoin/pull/31205#issuecomment-2455105308)
> > 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.
In that case I will convert this PR to draft and work on figuring a way to
...
💬 fanquake commented on pull request "guix: Do not set `C{PLUS}_INCLUDE_PATH` variables for Darwin builds":
(https://github.com/bitcoin/bitcoin/pull/31214#issuecomment-2455116834)
This doesn't Guix build for Darwin:
```bash
INFO: Building 9caf3c3fe59c for platform triple arm64-apple-darwin:
...using reference timestamp: 1730735608
...running at most 10 jobs
...from worktree directory: '/bitcoin'
...bind-mounted in container to: '/bitcoin'
...in build directory: '/bitcoin/guix-build-9caf3c3fe59c/distsrc-9caf3c3fe59c-arm64-apple-darwin'
...bind-mounted in container to: '/distsrc-base/distsrc-9caf3c3fe59c-arm64-apple-darwin
...
(https://github.com/bitcoin/bitcoin/pull/31214#issuecomment-2455116834)
This doesn't Guix build for Darwin:
```bash
INFO: Building 9caf3c3fe59c for platform triple arm64-apple-darwin:
...using reference timestamp: 1730735608
...running at most 10 jobs
...from worktree directory: '/bitcoin'
...bind-mounted in container to: '/bitcoin'
...in build directory: '/bitcoin/guix-build-9caf3c3fe59c/distsrc-9caf3c3fe59c-arm64-apple-darwin'
...bind-mounted in container to: '/distsrc-base/distsrc-9caf3c3fe59c-arm64-apple-darwin
...
📝 hebasto converted_to_draft a pull request: "guix: Do not set `C{PLUS}_INCLUDE_PATH` variables for Darwin builds"
(https://github.com/bitcoin/bitcoin/pull/31214)
When cross-building for macOS, the `C{PLUS}_INCLUDE_PATH` environment variables modify the default include directories for both native GCC and cross Clang compilers, which is overkill and undesirable for the latter.
This change avoids setting the `C{PLUS}_INCLUDE_PATH` environment variables and instead sets the required `-isystem` flags for the native compiler explicitly.
Additionally, this PR helps avoid non-trivial [patching](https://github.com/bitcoin/bitcoin/blob/add3067e766db6f6732836
...
(https://github.com/bitcoin/bitcoin/pull/31214)
When cross-building for macOS, the `C{PLUS}_INCLUDE_PATH` environment variables modify the default include directories for both native GCC and cross Clang compilers, which is overkill and undesirable for the latter.
This change avoids setting the `C{PLUS}_INCLUDE_PATH` environment variables and instead sets the required `-isystem` flags for the native compiler explicitly.
Additionally, this PR helps avoid non-trivial [patching](https://github.com/bitcoin/bitcoin/blob/add3067e766db6f6732836
...
💬 sipa commented on pull request "net: Use actual memory size in receive buffer accounting":
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827996590)
All of `memusage.h` is essentially a best effort to model the standard library. There are inevitably limitations to how good it can be.
My suggestion would be to actually add a `DynamicUsage(const std::string&)` to memusage. That's far more obviously correct than just using it in a test, and just as much work. If it happens to be wrong, it won't be more wrong than what we had before.
(https://github.com/bitcoin/bitcoin/pull/31164#discussion_r1827996590)
All of `memusage.h` is essentially a best effort to model the standard library. There are inevitably limitations to how good it can be.
My suggestion would be to actually add a `DynamicUsage(const std::string&)` to memusage. That's far more obviously correct than just using it in a test, and just as much work. If it happens to be wrong, it won't be more wrong than what we had before.
📝 vasild opened a pull request: "rpc: increase the defaults for -rpcthreads and -rpcworkqueue"
(https://github.com/bitcoin/bitcoin/pull/31215)
`rpcthreads` was introduced with a default of 4 in 2013 in 21eb5adadbe3110a8708f2570185566e1f137a49
`rpcworkqueue` was introduced with a default of 16 in 2015 in 40b556d3742a1f65d67e2d4c760d0b13fe8be5b7
Resolves: https://github.com/bitcoin/bitcoin/issues/29386
---
Just bump the ancient default values. There is no perfect default that would fit everybody. This could lead to https://bikeshed.com/
(https://github.com/bitcoin/bitcoin/pull/31215)
`rpcthreads` was introduced with a default of 4 in 2013 in 21eb5adadbe3110a8708f2570185566e1f137a49
`rpcworkqueue` was introduced with a default of 16 in 2015 in 40b556d3742a1f65d67e2d4c760d0b13fe8be5b7
Resolves: https://github.com/bitcoin/bitcoin/issues/29386
---
Just bump the ancient default values. There is no perfect default that would fit everybody. This could lead to https://bikeshed.com/
💬 vasild commented on issue "Default rpcthreads value (4) is too small":
(https://github.com/bitcoin/bitcoin/issues/29386#issuecomment-2455129219)
See https://github.com/bitcoin/bitcoin/pull/31215
(https://github.com/bitcoin/bitcoin/issues/29386#issuecomment-2455129219)
See https://github.com/bitcoin/bitcoin/pull/31215
👍 hodlinator approved a pull request: "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues"
(https://github.com/bitcoin/bitcoin/pull/30349#pullrequestreview-2413503236)
ACK 42066f45ff5d48e78a317eda63c035809bd657c6
> 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.
Ah, that makes sense that the CPU/compiler wasn't able to do as much out-of-order execution previously because of reusing the output from the last ru
...
(https://github.com/bitcoin/bitcoin/pull/30349#pullrequestreview-2413503236)
ACK 42066f45ff5d48e78a317eda63c035809bd657c6
> 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.
Ah, that makes sense that the CPU/compiler wasn't able to do as much out-of-order execution previously because of reusing the output from the last ru
...
💬 hebasto commented on pull request "guix: Do not set `C{PLUS}_INCLUDE_PATH` variables for Darwin builds":
(https://github.com/bitcoin/bitcoin/pull/31214#issuecomment-2455144517)
> This doesn't Guix build for Darwin:
Right. The current `qt.mk` does not use `build_{CC,CXX}` variables. This patch is meaningful only in https://github.com/bitcoin/bitcoin/pull/30997.
(https://github.com/bitcoin/bitcoin/pull/31214#issuecomment-2455144517)
> This doesn't Guix build for Darwin:
Right. The current `qt.mk` does not use `build_{CC,CXX}` variables. This patch is meaningful only in https://github.com/bitcoin/bitcoin/pull/30997.