💬 glozow commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469604073)
Yes, calling `UpdateModifiedFee(0)` does nothing. I think it's fine to keep this :shrug:
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2469604073)
Yes, calling `UpdateModifiedFee(0)` does nothing. I think it's fine to keep this :shrug:
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2469981199)
> Isn't this comment already implied by `GUARDED_BY(m_mutex)`? Also it's written by the control thread and read by worker threads, so of course it must only be modified while holding the mutex. This comment doesn't add anything IMO.
What I meant there is that the variable shouldn’t be an atomic bool alone (in case someone propose to change it in the future). It must share the same mutex as the condition variable; otherwise, workers might not see the update. This is stated in the `std::conditi
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2469981199)
> Isn't this comment already implied by `GUARDED_BY(m_mutex)`? Also it's written by the control thread and read by worker threads, so of course it must only be modified while holding the mutex. This comment doesn't add anything IMO.
What I meant there is that the variable shouldn’t be an atomic bool alone (in case someone propose to change it in the future). It must share the same mutex as the condition variable; otherwise, workers might not see the update. This is stated in the `std::conditi
...
💬 fanquake commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2469984562)
Given the upstream fix has landed (and was possibly backported?), this workaround should either be removed entirely, or updated to indicate the version of Clang which this was fixed in.
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2469984562)
Given the upstream fix has landed (and was possibly backported?), this workaround should either be removed entirely, or updated to indicate the version of Clang which this was fixed in.
💬 ajtowns commented on pull request "p2p: reduce false-positives in addr rate-limiting":
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3457076576)
> On some occasions, a peer will send us addresses a few seconds after the connection is established.
What's the drawback of ignoring these addresses? If we'd connected a few seconds/a minute later, we'd have missed them anyway.
An advantage of starting with an (almost) empty bucket is that someone attempting an address spamming attack has to pay for that attack by maintaining a connection to us. Increasing the initial bucket for outbound/manual connections but not inbound connections migh
...
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3457076576)
> On some occasions, a peer will send us addresses a few seconds after the connection is established.
What's the drawback of ignoring these addresses? If we'd connected a few seconds/a minute later, we'd have missed them anyway.
An advantage of starting with an (almost) empty bucket is that someone attempting an address spamming attack has to pay for that attack by maintaining a connection to us. Increasing the initial bucket for outbound/manual connections but not inbound connections migh
...
💬 ajtowns commented on pull request "refactor: optimize: avoid allocations in script & policy verification":
(https://github.com/bitcoin/bitcoin/pull/33645#discussion_r2470028771)
Would be better to have `IsWitnessProgram()` populate a span rather than a vector if we're trying to minimise allocations, as far as I can see.
(https://github.com/bitcoin/bitcoin/pull/33645#discussion_r2470028771)
Would be better to have `IsWitnessProgram()` populate a span rather than a vector if we're trying to minimise allocations, as far as I can see.
💬 fanquake commented on pull request "build: Remove CMAKE_SKIP_BUILD_RPATH and SKIP_BUILD_RPATH settings":
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2470035274)
This is turning `CMAKE_SKIP_BUILD_RPATH` into `CMAKE_SKIP_INSTALL_RPATH`?
(https://github.com/bitcoin/bitcoin/pull/33247#discussion_r2470035274)
This is turning `CMAKE_SKIP_BUILD_RPATH` into `CMAKE_SKIP_INSTALL_RPATH`?
💬 furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470045034)
> I'm unclear what the purpose of this function is. It interrupts the workers, like `Stop`, but doesn't join them or wait for them to finish.
> If the idea is to clear the workqueue and park the threads without being able to add more tasks, it does not accomplish that since the threads will exit their loops as well and not be parked on the cv.
Actually, the goal is to keep the current shutdown semantics unchanged. The process already follows two stages: first, we stop accepting new requests
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470045034)
> I'm unclear what the purpose of this function is. It interrupts the workers, like `Stop`, but doesn't join them or wait for them to finish.
> If the idea is to clear the workqueue and park the threads without being able to add more tasks, it does not accomplish that since the threads will exit their loops as well and not be parked on the cv.
Actually, the goal is to keep the current shutdown semantics unchanged. The process already follows two stages: first, we stop accepting new requests
...
💬 fanquake commented on pull request "guix: build for Linux HOSTS with `-static-libgcc`":
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3457121575)
Guix Build (aarch64):
```bash
8e4d9f4db54d26c8720b4f4031f83105f81cab36ede446cf31e1e17a302cc062 guix-build-a99bbec14500/output/aarch64-linux-gnu/SHA256SUMS.part
ae43ea7ec85c4929843a2c1edfa1df3b68ad9f991c23630590aa18e6de36ebd9 guix-build-a99bbec14500/output/aarch64-linux-gnu/bitcoin-a99bbec14500-aarch64-linux-gnu-debug.tar.gz
b75993d29f48a6e820d638067648a6b9e4d76a23848c640523365b379ea64b01 guix-build-a99bbec14500/output/aarch64-linux-gnu/bitcoin-a99bbec14500-aarch64-linux-gnu.tar.gz
7a582f
...
(https://github.com/bitcoin/bitcoin/pull/33181#issuecomment-3457121575)
Guix Build (aarch64):
```bash
8e4d9f4db54d26c8720b4f4031f83105f81cab36ede446cf31e1e17a302cc062 guix-build-a99bbec14500/output/aarch64-linux-gnu/SHA256SUMS.part
ae43ea7ec85c4929843a2c1edfa1df3b68ad9f991c23630590aa18e6de36ebd9 guix-build-a99bbec14500/output/aarch64-linux-gnu/bitcoin-a99bbec14500-aarch64-linux-gnu-debug.tar.gz
b75993d29f48a6e820d638067648a6b9e4d76a23848c640523365b379ea64b01 guix-build-a99bbec14500/output/aarch64-linux-gnu/bitcoin-a99bbec14500-aarch64-linux-gnu.tar.gz
7a582f
...
💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2470046855)
The comment has been updated.
> and was possibly backported?
No, it wasn't.
(https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2470046855)
The comment has been updated.
> and was possibly backported?
No, it wasn't.
💬 hebasto commented on pull request "ci: Update Clang in "tidy" job":
(https://github.com/bitcoin/bitcoin/pull/33445#issuecomment-3457125965)
Rebased and addressed [feedback](https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2469984562) from @fanquake.
(https://github.com/bitcoin/bitcoin/pull/33445#issuecomment-3457125965)
Rebased and addressed [feedback](https://github.com/bitcoin/bitcoin/pull/33445#discussion_r2469984562) from @fanquake.
💬 Raimo33 commented on pull request "refactor: optimize: avoid allocations in script & policy verification":
(https://github.com/bitcoin/bitcoin/pull/33645#discussion_r2470058888)
here the vector is totally useless. it's required by the `IsWitnessProgram()` API but not actually needed. I've made more significant changes on this private branch, for another PR: https://github.com/Raimo33/bitcoinknots/commits/optimize-tx-policy-verification-2/
(https://github.com/bitcoin/bitcoin/pull/33645#discussion_r2470058888)
here the vector is totally useless. it's required by the `IsWitnessProgram()` API but not actually needed. I've made more significant changes on this private branch, for another PR: https://github.com/Raimo33/bitcoinknots/commits/optimize-tx-policy-verification-2/
🤔 glozow reviewed a pull request: "doc: mention key removal in rpc interface modification"
(https://github.com/bitcoin/bitcoin/pull/32867#pullrequestreview-3389486508)
lgtm ACK 944e5ff848f656d2ee6202b2330f3ae178ad0fbe
(https://github.com/bitcoin/bitcoin/pull/32867#pullrequestreview-3389486508)
lgtm ACK 944e5ff848f656d2ee6202b2330f3ae178ad0fbe
🚀 glozow merged a pull request: "doc: mention key removal in rpc interface modification"
(https://github.com/bitcoin/bitcoin/pull/32867)
(https://github.com/bitcoin/bitcoin/pull/32867)
💬 mzumsande commented on pull request "p2p: reduce false-positives in addr rate-limiting":
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3457197373)
> What's the drawback of ignoring these addresses? If we'd connected a few seconds/a minute later, we'd have missed them anyway.
Ignoring the initial self-announcement of the peer if it get mixed up with other addrs would feel strange.
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3457197373)
> What's the drawback of ignoring these addresses? If we'd connected a few seconds/a minute later, we'd have missed them anyway.
Ignoring the initial self-announcement of the peer if it get mixed up with other addrs would feel strange.
💬 andrewtoth commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470109463)
I see, so the goal is to stop the thread pool but in a non-blocking way. Could the same be done via a bool parameter to `Stop` to skip joining?
Also, could we modify `Start` in that case to not assert on number of threads, and instead join them if `m_interrupt` is true and we still have outstanding threads?
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470109463)
I see, so the goal is to stop the thread pool but in a non-blocking way. Could the same be done via a bool parameter to `Stop` to skip joining?
Also, could we modify `Start` in that case to not assert on number of threads, and instead join them if `m_interrupt` is true and we still have outstanding threads?
📝 SatsAndSports opened a pull request: "Remove unnecessary seed from chainparams.cpp"
(https://github.com/bitcoin/bitcoin/pull/33723)
New Bitcoin nodes don't gain anything by connecting to `dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us.` any more
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user e
...
(https://github.com/bitcoin/bitcoin/pull/33723)
New Bitcoin nodes don't gain anything by connecting to `dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us.` any more
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user e
...
💬 glozow commented on pull request "refactor: unify container presence checks":
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3457210211)
As mentioned previously, I don't think we should make people rebase for this change, particularly if it's just for style. Maybe reduce the conflicts or close the PR?
(https://github.com/bitcoin/bitcoin/pull/33192#issuecomment-3457210211)
As mentioned previously, I don't think we should make people rebase for this change, particularly if it's just for style. Maybe reduce the conflicts or close the PR?
📝 maflcko opened a pull request: " refactor: Return uint64_t from GetSerializeSize "
(https://github.com/bitcoin/bitcoin/pull/33724)
Consensus code should arrive at the same conclusion, regardless of the architecture it runs on. Using architecture-specific types such as `size_t` can lead to issues, such as the low-severity [CVE-2025-46597](https://bitcoincore.org/en/2025/10/24/disclose-cve-2025-46597/).
The CVE was already worked around, but it may be good to still fix the underlying issue.
Fixes https://github.com/bitcoin/bitcoin/issues/33709 with a few refactors to use explicit fixed-sized integer types in serializati
...
(https://github.com/bitcoin/bitcoin/pull/33724)
Consensus code should arrive at the same conclusion, regardless of the architecture it runs on. Using architecture-specific types such as `size_t` can lead to issues, such as the low-severity [CVE-2025-46597](https://bitcoincore.org/en/2025/10/24/disclose-cve-2025-46597/).
The CVE was already worked around, but it may be good to still fix the underlying issue.
Fixes https://github.com/bitcoin/bitcoin/issues/33709 with a few refactors to use explicit fixed-sized integer types in serializati
...
💬 m3dwards commented on issue "ci: Where to run heavy and fragile CI tasks?":
(https://github.com/bitcoin/bitcoin/issues/33668#issuecomment-3457222086)
> They could be run on just the master branch for every merge
I like this idea for things that are expensive (beyond what can be solved with more CPU / Memory). As for fragile, I'm not sure I like the idea of just running fragile things less over running them regularly to get more data on why they are fragile and hopefully fixing that. That said, fixing fragile CI jobs often takes significant development effort and you can often be left wondering if it was worth the investment over disabling th
...
(https://github.com/bitcoin/bitcoin/issues/33668#issuecomment-3457222086)
> They could be run on just the master branch for every merge
I like this idea for things that are expensive (beyond what can be solved with more CPU / Memory). As for fragile, I'm not sure I like the idea of just running fragile things less over running them regularly to get more data on why they are fragile and hopefully fixing that. That said, fixing fragile CI jobs often takes significant development effort and you can often be left wondering if it was worth the investment over disabling th
...
🚀 glozow merged a pull request: "refactor: rename `fees.{h,cpp}` to `fees/block_policy_estimator.{h,cpp}`"
(https://github.com/bitcoin/bitcoin/pull/33218)
(https://github.com/bitcoin/bitcoin/pull/33218)