💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1554004259)
Hm no, I think we should return pairs of tx and the `fromPeer`, and just attribute the error to the right peer. If we restrict it to same sender, then we can also block packages by sending the children fast and refusing to send the parent.
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1554004259)
Hm no, I think we should return pairs of tx and the `fromPeer`, and just attribute the error to the right peer. If we restrict it to same sender, then we can also block packages by sending the children fast and refusing to send the parent.
⚠️ achow101 opened an issue: "`WalletCreate{Encrypted/Plain}` benchmark crash on Windows"
(https://github.com/bitcoin/bitcoin/issues/29816)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
After modifying the guix build script for 27.0rc1 to produce the benchmark binary, when `bench_bitcoin` is run on Windows, it crashes on the `WalletCreateEncrypted` and `WalletCreatePlain` benchmarks with
```
terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
what(): filesystem error: cannot remove all: Broken pipe [C:\Users\ava\AppData\Loca
...
(https://github.com/bitcoin/bitcoin/issues/29816)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
After modifying the guix build script for 27.0rc1 to produce the benchmark binary, when `bench_bitcoin` is run on Windows, it crashes on the `WalletCreateEncrypted` and `WalletCreatePlain` benchmarks with
```
terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
what(): filesystem error: cannot remove all: Broken pipe [C:\Users\ava\AppData\Loca
...
💬 theuni commented on pull request "depends: add new LLVM debug macro":
(https://github.com/bitcoin/bitcoin/pull/29781#issuecomment-2040274229)
Oh, heh, I missed that @maflcko had already linked that. See also [here for an RFC about the deprecation](https://discourse.llvm.org/t/rfc-removing-the-legacy-debug-mode-from-libc/71026).
I think I'm inclined to agree that it's not worth supporting the old mode. It seems it's old and janky and deprecated for good reason.
(https://github.com/bitcoin/bitcoin/pull/29781#issuecomment-2040274229)
Oh, heh, I missed that @maflcko had already linked that. See also [here for an RFC about the deprecation](https://discourse.llvm.org/t/rfc-removing-the-legacy-debug-mode-from-libc/71026).
I think I'm inclined to agree that it's not worth supporting the old mode. It seems it's old and janky and deprecated for good reason.
💬 fanquake commented on pull request "depends: add new LLVM debug macro":
(https://github.com/bitcoin/bitcoin/pull/29781#issuecomment-2040280405)
Dropped the older define.
(https://github.com/bitcoin/bitcoin/pull/29781#issuecomment-2040280405)
Dropped the older define.
💬 hebasto commented on issue "`WalletCreate{Encrypted/Plain}` benchmark crash on Windows":
(https://github.com/bitcoin/bitcoin/issues/29816#issuecomment-2040280595)
Does your Windows user have a OS permission to create symbolic links?
(https://github.com/bitcoin/bitcoin/issues/29816#issuecomment-2040280595)
Does your Windows user have a OS permission to create symbolic links?
🤔 tdb3 reviewed a pull request: "test: Handle functional test disk-full error"
(https://github.com/bitcoin/bitcoin/pull/29335#pullrequestreview-1983747381)
ACK for 2f7fcd5fe7289f7ad3efd566dbbd725264654f8f.
Left a nit inline.
Used a small ramdisk (1.2GB) with high jobs count (16) to force space exhaustion. As expected, received the initial warning `Running the test suite with fewer than 2626.0 MB of free space might cause tests to fail.` and also received the exiting error `Early exiting after test failure due to disk running out of space...`
(https://github.com/bitcoin/bitcoin/pull/29335#pullrequestreview-1983747381)
ACK for 2f7fcd5fe7289f7ad3efd566dbbd725264654f8f.
Left a nit inline.
Used a small ramdisk (1.2GB) with high jobs count (16) to force space exhaustion. As expected, received the initial warning `Running the test suite with fewer than 2626.0 MB of free space might cause tests to fail.` and also received the exiting error `Early exiting after test failure due to disk running out of space...`
💬 tdb3 commented on pull request "test: Handle functional test disk-full error":
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1554019884)
nit: The directory is left full upon exit. The user would need to clean it up before re-running tests. Having the user manually clean up the directory seems reasonable to me (the user may want to see actions leading to the failure). An enhanced error message adds value.
```diff
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index dd3ea370bd..f08eb63b21 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -668,7 +668,9 @@ de
...
(https://github.com/bitcoin/bitcoin/pull/29335#discussion_r1554019884)
nit: The directory is left full upon exit. The user would need to clean it up before re-running tests. Having the user manually clean up the directory seems reasonable to me (the user may want to see actions leading to the failure). An enhanced error message adds value.
```diff
diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py
index dd3ea370bd..f08eb63b21 100755
--- a/test/functional/test_runner.py
+++ b/test/functional/test_runner.py
@@ -668,7 +668,9 @@ de
...
💬 glozow commented on pull request "p2p: opportunistically accept 1-parent-1-child packages":
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1554024570)
Last push should fix this with the above approach. Still need to write a test case for the peers being different.
(https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1554024570)
Last push should fix this with the above approach. Still need to write a test case for the peers being different.
💬 achow101 commented on issue "`WalletCreate{Encrypted/Plain}` benchmark crash on Windows":
(https://github.com/bitcoin/bitcoin/issues/29816#issuecomment-2040289538)
> Does your Windows user have an OS permission to create symbolic links?
No, but even when run as admin which does, the error still occurs.
(https://github.com/bitcoin/bitcoin/issues/29816#issuecomment-2040289538)
> Does your Windows user have an OS permission to create symbolic links?
No, but even when run as admin which does, the error still occurs.
💬 theuni commented on pull request "build: replace custom `MAC_OSX` macro with standard `__APPLE__` for consistent macOS detection":
(https://github.com/bitcoin/bitcoin/pull/29450#issuecomment-2040323463)
I recently did the same thing in one of my branches while working to remove the remaining autoconf vars from crypto/: https://github.com/theuni/bitcoin/commit/3646f76688e289774cce45e8f7a9967ee570ddda .
I think this is the right thing to do. I just checked locally and this applies to iOS as well:
`clang -target arm64-apple-ios test.cpp --sysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS16.4.sdk -o testing`
Where test.cpp is:
```c++
#in
...
(https://github.com/bitcoin/bitcoin/pull/29450#issuecomment-2040323463)
I recently did the same thing in one of my branches while working to remove the remaining autoconf vars from crypto/: https://github.com/theuni/bitcoin/commit/3646f76688e289774cce45e8f7a9967ee570ddda .
I think this is the right thing to do. I just checked locally and this applies to iOS as well:
`clang -target arm64-apple-ios test.cpp --sysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS16.4.sdk -o testing`
Where test.cpp is:
```c++
#in
...
💬 theuni commented on pull request "build: replace custom `MAC_OSX` macro with standard `__APPLE__` for consistent macOS detection":
(https://github.com/bitcoin/bitcoin/pull/29450#issuecomment-2040325415)
Whoops, sorry. I spoke too fast. This is the right thing to do for `sha256.cpp`. I'd have to look into the rest in detail, but I'd ACK that small change.
(https://github.com/bitcoin/bitcoin/pull/29450#issuecomment-2040325415)
Whoops, sorry. I spoke too fast. This is the right thing to do for `sha256.cpp`. I'd have to look into the rest in detail, but I'd ACK that small change.
💬 hebasto commented on pull request "crypto: chacha20: always use our fallback timingsafe_bcmp rather than libc's":
(https://github.com/bitcoin/bitcoin/pull/29815#issuecomment-2040326708)
Concept ACK.
(https://github.com/bitcoin/bitcoin/pull/29815#issuecomment-2040326708)
Concept ACK.
💬 itornaza commented on pull request "test: Bump timeouts in feature_index_prune and wallet_importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/29791#issuecomment-2040330763)
approach ACK 49c0b8b2288e60ae22fcac5d03811cf36ecec058
It is indeed frustrating to watch some functional tests failing by just using 12-16 jobs in parallel. This PR addresses the issue with minimal footprint in the test code. However, the timeout of some tests are even greater on my machine without me understanding the reason yet. Until this issue is resolved, running the extended tests with the default number of jobs seems the best approach to me.
## On Master branch
Replicated the erra
...
(https://github.com/bitcoin/bitcoin/pull/29791#issuecomment-2040330763)
approach ACK 49c0b8b2288e60ae22fcac5d03811cf36ecec058
It is indeed frustrating to watch some functional tests failing by just using 12-16 jobs in parallel. This PR addresses the issue with minimal footprint in the test code. However, the timeout of some tests are even greater on my machine without me understanding the reason yet. Until this issue is resolved, running the extended tests with the default number of jobs seems the best approach to me.
## On Master branch
Replicated the erra
...
💬 theuni commented on pull request "Update libsecp256k1 subtree to latest master":
(https://github.com/bitcoin/bitcoin/pull/29803#issuecomment-2040434183)
Subtree merge looks clean and not backdoored:
```
$ test/lint/git-subtree-check.sh -r src/secp256k1
src/secp256k1 in HEAD currently refers to tree c3c4db9c0e4636135963272b005b11a451303feb
src/secp256k1 in HEAD was last updated in commit 53eec53dca1cb677d11564b055d3b8581ddd6747 (tree c3c4db9c0e4636135963272b005b11a451303feb)
src/secp256k1 in HEAD was last updated to upstream commit d8311688bd383d3a923a1b11789cded3cc8e5e03 (tree c3c4db9c0e4636135963272b005b11a451303feb)
GOOD
```
secp chang
...
(https://github.com/bitcoin/bitcoin/pull/29803#issuecomment-2040434183)
Subtree merge looks clean and not backdoored:
```
$ test/lint/git-subtree-check.sh -r src/secp256k1
src/secp256k1 in HEAD currently refers to tree c3c4db9c0e4636135963272b005b11a451303feb
src/secp256k1 in HEAD was last updated in commit 53eec53dca1cb677d11564b055d3b8581ddd6747 (tree c3c4db9c0e4636135963272b005b11a451303feb)
src/secp256k1 in HEAD was last updated to upstream commit d8311688bd383d3a923a1b11789cded3cc8e5e03 (tree c3c4db9c0e4636135963272b005b11a451303feb)
GOOD
```
secp chang
...
💬 TheCharlatan commented on pull request "depends: build libnatpmp with CMake":
(https://github.com/bitcoin/bitcoin/pull/29708#issuecomment-2040551060)
Guix build (x86_64):
```
a134241bc9ff9823ce078ea10dbac544ff63536ee3f66fb557414ca191b1d62d guix-build-3c1ae3ee33d4/output/aarch64-linux-gnu/SHA256SUMS.part
f5a46ab7a5faae9ce50b2d084cf65d14372b354b9f8369178f1348b70a3b675b guix-build-3c1ae3ee33d4/output/aarch64-linux-gnu/bitcoin-3c1ae3ee33d4-aarch64-linux-gnu-debug.tar.gz
e2353ed4f0738026fe16cd90b503ef2f02f31e0b68ba42ea80924997225ae374 guix-build-3c1ae3ee33d4/output/aarch64-linux-gnu/bitcoin-3c1ae3ee33d4-aarch64-linux-gnu.tar.gz
78060899627
...
(https://github.com/bitcoin/bitcoin/pull/29708#issuecomment-2040551060)
Guix build (x86_64):
```
a134241bc9ff9823ce078ea10dbac544ff63536ee3f66fb557414ca191b1d62d guix-build-3c1ae3ee33d4/output/aarch64-linux-gnu/SHA256SUMS.part
f5a46ab7a5faae9ce50b2d084cf65d14372b354b9f8369178f1348b70a3b675b guix-build-3c1ae3ee33d4/output/aarch64-linux-gnu/bitcoin-3c1ae3ee33d4-aarch64-linux-gnu-debug.tar.gz
e2353ed4f0738026fe16cd90b503ef2f02f31e0b68ba42ea80924997225ae374 guix-build-3c1ae3ee33d4/output/aarch64-linux-gnu/bitcoin-3c1ae3ee33d4-aarch64-linux-gnu.tar.gz
78060899627
...
💬 cbergqvist commented on pull request "test: Bump timeouts in feature_index_prune and wallet_importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/29791#issuecomment-2040552035)
Thanks for trying it out!
> and again the same test that is not a subject of this PR:
>
> ```
> wallet_transactiontime_rescan.py --legacy-wallet | ✖ Failed | 159 s (not covered by this PR)
> ```
>
> Note: All test run on Apple M2 Max chip and macos 14.4.1 (23E224)
Searched closed issues and it seems like others running on Mac are also seeing `wallet_transactiontime_rescan.py --legacy-wallet` fail: #28221. Hopefully that specific failure & fix can be addressed in that issue.
(https://github.com/bitcoin/bitcoin/pull/29791#issuecomment-2040552035)
Thanks for trying it out!
> and again the same test that is not a subject of this PR:
>
> ```
> wallet_transactiontime_rescan.py --legacy-wallet | ✖ Failed | 159 s (not covered by this PR)
> ```
>
> Note: All test run on Apple M2 Max chip and macos 14.4.1 (23E224)
Searched closed issues and it seems like others running on Mac are also seeing `wallet_transactiontime_rescan.py --legacy-wallet` fail: #28221. Hopefully that specific failure & fix can be addressed in that issue.
💬 theuni commented on pull request "depends: add new LLVM debug macro":
(https://github.com/bitcoin/bitcoin/pull/29781#issuecomment-2040590662)
utACK. Should darwin get this too though since it also uses libc++ ?
(https://github.com/bitcoin/bitcoin/pull/29781#issuecomment-2040590662)
utACK. Should darwin get this too though since it also uses libc++ ?
💬 dergoegge commented on pull request "clang-tidy: Enable misc-no-recursion":
(https://github.com/bitcoin/bitcoin/pull/29690#issuecomment-2040596926)
Changed the dev note. Ready for review.
(https://github.com/bitcoin/bitcoin/pull/29690#issuecomment-2040596926)
Changed the dev note. Ready for review.
📝 TheCharlatan opened a pull request: "kernel: De-globalize fReindex"
(https://github.com/bitcoin/bitcoin/pull/29817)
fReindex is one of the last remaining globals exposed by the kernel library, so move it into the blockstorage class to reduce the amount of global mutable state and make the kernel library a bit less awkward to use.
---
This pull request is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
(https://github.com/bitcoin/bitcoin/pull/29817)
fReindex is one of the last remaining globals exposed by the kernel library, so move it into the blockstorage class to reduce the amount of global mutable state and make the kernel library a bit less awkward to use.
---
This pull request is part of the [libbitcoinkernel project](https://github.com/bitcoin/bitcoin/issues/27587).
💬 cbergqvist commented on pull request "test: Bump timeouts in feature_index_prune and wallet_importdescriptors":
(https://github.com/bitcoin/bitcoin/pull/29791#issuecomment-2040608592)
@itornaza if you have the inclination, it might be helpful if you could provide combined logs for the `wallet_importdescriptors.py --descriptors` test when it fails even when using this PR's commit. Then I/we could compare it with the combined logs produced on my machine (uploaded and linked in the first comment) and maybe figure out what lies behind the performance difference.
(https://github.com/bitcoin/bitcoin/pull/29791#issuecomment-2040608592)
@itornaza if you have the inclination, it might be helpful if you could provide combined logs for the `wallet_importdescriptors.py --descriptors` test when it fails even when using this PR's commit. Then I/we could compare it with the combined logs produced on my machine (uploaded and linked in the first comment) and maybe figure out what lies behind the performance difference.