Bitcoin Core Github
45 subscribers
118K links
Download Telegram
💬 real-or-random commented on pull request "cleanse: switch to SecureZeroMemory for Windows cross-compile":
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2429613882)
@fanquake Can you elaborate on the motivation for this PR? The code in the `#else` should work for any gcc and clang, on any platform. While it uses an asm block, there's no actual asm instruction inside that could be platform-specific.

See also its platform-independent usage in Linux:
https://github.com/torvalds/linux/blob/c2ee9f594da826bea183ed14f2cc029c719bf4da/include/linux/string.h#L369-L376
https://github.com/torvalds/linux/blob/c2ee9f594da826bea183ed14f2cc029c719bf4da/include/linux
...
💬 maflcko commented on issue "bench: `linearizeoptimallyexample11` benchmark now running 4x slow than previously":
(https://github.com/bitcoin/bitcoin/issues/31178#issuecomment-2449665430)
Then I think the only solution is to make it a build-time option again:

```cpp
constexpr bool g_fuzzing{
#ifdef FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION
true
#else
false
#endif
};
💬 hodlinator commented on pull request "build, ci: Fix linking `bitcoin-chainstate.exe` to `bitcoinkernel.dll` on Windows":
(https://github.com/bitcoin/bitcoin/pull/31158#discussion_r1981191286)
nit: Less confusing for every branch in the block to result in `#define`:
```suggestion
#else
```
💬 l0rinc commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#discussion_r2002955096)
I'm not exactly sure how we got to the `min_alloc` of `9` on these platforms, but we can probably simplify the platform dependent code to:
```C++
#if defined(__arm__) || SIZE_MAX == UINT64_MAX
constexpr size_t min_alloc{9};
#else
constexpr size_t min_alloc{0};
#endif
```
💬 l0rinc commented on pull request "feefrac: avoid integer overflow in temporary":
(https://github.com/bitcoin/bitcoin/pull/32300#discussion_r2049555771)
The simplest way I could find is to call it directly (not sure what the fuzzer did, don't have access):
```C++
BOOST_AUTO_TEST_CASE(feefrac_div_round_down_overflow)
{
#ifdef __SIZEOF_INT128__
constexpr auto divisor{2};
constexpr auto expected_result{std::numeric_limits<int64_t>::max()};
constexpr auto overflow_numerator{__int128{expected_result} * divisor + 1};
BOOST_CHECK_EQUAL(FeeFrac::Div(overflow_numerator, divisor, /*round_down=*/true), expected_result);
#else

...
💬 maflcko commented on issue "OpenBSD, NetBSD: `-reindex` is broken":
(https://github.com/bitcoin/bitcoin/issues/33128#issuecomment-3206135519)
Diff to reproduce on Linux:


```diff
diff --git a/src/util/fs_helpers.cpp b/src/util/fs_helpers.cpp
index be7f1ee5a2..ada3280dbc 100644
--- a/src/util/fs_helpers.cpp
+++ b/src/util/fs_helpers.cpp
@@ -200,7 +200,7 @@ void AllocateFileRange(FILE* file, unsigned int offset, unsigned int length)
}
ftruncate(fileno(file), static_cast<off_t>(offset) + length);
#else
-#if defined(HAVE_POSIX_FALLOCATE)
+#if 0
// Version using posix_fallocate
off_t nEndPos = (off_t)offset + length;

...
💬 purpleKarrot commented on pull request "kernel: Introduce initial C header API":
(https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2288681559)
I think the deep nesting can be avoided with the approach mentioned in https://habla.news/u/purplekarrot.net/cmake-import-export

Also, I don't think the `#else` branch is needed. To my knowledge, there is no platform that supports C++20 but neither dllexport nor visibility.
💬 l0rinc commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2347458082)
If any of you feel strongly, I can change it, but it was a deliberate choice from my part.
See https://godbolt.org/z/o3sKqefT5 for an ugly error on Windows that also convinced me to just use the shorter version. I could of course hack it with:
```C++
#ifdef _WIN32
#include <windows.h>
#undef max
#else
```
or just hope that https://github.com/bitcoin/bitcoin/blob/9f744fffc39d93c9966dec1d61f113a7521983ad/CMakeLists.txt#L261 fixes it, but `SIZE_MAX` seemed simpler and shorter in this case :/

-
...
💬 hebasto commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2362554073)
This should work:
```suggestion
std::optional<size_t> GetTotalRAM()
{
auto clamp{[](uint64_t v) { return size_t(std::min(v, uint64_t{std::numeric_limits<size_t>::max()})); }};
#ifdef WIN32
if (MEMORYSTATUSEX m{}; (m.dwLength = sizeof(m), GlobalMemoryStatusEx(&m))) return clamp(m.ullTotalPhys);
#else
if (long p{sysconf(_SC_PHYS_PAGES)}, s{sysconf(_SC_PAGESIZE)}; p > 0 && s > 0) return clamp(1ULL * p * s);
#endif
return std::nullopt;
}

```

See CI outputs for:
- Fr
...
💬 hebasto commented on pull request "coins: warn on oversized `-dbcache`":
(https://github.com/bitcoin/bitcoin/pull/33333#discussion_r2362723602)
> I did not go with `#else` since I am not sure that will compile and work on all possible platforms that are not Windows.

Right. We could end up with macros for all tested platforms.