💬 willcl-ark commented on pull request "docs: Add instructions on how to self-sign bitcoin-core binaries for macOS":
(https://github.com/bitcoin/bitcoin/pull/30982#issuecomment-2377438048)
> > I think it might be good to also include instructions for signing a downloaded `Bitcoin-Qt.app`, as I would think that might be what a lot (majority?) of MacOS users are downloading.
>
> I presume this is the GUI? I'm not sure if assuming command line literacy makes sense for that.

Well, as the app does not work at all on macOS without that it seems better to me to at least provide the required steps, rather than nothing at all?

💬 marcofleon commented on issue "Test p2p_headers_presync fuzz target on macOS/Windows":
(https://github.com/bitcoin/bitcoin/issues/30950#issuecomment-2377443531)
Is that better than adding `-x p2p_headers_presync` to the Windows and macOS test runner calls in the CI?

Using the exclude flag, as opposed to altering the harness, feels cleaner to me. Afaik they would both accomplish the same thing, so ultimately I'm okay with either.

💬 vasild commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2377448170)
In https://github.com/bitcoin/bitcoin/issues/29618 and here, I don't see the motivation to forbid v1 connections. In other words, what is the purpose of this?

In addition to the concerns expressed in https://github.com/bitcoin/bitcoin/issues/29618, there is one more - if V2-only is widespread, it may be hard to eclipse V2-only node, but then it becomes easy to eclipse a V1 node which has a problem of finding peers to connect to, so an attacker starts a lot of nodes that do accept V1 connectio
...

💬 TheCharlatan commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1777432701)
The previous loop this refers to is the initialization retry loop that I am getting rid of in #30968.

💬 maflcko commented on issue "Test p2p_headers_presync fuzz target on macOS/Windows":
(https://github.com/bitcoin/bitcoin/issues/30950#issuecomment-2377453308)
What happens in the CI should only be secondary to what developers are exposed to. So the question would be: Why should developers and testers be let to run into the same timeout? If there is no value in running the target when `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` isn't set, then I don't see why developers and testers should be bothered with a timeout.

💬 itornaza commented on issue "depends: llvm-ranlib (etc): "No such file or directory" on Intel macOS 15.0":
(https://github.com/bitcoin/bitcoin/issues/30978#issuecomment-2377459457)
I replicated the same on Apple silicon (Apple M2 Max) and the same setup (macos 15.0 and Xcode 16.0). Also checked that `/usr/bin/{ranlib,strip,nm,objdump,dsymutil}` are present on my system. The compilation process successfully completes.

💬 maflcko commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2377469923)
From CI:

```
SUMMARY: MemorySanitizer: use-of-uninitialized-value /usr/src/mp/proxy.cpp:146:25 in mp::Connection::addAsyncCleanup(std::__1::function<void ()>)

👍 ryanofsky approved a pull request: "refactor: Replace g_genesis_wait_cv with m_tip_block_cv"
(https://github.com/bitcoin/bitcoin/pull/30967#pullrequestreview-2331887884)
Code review ACK fa877f4c5f9abb057f1476f21a063e0f687d0ad1. Since last review, just added new commit with another simplification.

---

Could consider squashing the new commit:

- "refactor: Use wait_for predicate to check for interrupt" (fa877f4c5f9abb057f1476f21a063e0f687d0ad1)

into earlier commit:

- "refactor: Remove dead code in waitTipChanged" (fafffc8b5cc25adf278031561f060023a14e4387)

since they are touching same code, and there should be no need to have one commit simplifyi
...

💬 josibake commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#issuecomment-2377504632)
crACK https://github.com/bitcoin/bitcoin/commit/e9d60af9889c12b4d427adefa53fd234e417f8f6

Came here from reviewing https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1776649353, haven't tested but did review and the code change is relatively straightforward (mostly a move-only change) and definitely improves the readability here.

💬 marcofleon commented on issue "Test p2p_headers_presync fuzz target on macOS/Windows":
(https://github.com/bitcoin/bitcoin/issues/30950#issuecomment-2377507173)
Wouldn't a developer looking to fuzz test set `-DBUILD_FOR_FUZZING=ON`? In which case, that macro is automatically set (along with `ABORT_ON_FAILED_ASSUME`) and there would be no timeout. Would future harnesses relying on `FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION` also do an early return?

Also, sorry I'm misunderstanding your point. I'm not too familiar with what the best practice would be here.

💬 josibake commented on pull request "multiprocess: Add capnp wrapper for Chain interface":
(https://github.com/bitcoin/bitcoin/pull/29409#issuecomment-2377512124)
Concept ACK

Thanks for the rebase, I'll be digging into this next week!

💬 maflcko commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1777476826)
Actually it uncovered a bug in GCC:


```
node1 stderr /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/chrono.h:225:8: runtime error: inf is outside the range of representable values of type 'long'
#0 0x55c8ba4a1562 in std::chrono::duration<long, std::ratio<1l, 1000000000l>> std::chrono::__duration_cast_impl<std::chrono::duration<long, std::ratio<1l, 1000000000l>>, std::ratio<1000000l, 1l>, double, false, true>::__cast<double, std::ratio<1l, 1000l>>(std::chrono::
...

💬 laanwj commented on pull request "contrib: Update asmap link in seeds readme":
(https://github.com/bitcoin/bitcoin/pull/30979#issuecomment-2377536805)
ACK f158993fd5575d3cbf991b6df7bcd2af7cf96c9a
Checked that these are the only mentions of this repo.

💬 ffrediani commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2377541658)
Great improvement to have more home nodes serving content back to network in IPv6, specially in times of IPv4 exhaustion.

What would be the configuration in bitcoin.conf file ?

nat=1 will keep doing for IPv4 only ?
pcp=1 will do for IPv6 only or for both and replace/complement the existing IPv4 and replace nat=1 in the future ?

💬 sipa commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2377557615)
In this PR, `natpmp=1` controls both PCP and NAT-PMP.

💬 maflcko commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1777501331)
I guess the bug is in stdlibc++ as well: https://godbolt.org/z/3az431Gdn

💬 theuni commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1777515026)
Smells familiar. I wonder if it's the same root issue as this: https://github.com/bitcoin/bitcoin/pull/30462#discussion_r1679972352

💬 tdb3 commented on issue "V2 Only Option":
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2377604439)
> This is what I mean by "Tor peers", not IPv4 peers accessed via the Tor network, involving exit nodes and part of the route is through clearnet.

Ah, thanks. I overlooked that

> The Tor entry node is usually on localhost.

True, and using a non-localhost entry node is something the user would already need to be conscious of.

💬 ariard commented on pull request "Disable RBF Rule 2":
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2377634380)
Yes the rbf rule 2 bypass is an old known issue and yes the rbf rules are fairly broken.

For bitcoin clients fee-bumping chain of transactions, they should ensure to never spent from unconfirmed inputs.

💬 maflcko commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1777583955)
Yeah I guess values too large or too small are unsupported in the stdlibs for now and have to be avoided before passing them in.

I've capped at 100 years for now.