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

💬 Christewart 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-2377685948)
> > > 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?

That makes sense to me, but i'm not us
...

💬 maflcko commented on pull request "init: Remove retry for loop":
(https://github.com/bitcoin/bitcoin/pull/30968#discussion_r1777604226)
Unrelated to this pull, the comment is wrong. `Shutdown()` *is* called. So it would be good (in a follow-up), or if you re-touch, to remove the incorrect and confusing comment:


```diff
diff --git a/src/init.cpp b/src/init.cpp
index ebe48dc307..5f786fd4df 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1649,7 +1649,6 @@ bool AppInitMain(NodeContext& node, interfaces::BlockAndHeaderTipInfo* tip_info)

// As LoadBlockIndex can take several minutes, it's possible the user
//
...

⚠️ ryanofsky opened an issue: "RFC: Multiprocess binaries and packaging options"
(https://github.com/bitcoin/bitcoin/issues/30983)
Issue for discussion about ways multiprocess functionality could be packaged and released.

### Binaries

One goal of the [multiprocess project](https://github.com/bitcoin/bitcoin/issues/28722) has been to provide minimal binaries that only include node, wallet, or gui code, which spawn or connect to other processes as needed to provide other functionality.

The idea implemented in #10102 is to have 3 binaries:

- **`bitcoin-node`** - contains node code and libraries (leveldb), and no wa
...

💬 maflcko commented on pull request "ci: Inline PACKAGE_MANAGER_INSTALL":
(https://github.com/bitcoin/bitcoin/pull/30974#issuecomment-2377728205)
(rebased)

👍 ryanofsky approved a pull request: "refactor: Replace g_genesis_wait_cv with m_tip_block_cv"
(https://github.com/bitcoin/bitcoin/pull/30967#pullrequestreview-2332196385)
Code review ACK fa24d8dbddebd61b9c418bd6773c333ad72bed67. Fixed wait_for overflow bug since last review

💬 ryanofsky commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1777641007)
In commit "refactor: Add missing GUARDED_BY(m_tip_block_mutex)" (fa47d893acdd18132204810dd95880ab21abf415)

It seems like it would be a little better to make this change in the next commit "refactor: Use wait_for predicate to check for interrupt" (faa3f411f6842bf1201b25d6391b1913e1b72439) instead of this one so this code is not changing more than once.

💬 ryanofsky commented on pull request "refactor: Replace g_genesis_wait_cv with m_tip_block_cv":
(https://github.com/bitcoin/bitcoin/pull/30967#discussion_r1777642365)
In commit "refactor: Use wait_for predicate to check for interrupt" (faa3f411f6842bf1201b25d6391b1913e1b72439)

Might be better to avoid the 100 year hack by switching to wait_until

```diff
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -940,10 +940,10 @@ public:

BlockRef waitTipChanged(uint256 current_tip, MillisecondsDouble timeout) override
{
- if (timeout > std::chrono::years{100}) timeout = std::chrono::years{100}; // Upper bound to avoid UB in
...

💬 ryanofsky commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2377800457)
> For my part, I need to fix the windows build and also open an issue listing different ways it would be possible to make releases including multiprocess support in short or long term.

Update: currently working on getting mingw32 build working, and I created issue #30983 to lay out different multiprocess release options.

💬 maflcko commented on issue "Test p2p_headers_presync fuzz target on macOS/Windows":
(https://github.com/bitcoin/bitcoin/issues/30950#issuecomment-2377843249)
> Wouldn't a developer looking to fuzz test set `-DBUILD_FOR_FUZZING=ON`?

Most likely yes, but it isn't enforced. For example, it is possible to compile the fuzz binary without `DBUILD_FOR_FUZZING` and execute it. Support for that is also added to the `fuzz/test_runner.py` script. (This is also what the failing CIs were using to call the fuzz binary).

> Would future harnesses relying on FUZZING_BUILD_MODE_UNSAFE_FOR_PRODUCTION also do an early return?

I'd say it depends on the fuzz targ
...

💬 mzumsande commented on pull request "kernel: Move block tree db open to block manager":
(https://github.com/bitcoin/bitcoin/pull/30965#discussion_r1777703574)
This changes the error handling if the block tree db is corrupted a bit worse as a side effect. Before, we'd get the advice "Please restart with -reindex or -reindex-chainstate to recover." on the console, and, if using the GUI, the option to reindex with by clicking a button. Now we just get a "Error opening block database" error on both without any advice how to fix it.

I'm not sure if the GUI one-click reindex option is totally essential, but in my opinion we should at least keep the advic
...

🤔 mzumsande reviewed a pull request: "kernel: Move block tree db open to block manager"
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2332298896)
Concept ACK

💬 instagibbs commented on pull request "Disable RBF Rule 2":
(https://github.com/bitcoin/bitcoin/pull/30964#issuecomment-2377936088)
@sdaftuar I think the same reasoning I gave is just being applied recursively, which is fine :+1:

🤔 hodlinator reviewed a pull request: "rpc: add getorphantxs"
(https://github.com/bitcoin/bitcoin/pull/30793#pullrequestreview-2332231261)
Concept ACK

Feels like *p2p_orphan_handling.py*/`test_same_txid_orphan()` might benefit from some augmented checking using this new `getorphantxs`. It could verify that same-txids-differing-wtxid orphans are handled in the expected way, with the bogus one being removed after the one with the valid witness gets adopted.

💬 hodlinator commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#discussion_r1777666525)
My guess is that this weird indentation style is an artifact of refactorings not wanting to re-indent old lines. Should not apply to new code.