π¬ maflcko commented on pull request "test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1567097669)
I don't see how using `auto` makes anything clearer.
There certainly are style and language features that make maintenance easier, but they need to be well motivated and explained. Also, if there is a good reason for them, they should be applied globally via CI and checks. Otherwise, they are again applied inconsistently.
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1567097669)
I don't see how using `auto` makes anything clearer.
There certainly are style and language features that make maintenance easier, but they need to be well motivated and explained. Also, if there is a good reason for them, they should be applied globally via CI and checks. Otherwise, they are again applied inconsistently.
π¬ paplorinc commented on pull request "test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1567105912)
I agree. Is there any solution?
Is there any non-test change that I could delve intoβpreferably an optimization that doesn't require me to understand every part of the codeβthat would be welcome?
For example, like my other [base58 or bech32 speedups](https://github.com/bitcoin/bitcoin/pulls/paplorinc).
Or am I just barking up the wrong tree and should move on to other projects?
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1567105912)
I agree. Is there any solution?
Is there any non-test change that I could delve intoβpreferably an optimization that doesn't require me to understand every part of the codeβthat would be welcome?
For example, like my other [base58 or bech32 speedups](https://github.com/bitcoin/bitcoin/pulls/paplorinc).
Or am I just barking up the wrong tree and should move on to other projects?
π¬ fanquake commented on pull request "test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058741052)
> I don't yet understand how we expect this code to become more and more maintainable, if we don't regularly leave the campground cleaner than we've found it.
The issue here is that even if you've found the campground, and while maybe it looks like it could use a little maintenance, if you zoom out for a birds-eye view, you'll actually notice that everyone else is busy somewhere next door, trying to fight the forest fires. Even if the code here is (maybe arbitrarily) "better" or "faster" by s
...
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058741052)
> I don't yet understand how we expect this code to become more and more maintainable, if we don't regularly leave the campground cleaner than we've found it.
The issue here is that even if you've found the campground, and while maybe it looks like it could use a little maintenance, if you zoom out for a birds-eye view, you'll actually notice that everyone else is busy somewhere next door, trying to fight the forest fires. Even if the code here is (maybe arbitrarily) "better" or "faster" by s
...
π¬ hebasto commented on pull request "[WIP] build: remove need to test for endianness":
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2058753271)
> > However, the changes in MSVC generated assembly code look quite significant.
> > Before stacking another performance deterioration change on top of the pile
>
> Isn't that because optimisations haven't been turned on? Otherwise, can you provide a concrete example of what you're talking about.
https://godbolt.org/z/of4T8hM8j provides examples with the [`/O2`](https://learn.microsoft.com/en-us/cpp/build/reference/o1-o2-minimize-size-maximize-speed) optimization flag.
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2058753271)
> > However, the changes in MSVC generated assembly code look quite significant.
> > Before stacking another performance deterioration change on top of the pile
>
> Isn't that because optimisations haven't been turned on? Otherwise, can you provide a concrete example of what you're talking about.
https://godbolt.org/z/of4T8hM8j provides examples with the [`/O2`](https://learn.microsoft.com/en-us/cpp/build/reference/o1-o2-minimize-size-maximize-speed) optimization flag.
π¬ paplorinc commented on pull request "test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058755998)
Thanks for the detailed answer @fanquake.
> everyone else is busy [...] trying to fight the forest fires
So how do we prevent future forest fires while extinguishing current ones?
I have looked through `good first issue` since the beginning, there's barely any, all of which were started by others, often years ago.
That's why I started working on what *I can* contribute instead.
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058755998)
Thanks for the detailed answer @fanquake.
> everyone else is busy [...] trying to fight the forest fires
So how do we prevent future forest fires while extinguishing current ones?
I have looked through `good first issue` since the beginning, there's barely any, all of which were started by others, often years ago.
That's why I started working on what *I can* contribute instead.
π stickies-v approved a pull request: "doc: archive 27.0 release notes"
(https://github.com/bitcoin/bitcoin/pull/29886#pullrequestreview-2003249019)
ACK c08754971d207bd2b60ba9c4faf34396a97bbc26
(https://github.com/bitcoin/bitcoin/pull/29886#pullrequestreview-2003249019)
ACK c08754971d207bd2b60ba9c4faf34396a97bbc26
π¬ hebasto commented on pull request "[WIP] build: remove need to test for endianness":
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2058762767)
What benchmarks might be appropiate for testing changes like these?
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2058762767)
What benchmarks might be appropiate for testing changes like these?
π¬ maflcko commented on pull request "test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058797361)
Another great way to contribute would be to spend time on in-depth review of open pull requests and compare your review with the review done by others. Obviously this will be time-consuming, starting out, but if you care about the project, over time it will teach you relevant skills (for this project and for yourself to apply elsewhere).
On some pull requests, there remain small follow-ups that were found during review, so you'll naturally find stuff to work on if you start out by reviewing p
...
(https://github.com/bitcoin/bitcoin/pull/29847#issuecomment-2058797361)
Another great way to contribute would be to spend time on in-depth review of open pull requests and compare your review with the review done by others. Obviously this will be time-consuming, starting out, but if you care about the project, over time it will teach you relevant skills (for this project and for yourself to apply elsewhere).
On some pull requests, there remain small follow-ups that were found during review, so you'll naturally find stuff to work on if you start out by reviewing p
...
π¬ Jgoedhart1988 commented on pull request "Fix typos in `subprocess.hpp`":
(https://github.com/bitcoin/bitcoin/pull/29849#issuecomment-2058798586)
Hello World
(https://github.com/bitcoin/bitcoin/pull/29849#issuecomment-2058798586)
Hello World
π€ Jgoedhart1988 reviewed a pull request: "Fix typos in `subprocess.hpp`"
(https://github.com/bitcoin/bitcoin/pull/29849#pullrequestreview-2003287765)
Ok
(https://github.com/bitcoin/bitcoin/pull/29849#pullrequestreview-2003287765)
Ok
π fanquake locked a pull request: "Fix typos in `subprocess.hpp`"
(https://github.com/bitcoin/bitcoin/pull/29849)
Resolves one item in the https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1991272752:
> - Remove linter exclusions and fix all issues.
Based on upstream https://github.com/arun11299/cpp-subprocess/pull/101.
(https://github.com/bitcoin/bitcoin/pull/29849)
Resolves one item in the https://github.com/bitcoin/bitcoin/pull/28981#pullrequestreview-1991272752:
> - Remove linter exclusions and fix all issues.
Based on upstream https://github.com/arun11299/cpp-subprocess/pull/101.
π fanquake merged a pull request: "doc: archive 27.0 release notes"
(https://github.com/bitcoin/bitcoin/pull/29886)
(https://github.com/bitcoin/bitcoin/pull/29886)
π¬ maflcko commented on pull request "[WIP] build: remove need to test for endianness":
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2058803343)
> What benchmarks might be appropiate for testing changes like these?
Microbenchmarks + IBD?
(https://github.com/bitcoin/bitcoin/pull/29852#issuecomment-2058803343)
> What benchmarks might be appropiate for testing changes like these?
Microbenchmarks + IBD?
π fanquake opened a pull request: "deploy: remove some tools when cross-compiling for macOS"
(https://github.com/bitcoin/bitcoin/pull/29890)
Neither of these tools are actually used when we are cross-compiling for macOS. They are used when we have to adjust non-static libs during a deploy after building on a macOS machine. Simplies #29739 (will be rebased on top).
Guix (aarch64):
```bash
8f29bce75d7f574306a0e38d793e0e4e145b547a4b9e5a755a54976121d8ac41 guix-build-5afd3c302051/output/arm64-apple-darwin/SHA256SUMS.part
9ba01fe46be715adcbe80f39dc7dbe449f32ca9d9b660da698f933aef3e6d80b guix-build-5afd3c302051/output/arm64-apple-dar
...
(https://github.com/bitcoin/bitcoin/pull/29890)
Neither of these tools are actually used when we are cross-compiling for macOS. They are used when we have to adjust non-static libs during a deploy after building on a macOS machine. Simplies #29739 (will be rebased on top).
Guix (aarch64):
```bash
8f29bce75d7f574306a0e38d793e0e4e145b547a4b9e5a755a54976121d8ac41 guix-build-5afd3c302051/output/arm64-apple-darwin/SHA256SUMS.part
9ba01fe46be715adcbe80f39dc7dbe449f32ca9d9b660da698f933aef3e6d80b guix-build-5afd3c302051/output/arm64-apple-dar
...
π€ glozow reviewed a pull request: "p2p: opportunistically accept 1-parent-1-child packages"
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2003285723)
thanks for the review @mzumsande!
(https://github.com/bitcoin/bitcoin/pull/28970#pullrequestreview-2003285723)
thanks for the review @mzumsande!
π¬ glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567166818)
marking as resolved since #29827 does this
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567166818)
marking as resolved since #29827 does this
π¬ glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567166471)
updated
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567166471)
updated
π¬ glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567165968)
Elaborated in commit message
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567165968)
Elaborated in commit message
π¬ glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567146604)
I've added more explicit tests and changed this to be ordered based on `uint256.GetHex()` instead. I don't know enough to say which sorting is better here, but this seems like the natural ordering when I'm reading the hex strings as a human.
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567146604)
I've added more explicit tests and changed this to be ordered based on `uint256.GetHex()` instead. I don't know enough to say which sorting is better here, but this seems like the natural ordering when I'm reading the hex strings as a human.
π¬ glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567166366)
added
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1567166366)
added