💬 Sjors commented on issue "depends: capnp build ignores config_opts":
(https://github.com/bitcoin/bitcoin/issues/32068#issuecomment-2725004999)
Opened #32070 to fix it.

💬 Sjors commented on pull request "build: use make < 3.82 syntax for define directive":
(https://github.com/bitcoin/bitcoin/pull/32070#issuecomment-2725007393)
We don't seem to specify a minimum make version?

💬 ryanofsky commented on pull request "[IBD] batch block reads/writes during `AutoFile` serialization":
(https://github.com/bitcoin/bitcoin/pull/31551#issuecomment-2725048933)
No strong opinion. I guess I'd be a little 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 that, you should add kludges to this code and implement ad-hoc buffering here. I don't think buffering is difficult to reason about and don't think buffering is the type of thing that needs to be deployed on a case-by-case basis, especially since performance of buffering should mostly depend on *where* data is bei
...

💬 darosior commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1995800292)
BIP9 does not define the period as configurable, it's always the retarget period. No strong opinion, but i find it surprising that the BIP9 warning mechanism should support (a specific type of) non-BIP9 deployments.

👍 ryanofsky approved a pull request: "build: use make < 3.82 syntax for define directive"
(https://github.com/bitcoin/bitcoin/pull/32070#pullrequestreview-2685973287)
Code review ACK 9157d9e449870851ef455e077249ac46fc2df24c. 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.

(Another case where there was a problem with make 3.81 was https://github.com/bitcoin-core/libmultiprocess/issues/139 / https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2621793644)

💬 vasild commented on pull request "net: replace manual reference counting of CNode with shared_ptr":
(https://github.com/bitcoin/bitcoin/pull/32015#issuecomment-2725084083)
Further idea around the second sentence of this:

https://github.com/bitcoin/bitcoin/pull/10738#issue-240283546:
> Special care must be taken, though, to only delete CNodes from a single thread, and to control which thread that is. Eventually, this should move to the message processing thread, so that it's not possible to hit cs_main from the main network thread.

It is possible to decouple `CNode` deletion from the `FinalizeNode()` call and do the latter in the `msghand` thread regardless
...

💬 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
...