💬 Sjors commented on pull request "build: use make < 3.82 syntax for define directive":
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2725088984)
> I don't understand how it wasn't causing any errors with make 3.81

I probably haven't recently used the depends build to test multiprocess on my development machine. I would either use my locally install `libmultiprocess` or a (cross-compiled) Guix build.

💬 maflcko commented on pull request "build: use make < 3.82 syntax for define directive":
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2725091163)
> This is a pretty unusual bug and I don't understand how it wasn't causing any errors with make 3.81, just causing the flags to be ignored.

In light of silent failures, it would be better to rule out this class of bug completely. Otherwise, a new instance will be added in the future, or will already exist somewhere in the code.

I am not sure why reviewers should spend time on supporting build tools that were released about 20 years ago. So rejecting such a build tool early with a failure
...

💬 Sjors commented on pull request "build: use make < 3.82 syntax for define directive":
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2725097671)
@maflcko I'm fine with introducing a minimum GNU make version, as long as we actually check the version (otherwise we're back to silent failures).

But note that I run the stock macOS GNU make all the time, as do many other devs. So new issues are likely to be caught during review, though they'll still waste time debugging.

💬 maflcko commented on pull request "build: use make < 3.82 syntax for define directive":
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2725105639)
(unrelated to the make issue, the same conceptual problem exists for the macOS bash version, but I guess the bash version check would be even harder to achieve and is only reliably avoidable by not writing bash code)

💬 vasild commented on pull request "build: enable libc++ hardening in debug builds":
(https://github.com/bitcoin/bitcoin/pull/31424#issuecomment-2725133536)
@maflcko:

```diff
--- i/src/test/random_tests.cpp
+++ w/src/test/random_tests.cpp
@@ -17,12 +17,18 @@ BOOST_FIXTURE_TEST_SUITE(random_tests, BasicTestingSetup)

BOOST_AUTO_TEST_CASE(osrandom_tests)
{
BOOST_CHECK(Random_SanityCheck());
}

+BOOST_AUTO_TEST_CASE(t)
+{
+ std::vector<int> v{1, 2, 3};
+ (void)v[10];
+}
+
BOOST_AUTO_TEST_CASE(fastrandom_tests_deterministic)
{
// Check that deterministic FastRandomContexts are deterministic
SeedRandomFor
...

💬 Sjors commented on issue "v29.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/32052#issuecomment-2725161513)
This was fixed in #32064. So I guess we can't do the guix-sign step until the next release candidate?

💬 Sjors commented on pull request "build: Remove manpages when making MacOS app":
(https://github.com/bitcoin/bitcoin/pull/32064#issuecomment-2725164412)
Having the man files in the `.tar.gz` release, where `bitcoind` and `bitcoin-cli` live, should be enough.

💬 l0rinc commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2725167625)
> since the BufferedReadOnlyFile class implemented there duplicated functionality of AutoFile, didn't support writing, and didn't work with any type of stream

There was a writer as well, but it resulted in some [weird CI failures](https://github.com/bitcoin/bitcoin/pull/31539#issuecomment-2612262087) and given this alternative I just closed the PR.

> disappointed if you initially had a general solution that made it easy to enable buffering any place and then were told that instead of using
...

💬 davidgumberg commented on pull request "test: Add support for mainnet addresses in address_to_scriptpubkey":
(https://github.com/bitcoin/bitcoin/pull/32060#discussion_r1995892980)
What you're saying doesn't make any sense

💬 davidgumberg commented on pull request "test: Add support for mainnet addresses in address_to_scriptpubkey":
(https://github.com/bitcoin/bitcoin/pull/32060#discussion_r1995894067)
You could also apply the suggestion yourself

💬 yancyribbens commented on pull request "test: replace hardcoded fee with node relay fee based calculation":
(https://github.com/bitcoin/bitcoin/pull/32058#issuecomment-2725238772)
Concept ACK. The changes appear to address the TODO correctly.

💬 instagibbs commented on pull request "test: Rename send_message to send_without_ping":
(https://github.com/bitcoin/bitcoin/pull/31859#issuecomment-2725240640)
ACK https://github.com/bitcoin/bitcoin/pull/31859/commits/fa9cf38ab666b50b0d8e82cb17f9e5a8a613547d

Some one-time churn that should help future-proof testing code a bit.

💬 yancyribbens commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#discussion_r1995938540)
```suggestion
// (with N being the number of peers from which we're downloading validated blocks), disconnect due to timeout
```

💬 ryanofsky commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2725260200)
> I can get behind that, can you point me to other usages where we could use this?

I don't know, but if I were looking I'd just grep for places where AutoFile class is used. I don't have any real concerns here and I'm fine with the current approach. To the extent I do have concerns they are about readability and maintainability of code not performance, so I like an approach that makes it trivial to turn buffering on and off, which is I why thought BufferedReader / BufferedWriter stream adapte
...

💬 yancyribbens commented on pull request "fuzz: split `coinselection` harness":
(https://github.com/bitcoin/bitcoin/pull/31870#issuecomment-2725268217)
Concept ACK. I'm curious if there should be any doc updates accompanying this change.

💬 janb84 commented on pull request "contrib: Update coverage.cpp to drop linux restriction":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1995978301)
### Main problem:

> "macOS’s linker (ld64) doesn’t silently resolve unreferenced weak symbols to NULL when they’re called—it demands a definition".

Combined with calling `__gcov_reset` and `__llvm_profile_reset_counters` directly, forcing the linker to find a definition, will result in that the linker will break on trying to link the following code:
```C
extern "C" void __llvm_profile_reset_counters(void) __attribute__((weak));
extern "C" void __gcov_reset(void) __attribute__((weak)
...

💬 janb84 commented on pull request "contrib: Update coverage.cpp to drop linux restriction":
(https://github.com/bitcoin/bitcoin/pull/32059#issuecomment-2725326675)
In my write-up of the explanation to hodlinator I discovered one edge case that needed fixing, sorry for the new commit.

Reason:
Weak fallbacks (__attribute__((weak))) were added instead of strong ones to ensure the profiling runtime’s implementations are used when profiling is enabled, **_avoiding linker conflicts or silent overrides_**. This guarantees correct counter resets while keeping safe, empty fallbacks when profiling is off, improving portability and reliability across Clang and G
...

💬 VolodymyrBg commented on pull request "test: Add support for mainnet addresses in address_to_scriptpubkey":
(https://github.com/bitcoin/bitcoin/pull/32060#issuecomment-2725346210)
@maflcko Done

💬 VolodymyrBg commented on pull request "test: Add support for mainnet addresses in address_to_scriptpubkey":
(https://github.com/bitcoin/bitcoin/pull/32060#issuecomment-2725350636)
@davidgumberg I applied myself. Thank you

💬 zzzi2p commented on pull request "i2p: make a time gap between creating transient sessions and using them":
(https://github.com/bitcoin/bitcoin/pull/32065#issuecomment-2725369358)
Thanks for CC'ing me. I also asked eyedeekay and orignal from i2pd to take a look.

10 standby sessions is a lot, except maybe at startup. If your I2P connections limit is 10, then you're doubling your network resource usage. 2 or 3 is probably enough, depending on how fast your connections churn and how long is long enough for a unused session to be up to give you the anonymity you're looking for.

A couple of alternatives:

```c
int max = MAX_UNUSED_I2P_SESSIONS_SIZE;
if (uptime() > 15
...