💬 hebasto commented on pull request "random: scope the environ extern to macOS":
(https://github.com/bitcoin/bitcoin/pull/33714#issuecomment-3459211092)
> Testing on some other platforms: [hebasto/bitcoin-core-nightly#79](https://github.com/hebasto/bitcoin-core-nightly/pull/79).
Builds on the *BSD and illumos systems failed.
(https://github.com/bitcoin/bitcoin/pull/33714#issuecomment-3459211092)
> Testing on some other platforms: [hebasto/bitcoin-core-nightly#79](https://github.com/hebasto/bitcoin-core-nightly/pull/79).
Builds on the *BSD and illumos systems failed.
💬 hebasto commented on pull request "guix: Build for macOS using Clang only":
(https://github.com/bitcoin/bitcoin/pull/32764#issuecomment-3459238801)
My Guix build:
```
aarch64
a8613aaa30cc598f8f193932478e57381299399fb1a6a3e8cd27d41ed58e1a2c guix-build-7c97bae595b0/output/aarch64-linux-gnu/SHA256SUMS.part
6b32bd6a25e2c8b2823efc9deb8d9dc1fd4da7713dc00992ee721171500ebbf0 guix-build-7c97bae595b0/output/aarch64-linux-gnu/bitcoin-7c97bae595b0-aarch64-linux-gnu-debug.tar.gz
fbda951a67c0b02a9c4ed8014b3371ecacf7c41f2f0338653963c2f765e29f7f guix-build-7c97bae595b0/output/aarch64-linux-gnu/bitcoin-7c97bae595b0-aarch64-linux-gnu.tar.gz
d99914fd
...
(https://github.com/bitcoin/bitcoin/pull/32764#issuecomment-3459238801)
My Guix build:
```
aarch64
a8613aaa30cc598f8f193932478e57381299399fb1a6a3e8cd27d41ed58e1a2c guix-build-7c97bae595b0/output/aarch64-linux-gnu/SHA256SUMS.part
6b32bd6a25e2c8b2823efc9deb8d9dc1fd4da7713dc00992ee721171500ebbf0 guix-build-7c97bae595b0/output/aarch64-linux-gnu/bitcoin-7c97bae595b0-aarch64-linux-gnu-debug.tar.gz
fbda951a67c0b02a9c4ed8014b3371ecacf7c41f2f0338653963c2f765e29f7f guix-build-7c97bae595b0/output/aarch64-linux-gnu/bitcoin-7c97bae595b0-aarch64-linux-gnu.tar.gz
d99914fd
...
💬 mzumsande commented on pull request "Remove unreliable seed from chainparams.cpp, and the associated README":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3459305391)
In my opinion it doesn't really matter whether a single DNS seeds returns nodes running the newest version or not. DNS seeds are important for the initial bootstrap, they are ideally are queried exactly once in the lifetime of a node. After that, nodes will automatically learn about thousands of other peers, so they will get a representative sample of whatever mix of nodes anyway. Besides, newer Knots versions based on 29.x aren't returned either.
I think the other issues brought up [above](h
...
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3459305391)
In my opinion it doesn't really matter whether a single DNS seeds returns nodes running the newest version or not. DNS seeds are important for the initial bootstrap, they are ideally are queried exactly once in the lifetime of a node. After that, nodes will automatically learn about thousands of other peers, so they will get a representative sample of whatever mix of nodes anyway. Besides, newer Knots versions based on 29.x aren't returned either.
I think the other issues brought up [above](h
...
👍 polespinasa approved a pull request: "refactor: Header sync optimisations & simplifications"
(https://github.com/bitcoin/bitcoin/pull/32740#pullrequestreview-3391487971)
Code Review ACK 50e61f448eedfe0ccfa07f3124aeef9c7762a69b
This PR cleans and simplifies the code in many ways like avoiding calling unneeded functions, avoiding constructing temporary objects, removing unused params, among others...
Just left a single comment with a question :)
btw 552b9c3a565c1817074468bf71fb4526f3a47f42 and 67aa62c80e7d2faa485ba020cefe262f479190f4 are pretty :)
(https://github.com/bitcoin/bitcoin/pull/32740#pullrequestreview-3391487971)
Code Review ACK 50e61f448eedfe0ccfa07f3124aeef9c7762a69b
This PR cleans and simplifies the code in many ways like avoiding calling unneeded functions, avoiding constructing temporary objects, removing unused params, among others...
Just left a single comment with a question :)
btw 552b9c3a565c1817074468bf71fb4526f3a47f42 and 67aa62c80e7d2faa485ba020cefe262f479190f4 are pretty :)
💬 polespinasa commented on pull request "refactor: Header sync optimisations & simplifications":
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2471574938)
I don't get this change, what's the use-case for explicit here? and why is better to use the list instead of a "normal" constructor?
(https://github.com/bitcoin/bitcoin/pull/32740#discussion_r2471574938)
I don't get this change, what's the use-case for explicit here? and why is better to use the list instead of a "normal" constructor?
💬 delta1 commented on pull request "Remove unreliable seed from chainparams.cpp, and the associated README":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3459606922)
reACK https://github.com/bitcoin/bitcoin/commit/c18f9d1fd5d4e1fc255d83ed7f808e2571c4ed9f
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3459606922)
reACK https://github.com/bitcoin/bitcoin/commit/c18f9d1fd5d4e1fc255d83ed7f808e2571c4ed9f
💬 shyrwall commented on pull request "Remove unreliable seed from chainparams.cpp, and the associated README":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3459616234)
> I just queried all the DNS seeds and compared responses with BitNodes API, using `jq` to isolate the user agent:
>
> https://gist.github.com/pinheadmz/ff6057f14aa6b9aa642db2c98f331a1a
>
> ~No real anomolies to speak of,~ except that `seed.mainnet.achownodes.xyz` returned the **most** knots nodes 🤷
>
> update: `dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us` is not returning v29 or v30 nodes, apparently
You can just check https://luke.dashjr.org/programs/bitcoin/files/charts/data/seed
...
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3459616234)
> I just queried all the DNS seeds and compared responses with BitNodes API, using `jq` to isolate the user agent:
>
> https://gist.github.com/pinheadmz/ff6057f14aa6b9aa642db2c98f331a1a
>
> ~No real anomolies to speak of,~ except that `seed.mainnet.achownodes.xyz` returned the **most** knots nodes 🤷
>
> update: `dnsseed.bitcoin.dashjr-list-of-p2p-nodes.us` is not returning v29 or v30 nodes, apparently
You can just check https://luke.dashjr.org/programs/bitcoin/files/charts/data/seed
...
📝 Ashok8601 opened a pull request: "Fix formatting in windows-app.manifest.in"
(https://github.com/bitcoin/bitcoin/pull/33729)
<!--
*** 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 experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/33729)
<!--
*** 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 experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 frankomosh commented on pull request "test: Add bitcoin-chainstate test for assumeutxo functionality":
(https://github.com/bitcoin/bitcoin/pull/33728#discussion_r2471886228)
If user passes 3 args where `argv[2] != "-regtest"`(or generally malformed argument combinations), could it silently set `use_regtest = false` without warning?
If thats the case then I think there should atleast be a warning
(https://github.com/bitcoin/bitcoin/pull/33728#discussion_r2471886228)
If user passes 3 args where `argv[2] != "-regtest"`(or generally malformed argument combinations), could it silently set `use_regtest = false` without warning?
If thats the case then I think there should atleast be a warning
💬 frankomosh commented on pull request "test: Add bitcoin-chainstate test for assumeutxo functionality":
(https://github.com/bitcoin/bitcoin/pull/33728#discussion_r2471889658)
Is this now weaker that the original assertion, because I think it will now silently accepts a null mempool pointer?
(https://github.com/bitcoin/bitcoin/pull/33728#discussion_r2471889658)
Is this now weaker that the original assertion, because I think it will now silently accepts a null mempool pointer?
🤔 frankomosh reviewed a pull request: "test: Add bitcoin-chainstate test for assumeutxo functionality"
(https://github.com/bitcoin/bitcoin/pull/33728#pullrequestreview-3391873761)
Concept ACK
I think its a nice addition as it adds valuable functional coverage for bitcoin-chainstate.
Few nits and inquiries inline;
(https://github.com/bitcoin/bitcoin/pull/33728#pullrequestreview-3391873761)
Concept ACK
I think its a nice addition as it adds valuable functional coverage for bitcoin-chainstate.
Few nits and inquiries inline;
✅ maflcko closed a pull request: "Fix formatting in windows-app.manifest.in"
(https://github.com/bitcoin/bitcoin/pull/33729)
(https://github.com/bitcoin/bitcoin/pull/33729)
💬 SatsAndSports commented on pull request "Remove unreliable seed from chainparams.cpp, and the associated README":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3460104641)
I've squashed the two commits into one, and made the commit text more accurate, as suggested by @davidgumberg
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3460104641)
I've squashed the two commits into one, and made the commit text more accurate, as suggested by @davidgumberg
💬 purpleKarrot commented on pull request "ci, iwyu: Treat warnings as errors for `src/init` and `src/policy`":
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2472087236)
> but the benefit of including span, when span.h is included, seems limited.
That is not the right rationale. Since `span.h` may get phased out by future refactoring, it is advantageous to have an indicator at the top of the file whether `<span>` is used purely directly or directly and indirectly.
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2472087236)
> but the benefit of including span, when span.h is included, seems limited.
That is not the right rationale. Since `span.h` may get phased out by future refactoring, it is advantageous to have an indicator at the top of the file whether `<span>` is used purely directly or directly and indirectly.
💬 wizkid057 commented on pull request "Remove unreliable seed from chainparams.cpp, and the associated README":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3460375147)
NACK
This is clearly a personal attack with zero technical merit, zero violation of any policy, and an overall just part of the continuing witch hunt of some vs Luke.
Regardless, I'd highly suggest waiting until @luke-jr can comment and weigh in on this directly before even considering hastily merging this. Doing so without that minimum courtesy seems quite unnecessary for something that couldn't possibly pose any risk whatsoever nor require such any haste.
https://x.com/LukeDashjr/stat
...
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3460375147)
NACK
This is clearly a personal attack with zero technical merit, zero violation of any policy, and an overall just part of the continuing witch hunt of some vs Luke.
Regardless, I'd highly suggest waiting until @luke-jr can comment and weigh in on this directly before even considering hastily merging this. Doing so without that minimum courtesy seems quite unnecessary for something that couldn't possibly pose any risk whatsoever nor require such any haste.
https://x.com/LukeDashjr/stat
...
💬 l0rinc commented on pull request "Remove unreliable seed from chainparams.cpp, and the associated README":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3460383320)
reACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3460383320)
reACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
💬 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_r2472147424)
Yeah, I can see it both ways. I think the possible harm from including span.h over span is limited with this trivial header. Just noting I did the same with the time.h include: https://github.com/bitcoin/bitcoin/blob/3bb30658e631ed45b6c8609292facc7ae3dd0f61/src/util/time.h#L9
Also for the validation.h include: https://github.com/bitcoin/bitcoin/blob/3bb30658e631ed45b6c8609292facc7ae3dd0f61/src/validation.h#L19. There, the harm could be larger, as the validation header is a bit larger than the
...
(https://github.com/bitcoin/bitcoin/pull/33725#discussion_r2472147424)
Yeah, I can see it both ways. I think the possible harm from including span.h over span is limited with this trivial header. Just noting I did the same with the time.h include: https://github.com/bitcoin/bitcoin/blob/3bb30658e631ed45b6c8609292facc7ae3dd0f61/src/util/time.h#L9
Also for the validation.h include: https://github.com/bitcoin/bitcoin/blob/3bb30658e631ed45b6c8609292facc7ae3dd0f61/src/validation.h#L19. There, the harm could be larger, as the validation header is a bit larger than the
...
💬 delta1 commented on pull request "Remove unreliable seed from chainparams.cpp, and the associated README":
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3460546599)
reACK https://github.com/bitcoin/bitcoin/commit/b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
(https://github.com/bitcoin/bitcoin/pull/33723#issuecomment-3460546599)
reACK https://github.com/bitcoin/bitcoin/commit/b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
👍 dergoegge approved a pull request: "Remove unreliable seed from chainparams.cpp, and the associated README"
(https://github.com/bitcoin/bitcoin/pull/33723#pullrequestreview-3392411168)
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
(https://github.com/bitcoin/bitcoin/pull/33723#pullrequestreview-3392411168)
ACK b0c706795ce6a3a00bf068a81ee99fef2ee9bf7e
🚀 fanquake merged a pull request: "ci: use pycapnp 2.2.1"
(https://github.com/bitcoin/bitcoin/pull/33693)
(https://github.com/bitcoin/bitcoin/pull/33693)