π¬ hebasto commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470853688)
You're right.
Reworked.
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470853688)
You're right.
Reworked.
π¬ hebasto commented on pull request "random: scope the environ extern to macOS":
(https://github.com/bitcoin/bitcoin/pull/33714#issuecomment-3458248055)
Testing on some other platforms: https://github.com/hebasto/bitcoin-core-nightly/pull/79.
(https://github.com/bitcoin/bitcoin/pull/33714#issuecomment-3458248055)
Testing on some other platforms: https://github.com/hebasto/bitcoin-core-nightly/pull/79.
π¬ frodrik-froggo commented on pull request "Remove unnecessary seed from chainparams.cpp":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458279276)
ACK
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458279276)
ACK
π maflcko approved a pull request: "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`"
(https://github.com/bitcoin/bitcoin/pull/33725#pullrequestreview-3390598306)
review ACK 8ae41ae6f2c2ccc129a84f8f2cc8ae415e2125e0 π
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 8ae41ae6f2c2
...
(https://github.com/bitcoin/bitcoin/pull/33725#pullrequestreview-3390598306)
review ACK 8ae41ae6f2c2ccc129a84f8f2cc8ae415e2125e0 π
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 8ae41ae6f2c2
...
π¬ maflcko commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470891624)
nit in 8ae41ae6f2c2ccc129a84f8f2cc8ae415e2125e0: Same here: Doesn't matter much, but the benefit of including span, when span.h is included, seems limited.
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470891624)
nit in 8ae41ae6f2c2ccc129a84f8f2cc8ae415e2125e0: Same here: Doesn't matter much, but the benefit of including span, when span.h is included, seems limited.
π¬ furszy commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470902222)
> So why have `Start` and `Stop` then? Just start threads in constructor and join in destructor? Use `Interrupt` to stop new tasks from being added. This follows RAII principles.
Good question. We could easily build an RAII wrapper on top of this. The two approaches arenβt conflicting; it would just be an additional layer over the current code.
Yet, I think the current design smoothly integrates with our existing workflows. We already use the `start`, `interrupt`, and `stop` terms throughout
...
(https://github.com/bitcoin/bitcoin/pull/33689#discussion_r2470902222)
> So why have `Start` and `Stop` then? Just start threads in constructor and join in destructor? Use `Interrupt` to stop new tasks from being added. This follows RAII principles.
Good question. We could easily build an RAII wrapper on top of this. The two approaches arenβt conflicting; it would just be an additional layer over the current code.
Yet, I think the current design smoothly integrates with our existing workflows. We already use the `start`, `interrupt`, and `stop` terms throughout
...
π¬ glozow commented on pull request "p2p: reduce false-positives in addr rate-limiting":
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3458290468)
If these are Bitcoin Core nodes, is an alternative approach to increase the delay after which we'd send addrs?
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3458290468)
If these are Bitcoin Core nodes, is an alternative approach to increase the delay after which we'd send addrs?
π¬ hebasto commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470919585)
Right. But let me leave this change for a follow-up, as it would touch files in `src/crypto`.
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470919585)
Right. But let me leave this change for a follow-up, as it would touch files in `src/crypto`.
π¬ davidgumberg commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#issuecomment-3458345585)
crACK https://github.com/bitcoin/bitcoin/pull/33677/commits/5555bce994b648f836c35a02570f22ae9ad36da3
Concept ACK on moving the CI script from bash to python and crACK https://github.com/bitcoin/bitcoin/pull/33677/commits/fabe516440c96bb7a6a6902195684d3802d64139 and https://github.com/bitcoin/bitcoin/pull/33677/commits/5555bce994b648f836c35a02570f22ae9ad36da3 as refactors with no behavior change, but those changes seem orthogonal to the PR description and linked issue and I think they would i
...
(https://github.com/bitcoin/bitcoin/pull/33677#issuecomment-3458345585)
crACK https://github.com/bitcoin/bitcoin/pull/33677/commits/5555bce994b648f836c35a02570f22ae9ad36da3
Concept ACK on moving the CI script from bash to python and crACK https://github.com/bitcoin/bitcoin/pull/33677/commits/fabe516440c96bb7a6a6902195684d3802d64139 and https://github.com/bitcoin/bitcoin/pull/33677/commits/5555bce994b648f836c35a02570f22ae9ad36da3 as refactors with no behavior change, but those changes seem orthogonal to the PR description and linked issue and I think they would i
...
π¬ hebasto commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470959705)
Thanks! Reworked.
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2470959705)
Thanks! Reworked.
π¬ SatsAndSports commented on pull request "Remove unnecessary seed from chainparams.cpp":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458378187)
I've updated the description now, linking to some of the evidence provided by others in this thread, and mentioning the policy that you referenced. I'm not an expert at writing PRs, I hope my updated description is concise and helpful
> @SatsAndSports it would be helpful if you could update the PR description with some of the more specific motivations.
@davidgumberg , thanks for pointing out the README change. I've updated it now too in this PR
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458378187)
I've updated the description now, linking to some of the evidence provided by others in this thread, and mentioning the policy that you referenced. I'm not an expert at writing PRs, I hope my updated description is concise and helpful
> @SatsAndSports it would be helpful if you could update the PR description with some of the more specific motivations.
@davidgumberg , thanks for pointing out the README change. I've updated it now too in this PR
π¬ TheCharlatan commented on pull request "build: Introduce internal kernel library":
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-3458382072)
Rebased 78ed83ba337c97798134f4bd0f9307facde3a59d -> a9ec7cdc0a1f6d4a3bd532649e79459074ed9f3b ([kernelInternalLib_20](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_20) -> [kernelInternalLib_21](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_21), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelInternalLib_20..kernelInternalLib_21))
* Fixed conflict with #33218
(https://github.com/bitcoin/bitcoin/pull/28690#issuecomment-3458382072)
Rebased 78ed83ba337c97798134f4bd0f9307facde3a59d -> a9ec7cdc0a1f6d4a3bd532649e79459074ed9f3b ([kernelInternalLib_20](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_20) -> [kernelInternalLib_21](https://github.com/TheCharlatan/bitcoin/tree/kernelInternalLib_21), [compare](https://github.com/TheCharlatan/bitcoin/compare/kernelInternalLib_20..kernelInternalLib_21))
* Fixed conflict with #33218
π¬ instagibbs commented on pull request "Cluster mempool":
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2450652757)
there's a single lasting reference to "package-mempool-limits" in the comment in this file
(https://github.com/bitcoin/bitcoin/pull/33629#discussion_r2450652757)
there's a single lasting reference to "package-mempool-limits" in the comment in this file
π¬ davidgumberg commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2470986340)
+1 to the way @instagibbs pointed out for honest peers sending unsolicited CMPCTBLOCK's making this a bad idea. And with this, any way now or in the future that you could trick a node into sending an unsolicited CMPCTBLOCK becomes an attack vector. Given that there is little an attacker could accomplish by sending us an unsolicited compact block if we drop unsolicited compact blocks on the ground, I think we don't gain much by marking them as misbehaving or disconnecting them.
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2470986340)
+1 to the way @instagibbs pointed out for honest peers sending unsolicited CMPCTBLOCK's making this a bad idea. And with this, any way now or in the future that you could trick a node into sending an unsolicited CMPCTBLOCK becomes an attack vector. Given that there is little an attacker could accomplish by sending us an unsolicited compact block if we drop unsolicited compact blocks on the ground, I think we don't gain much by marking them as misbehaving or disconnecting them.
π¬ niteshbalusu11 commented on pull request "Remove unnecessary seed from chainparams.cpp":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458426762)
I ran a test a few times, my results.
<img width="1416" height="505" alt="Screenshot 2025-10-28 at 4 47 44 PM" src="https://github.com/user-attachments/assets/3bf09a9e-8132-4ac4-8198-b1687c7c4e69" />
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458426762)
I ran a test a few times, my results.
<img width="1416" height="505" alt="Screenshot 2025-10-28 at 4 47 44 PM" src="https://github.com/user-attachments/assets/3bf09a9e-8132-4ac4-8198-b1687c7c4e69" />
π¬ davidgumberg commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2471008758)
@ajtowns I'm not sure which suggestion you mean, I missed the suggestion to drop the newline, but in the current branch (5caaefd8d4174cfc27d5538e7f2d3a1c2bdb77d8) this is a `CMPCTBLOCK` message and IP's are logged: https://github.com/bitcoin/bitcoin/blob/5caaefd/src/net_processing.cpp#L2484
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2471008758)
@ajtowns I'm not sure which suggestion you mean, I missed the suggestion to drop the newline, but in the current branch (5caaefd8d4174cfc27d5538e7f2d3a1c2bdb77d8) this is a `CMPCTBLOCK` message and IP's are logged: https://github.com/bitcoin/bitcoin/blob/5caaefd/src/net_processing.cpp#L2484
π¬ onyxcoyote commented on pull request "Remove unnecessary seed from chainparams.cpp":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458447799)
There is another important reason why I ACK this...
Separation of duties, which is both a best practice, and even legally required in some industries (like banks).
Any one individual should not cross over multiple checks and balances. For example, they should not be able to develop, test, review, and deploy code to production (or really any 2-3 of those things). It does not matter if they are a rockstar developer and 100% trusted, or even if it could offend them. In my previous life I rem
...
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458447799)
There is another important reason why I ACK this...
Separation of duties, which is both a best practice, and even legally required in some industries (like banks).
Any one individual should not cross over multiple checks and balances. For example, they should not be able to develop, test, review, and deploy code to production (or really any 2-3 of those things). It does not matter if they are a rockstar developer and 100% trusted, or even if it could offend them. In my previous life I rem
...
π¬ ajtowns commented on pull request "p2p: Drop unsolicited CMPCTBLOCK from non-HB peer":
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2471037172)
```
$ git rev-parse HEAD
5caaefd8d4174cfc27d5538e7f2d3a1c2bdb77d8
$ grep not.marked net_processing.cpp
4416: LogDebug(BCLog::NET, "Peer %d, not marked as high-bandwidth, sent us an unsolicited compact block!\n", pfrom.GetId());
```
(https://github.com/bitcoin/bitcoin/pull/32606#discussion_r2471037172)
```
$ git rev-parse HEAD
5caaefd8d4174cfc27d5538e7f2d3a1c2bdb77d8
$ grep not.marked net_processing.cpp
4416: LogDebug(BCLog::NET, "Peer %d, not marked as high-bandwidth, sent us an unsolicited compact block!\n", pfrom.GetId());
```
π¬ SatsAndSports commented on pull request "Remove unnecessary seed from chainparams.cpp":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458485281)
Thanks @niteshbalusu11 and @john-moffett for your detailed breakdown of the versions. Can you share the code or system you used?
I just updated the description of this PR with a small script I made, with `dig` and `nmap`, also showing that recent versions are not represented in this seed. But it would be great to have your code and data described, as mine data is very basic
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3458485281)
Thanks @niteshbalusu11 and @john-moffett for your detailed breakdown of the versions. Can you share the code or system you used?
I just updated the description of this PR with a small script I made, with `dig` and `nmap`, also showing that recent versions are not represented in this seed. But it would be great to have your code and data described, as mine data is very basic
π¬ maflcko commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#issuecomment-3458487836)
> refactors with no behavior change, but those changes seem orthogonal to the PR title and linked issue and I think they would ideally be in a separate PR.
thx, i'll open a dedicated follow-up for the remaining refactors. Though, I'll leave this one as-is for now, as it already has 3 acks and seems rfm.
(https://github.com/bitcoin/bitcoin/pull/33677#issuecomment-3458487836)
> refactors with no behavior change, but those changes seem orthogonal to the PR title and linked issue and I think they would ideally be in a separate PR.
thx, i'll open a dedicated follow-up for the remaining refactors. Though, I'll leave this one as-is for now, as it already has 3 acks and seems rfm.