π¬ maflcko commented on pull request "build: Fix false positive `CHECK_ATOMIC` test":
(https://github.com/bitcoin/bitcoin/pull/29859#issuecomment-2058623912)
review ACK dd3e0fa12534c9e782dc9c24d2e30b70a0d73176
(https://github.com/bitcoin/bitcoin/pull/29859#issuecomment-2058623912)
review ACK dd3e0fa12534c9e782dc9c24d2e30b70a0d73176
β οΈ maflcko opened an issue: "Intermittent issue in test/ipc_tests.cpp Fatal glibc error: pthread_mutex_lock.c:450 (__pthread_mutex_lock_full): assertion failed: e != ESRCH || !robust"
(https://github.com/bitcoin/bitcoin/issues/29889)
Happened on current master d29fc3a245c070494155dad4cf68b9c95d99c13e in ci_i686_multiprocess
```
Running tests: wallet_util_tests from wallet/test/rpc_util_tests.cpp
Running tests: scriptpubkeyman_tests from wallet/test/scriptpubkeyman_tests.cpp
Running tests: walletload_tests from wallet/test/walletload_tests.cpp
Running tests: group_outputs_tests from wallet/test/group_outputs_tests.cpp
Running tests: db_tests from wallet/test/db_tests.cpp
Running tests: ipc_tests from test/ipc_tests.c
...
(https://github.com/bitcoin/bitcoin/issues/29889)
Happened on current master d29fc3a245c070494155dad4cf68b9c95d99c13e in ci_i686_multiprocess
```
Running tests: wallet_util_tests from wallet/test/rpc_util_tests.cpp
Running tests: scriptpubkeyman_tests from wallet/test/scriptpubkeyman_tests.cpp
Running tests: walletload_tests from wallet/test/walletload_tests.cpp
Running tests: group_outputs_tests from wallet/test/group_outputs_tests.cpp
Running tests: db_tests from wallet/test/db_tests.cpp
Running tests: ipc_tests from test/ipc_tests.c
...
π¬ maflcko commented on issue "Intermittent issue in test/ipc_tests.cpp Fatal glibc error: pthread_mutex_lock.c:450 (__pthread_mutex_lock_full): assertion failed: e != ESRCH || !robust":
(https://github.com/bitcoin/bitcoin/issues/29889#issuecomment-2058655310)
```
# podman exec ci_i686_multiprocess uname -a
Linux a943c4649ecd 5.14.21-150400.24.100-default #1 SMP PREEMPT_DYNAMIC Mon Dec 4 19:12:13 UTC 2023 (3f5cd84) x86_64 x86_64 x86_64 GNU/Linux
(https://github.com/bitcoin/bitcoin/issues/29889#issuecomment-2058655310)
```
# podman exec ci_i686_multiprocess uname -a
Linux a943c4649ecd 5.14.21-150400.24.100-default #1 SMP PREEMPT_DYNAMIC Mon Dec 4 19:12:13 UTC 2023 (3f5cd84) x86_64 x86_64 x86_64 GNU/Linux
π¬ fanquake commented on issue "ci: failure in `rpc_scanblocks.py`":
(https://github.com/bitcoin/bitcoin/issues/29831#issuecomment-2058678496)
https://cirrus-ci.com/task/4562186641604608?logs=ci#L4268
(https://github.com/bitcoin/bitcoin/issues/29831#issuecomment-2058678496)
https://cirrus-ci.com/task/4562186641604608?logs=ci#L4268
π¬ paplorinc commented on pull request "refactor/test: add a few more base32/64 calculation corner cases":
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1567078029)
Removed the 3/4 to 6/8 change, kept only the test.
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.
(https://github.com/bitcoin/bitcoin/pull/29847#discussion_r1567078029)
Removed the 3/4 to 6/8 change, kept only the test.
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.
π¬ furszy commented on pull request "index: race fix, lock cs_main while 'm_synced' is subject to change":
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2058716277)
> Is it worth it to partially revert? Why not just git revert 0faafb57f8298547949cbc0044ee9e925ed887ba?
I didn't do it because the revert is not clean. It conflicts with bbe82c116e72ca0638751e063bf564cd1fe5c4d5 and it would require an extra commit for the added doc (which, based on the issue, is a must have for me).
But np on doing it and squashing the commits down to one. One sec.
(https://github.com/bitcoin/bitcoin/pull/29867#issuecomment-2058716277)
> Is it worth it to partially revert? Why not just git revert 0faafb57f8298547949cbc0044ee9e925ed887ba?
I didn't do it because the revert is not clean. It conflicts with bbe82c116e72ca0638751e063bf564cd1fe5c4d5 and it would require an extra commit for the added doc (which, based on the issue, is a must have for me).
But np on doing it and squashing the commits down to one. One sec.
π¬ 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
...