💬 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.
✅ hebasto closed a pull request: "guix: Do not set `C{PLUS}_INCLUDE_PATH` variables for Darwin builds"
(https://github.com/bitcoin/bitcoin/pull/31214)
(https://github.com/bitcoin/bitcoin/pull/31214)
💬 instagibbs commented on pull request "fuzz: Fix difficulty target generation in `p2p_headers_presync`":
(https://github.com/bitcoin/bitcoin/pull/31213#issuecomment-2455155318)
@marcofleon Ah, I didn't understand how pre-sync nBits checking works, my question makes no sense for pre-sync
(https://github.com/bitcoin/bitcoin/pull/31213#issuecomment-2455155318)
@marcofleon Ah, I didn't understand how pre-sync nBits checking works, my question makes no sense for pre-sync
🤔 murchandamus reviewed a pull request: "Remove mempoolfullrbf"
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2413508572)
ACK c189eec848e3c31f438151d4d3422718a29df3a3
(https://github.com/bitcoin/bitcoin/pull/30592#pullrequestreview-2413508572)
ACK c189eec848e3c31f438151d4d3422718a29df3a3
💬 murchandamus commented on pull request "Remove mempoolfullrbf":
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1828002169)
This is a bit confusing, because defaulting to `mempoolfullrbf=1` already made full replace-by-fee the standard behavior, but this reads as if removing the option makes it the standard behavior.
Perhaps something like this:
```suggestion
After the policy was broadly adopted by miners, starting with v28.0, the `mempoolfullrbf` startup option was set to
default to `1`. With widespread adoption of full replace-by-fee, it becomes the standard behavior on the network. Users no longer benefit f
...
(https://github.com/bitcoin/bitcoin/pull/30592#discussion_r1828002169)
This is a bit confusing, because defaulting to `mempoolfullrbf=1` already made full replace-by-fee the standard behavior, but this reads as if removing the option makes it the standard behavior.
Perhaps something like this:
```suggestion
After the policy was broadly adopted by miners, starting with v28.0, the `mempoolfullrbf` startup option was set to
default to `1`. With widespread adoption of full replace-by-fee, it becomes the standard behavior on the network. Users no longer benefit f
...
💬 dergoegge commented on pull request "rpc: increase the defaults for -rpcthreads and -rpcworkqueue":
(https://github.com/bitcoin/bitcoin/pull/31215#discussion_r1828023493)
Maybe we should just make the default depend on how many cores are available? e.g. default: `num cores / 2`
(https://github.com/bitcoin/bitcoin/pull/31215#discussion_r1828023493)
Maybe we should just make the default depend on how many cores are available? e.g. default: `num cores / 2`
💬 andrewtoth commented on pull request "rpc: increase the defaults for -rpcthreads and -rpcworkqueue":
(https://github.com/bitcoin/bitcoin/pull/31215#discussion_r1828025349)
From the linked issue, the user is saying the default of 4 is not enough for a raspi, which has 4 cores. So this would lower that number on a raspi.
(https://github.com/bitcoin/bitcoin/pull/31215#discussion_r1828025349)
From the linked issue, the user is saying the default of 4 is not enough for a raspi, which has 4 cores. So this would lower that number on a raspi.
💬 dergoegge commented on pull request "rpc: increase the defaults for -rpcthreads and -rpcworkqueue":
(https://github.com/bitcoin/bitcoin/pull/31215#discussion_r1828042992)
I see, at least something based on the users system instead of picking a random number?
(https://github.com/bitcoin/bitcoin/pull/31215#discussion_r1828042992)
I see, at least something based on the users system instead of picking a random number?
💬 andrewtoth commented on pull request "rpc: increase the defaults for -rpcthreads and -rpcworkqueue":
(https://github.com/bitcoin/bitcoin/pull/31215#discussion_r1828046467)
Just number of cores would make sense as a default. At least it wouldn't be lower on raspis.
(https://github.com/bitcoin/bitcoin/pull/31215#discussion_r1828046467)
Just number of cores would make sense as a default. At least it wouldn't be lower on raspis.
💬 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-2455258749)
Possibly related: https://github.com/containers/podman/issues/23808
I'll try tomorrow without a container.
(https://github.com/bitcoin/bitcoin/issues/31210#issuecomment-2455258749)
Possibly related: https://github.com/containers/podman/issues/23808
I'll try tomorrow without a container.
💬 laanwj commented on pull request "rpc: increase the defaults for -rpcthreads and -rpcworkqueue":
(https://github.com/bitcoin/bitcoin/pull/31215#discussion_r1828088319)
i've always been kind of hestitant about adapting these values to the system automatically; it doesn't only depend on the CPU, but also the expected usage.
For a "fair" work queue it would indeed make sense to pick the number of CPU cores. However, bitcoin core's RPC threads often sleep for various reasons. Eg waiting for I/O, waiting for a lock in another thread, etc. So it's fine to have it a bit higher than the number of cores.
i'd definitely cap the minimum at 4, even for single-core CPU
...
(https://github.com/bitcoin/bitcoin/pull/31215#discussion_r1828088319)
i've always been kind of hestitant about adapting these values to the system automatically; it doesn't only depend on the CPU, but also the expected usage.
For a "fair" work queue it would indeed make sense to pick the number of CPU cores. However, bitcoin core's RPC threads often sleep for various reasons. Eg waiting for I/O, waiting for a lock in another thread, etc. So it's fine to have it a bit higher than the number of cores.
i'd definitely cap the minimum at 4, even for single-core CPU
...
👍 storopoli approved a pull request: "rpc: increase the defaults for -rpcthreads and -rpcworkqueue"
(https://github.com/bitcoin/bitcoin/pull/31215#pullrequestreview-2413651250)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31215#pullrequestreview-2413651250)
Concept ACK
💬 laanwj commented on pull request "rpc: increase the defaults for -rpcthreads and -rpcworkqueue":
(https://github.com/bitcoin/bitcoin/pull/31215#issuecomment-2455273805)
ACK on increasing the default work queue size. i don't think the RPC work queue uses that much memory. It's fine to set the default a lot higher.
Not sure about the threads.
(https://github.com/bitcoin/bitcoin/pull/31215#issuecomment-2455273805)
ACK on increasing the default work queue size. i don't think the RPC work queue uses that much memory. It's fine to set the default a lot higher.
Not sure about the threads.