Bitcoin Core Github
43 subscribers
122K links
Download Telegram
👍 TheCharlatan approved a pull request: "refactor: validation: mark CheckBlockIndex as const"
(https://github.com/bitcoin/bitcoin/pull/32364#pullrequestreview-2848233855)
ACK 3e6ac5bf772751c66cdcd015dcb7e6ce4ea2cc77
🤔 luke-jr reviewed a pull request: "config: allow setting -proxy per network"
(https://github.com/bitcoin/bitcoin/pull/32425#pullrequestreview-2848222887)
Probably an improvement as-is, but might want to think through the name proxy logic
💬 luke-jr commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2094078095)
I'm not sure this logic is ideal. If you set an all-network proxy, but then disable it for IPv6, you might be expecting name resolution to use the proxy. Or maybe not.
💬 luke-jr commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2094077060)
These conditions aren't needed. `SetProxy` itself just returns false if it's not valid.
💬 luke-jr commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2094087242)
string_view?
💬 maflcko commented on issue "cmake: Cannot find Qt 6 on SunOS / illumos (OpenIndiana Distribution)":
(https://github.com/bitcoin/bitcoin/issues/32536#issuecomment-2888283003)
Are there any known users on this platform? Maybe this can be closed for now, until there are a few users. Then, a `build-*.md` can be added with any needed details. Whether or not to do a guix build can be decided then as well.
💬 maflcko commented on pull request "[DONOTMERGE] subprocess: replace `fs::directory_iterator` with `readdir`":
(https://github.com/bitcoin/bitcoin/pull/32529#issuecomment-2888285136)
I haven't looked at the code, but the CI passed 50 times: https://cirrus-ci.com/task/6662743735926784
💬 luke-jr commented on pull request "config: allow setting -proxy per network":
(https://github.com/bitcoin/bitcoin/pull/32425#discussion_r2094091911)
Did you want to indent this?
💬 hebasto commented on issue "test: Running thread * was not suspended. False leaks are possible.":
(https://github.com/bitcoin/bitcoin/issues/32542#issuecomment-2888292948)
> ... I'd also think that running `ctest` vs running `test_bitcoin` directly, should generally result in the same output, otherwise if sanitizers are producing warnings, or other relevant output, that is currently being "hidden" by `ctest`.

cc @purpleKarrot
💬 TheCharlatan commented on pull request "validation: Add eligible ancestors of reconsidered block to setBlockIndexCandidates":
(https://github.com/bitcoin/bitcoin/pull/30479#discussion_r2094104836)
Nice catch! How about removing the default argument altogether:
```diff
diff --git a/src/chain.h b/src/chain.h
index c6d1640768..f5bfdb2fb4 100644
--- a/src/chain.h
+++ b/src/chain.h
@@ -295 +295 @@ public:
- bool IsValid(enum BlockStatus nUpTo = BLOCK_VALID_TRANSACTIONS) const
+ bool IsValid(enum BlockStatus nUpTo) const
diff --git a/src/test/fuzz/chain.cpp b/src/test/fuzz/chain.cpp
index 0363f317b6..09053e4815 100644
--- a/src/test/fuzz/chain.cpp
+++ b/src/test/fuzz/chain.cpp
...
👋 hebasto's pull request is ready for review: "build: Use clang-cl to build on Windows natively"
(https://github.com/bitcoin/bitcoin/pull/31507)
💬 romanz commented on pull request "index: store per-block transaction locations for efficient lookups":
(https://github.com/bitcoin/bitcoin/pull/32541#issuecomment-2888339681)
Fixed a few lint issues (following [SonarQube run](https://sonarcloud.io/project/issues?issueStatuses=OPEN%2CCONFIRMED&sinceLeakPeriod=true&branch=32541-1f629743e6c6ba4fab77ef1a8578bb82f0a70566&id=aureleoules_bitcoin)).
💬 hebasto commented on pull request "build: Use clang-cl to build on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31507#discussion_r2094105826)
More details have been added to the commit message.

> Also, any reason to not fix the issues in leveldb, rather than adding more suppressions?

I'll consider this option.
⚠️ hebasto opened an issue: "`AddressBookTests` failure"
(https://github.com/bitcoin-core/gui/issues/874)
From https://cirrus-ci.com/task/5063835456897024:
```
[11:58:03.176] ********* Start testing of AddressBookTests *********
[11:58:03.176] Config: Using QtTest library 6.7.3, Qt 6.7.3 (arm-little_endian-ilp32-eabi-hardfloat static release build; by GCC 13.3.0), ubuntu 24.04
[11:58:03.176] PASS : AddressBookTests::initTestCase()
[11:58:03.176] QDEBUG : AddressBookTests::addressBookTests() NotifyUnload
[11:58:03.176] QWARN : AddressBookTests::addressBookTests() This plugin does not support propa
...
💬 romanz commented on issue "intermittent issue in rpc_signer.py (enumeratesigners timeout)":
(https://github.com/bitcoin/bitcoin/issues/32524#issuecomment-2888369623)
Another instance: https://cirrus-ci.com/task/4688904676179968?logs=ci#L4390
💬 purpleKarrot commented on issue "test: Running thread * was not suspended. False leaks are possible.":
(https://github.com/bitcoin/bitcoin/issues/32542#issuecomment-2888371321)
CTest is instructed to pass some arguments to `test_bitcoin`: https://github.com/bitcoin/bitcoin/blob/7710a31f0cb69a04529f39840196826d0b9770ab/src/test/CMakeLists.txt#L193-L195

Maybe that is the reason? That being said, CTest has builtin support for memory checking using sanitizers.
💬 laanwj commented on pull request "[DONOTMERGE] subprocess: replace `fs::directory_iterator` with `readdir`":
(https://github.com/bitcoin/bitcoin/pull/32529#issuecomment-2888390983)
> I haven't looked at the code, but the CI passed 50 times: https://cirrus-ci.com/task/6662743735926784

Okay. So unless the original code is using `directory_iterator` wrong (which i don't think so), we can be reasonably sure there's an intermittent bug in the implementation of it on at least some libc++ versions.
👍 TheCharlatan approved a pull request: "validation: stricter internal handling of invalid blocks"
(https://github.com/bitcoin/bitcoin/pull/31405#pullrequestreview-2848281583)
Re-ACK ed172d3002da68a3ddbd5d13e7d3f0618c1378d4
💬 l0rinc commented on pull request "blocks: avoid recomputing block header hash in `ReadBlock`":
(https://github.com/bitcoin/bitcoin/pull/32487#discussion_r2094147733)
We're not always checking this, only when the index is available.
But after this change this check is for free, no need to recalculate the header's hash anymore.