👍 vasild approved a pull request: "fuzz: add I2P harness"
(https://github.com/bitcoin/bitcoin/pull/30230#pullrequestreview-2113132000)
ACK 193c748e44f8647a056121fc9cbb9c2efbcbfc49

💬 vasild commented on pull request "fuzz: add I2P harness":
(https://github.com/bitcoin/bitcoin/pull/30230#discussion_r1636531569)
> I think it's an acceptable tradeoff ...

Ok.

> ... advancing time would be a more accurate simulation. Maybe we leave this as a potential follow up ...

Right, further changes in `FuzzedSock` itself are kind of out of scope for this PR which resurrects the previously deleted test.

Thanks!

💬 sipa commented on pull request "Update minisketch subtree to eb37a9b8e79f9e49d73b96a49bf97a96d9eb676c":
(https://github.com/bitcoin/bitcoin/pull/30270#issuecomment-2163112473)
utACK 89464ad59cf11f68315ea3104236989e5b429d15

✅ maflcko closed a pull request: "doc: Mention EOL policy in release notes template"
(https://github.com/bitcoin/bitcoin/pull/30271)

💬 maflcko commented on pull request "doc: Mention EOL policy in release notes template":
(https://github.com/bitcoin/bitcoin/pull/30271#issuecomment-2163134086)
Closing for now as up-for-grabs.

⚠️ okabri opened an issue: "Btc"
(https://github.com/bitcoin/bitcoin/issues/30274)

👍 ryanofsky approved a pull request: "Encapsulate warnings in generalized node::Warnings and remove globals"
(https://github.com/bitcoin/bitcoin/pull/30058#pullrequestreview-2113194341)
Code review ACK 4ca48b328ba601299fc5aa4efc9749420590a022. Just rebase and a change to internal commits (removing DoWarning earlier) since last review.

✅ fanquake closed an issue: "Btc"
(https://github.com/bitcoin/bitcoin/issues/30274)

:lock: fanquake locked an issue: "Btc"
(https://github.com/bitcoin/bitcoin/issues/30274)

👍 ryanofsky approved a pull request: "p2p: For assumeutxo, download snapshot chain before background chain"
(https://github.com/bitcoin/bitcoin/pull/29519#pullrequestreview-2113216701)
Code review ACK ac547ea43beb1d66d2c11e7e8abb76dd1bfc2883. This is an improvement over the status quo in the normal case, though it seems like it introduces some odd behavior if snapshot block is not on the most work chain, as described in other comments. I think it would be nice to clean the code up more and clarify what is trying to do, instead of merging this change as it is. But it would also be ok to merge in its current form, if you prefer that.

🤔 BrandonOdiwuor reviewed a pull request: "assumeutxo: Check snapshot base block is not in invalid chain"
(https://github.com/bitcoin/bitcoin/pull/30267#pullrequestreview-2113239131)
Code Review ACK cbc604f6a7739bf22a5d811b6366453860d59f4c

💬 BrandonOdiwuor commented on pull request "assumeutxo: Check snapshot base block is not in invalid chain":
(https://github.com/bitcoin/bitcoin/pull/30267#discussion_r1636599996)
Nit: Would it be better to include the base block hash on the error message to help in debugging

```cpp
throw JSONRPCError(
RPC_INTERNAL_ERROR,
"The base block header (%s) is part of an invalid chain.", base_blockhash.ToString());
```

👍 ryanofsky approved a pull request: "Introduce Mining interface"
(https://github.com/bitcoin/bitcoin/pull/30200#pullrequestreview-2113260997)
Code review ACK 4f9c3369a453f7e5f0c12179dd629ee8fc042379. Only changes since last review are: restoring ApplyArgsManOptions function and inconclusive-not-best-prevblk check order, splitting up an internal commit, and improving some commit messages.

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2163209582)
Not sure what is wrong with Apple, but I don't have any insight into their XCode/Apple-Clang versioning and source code, so I think I'll just clarify that this needs Apple-Clang 16 (the meaning of which is unclear).

💬 maflcko commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2163230498)
Ref:

```
/Applications/Xcode_15.0.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/map:1282:24: note: in instantiation of function template specialization 'std::__tree<std::__value_type<std::string_view, FuzzTarget>, std::__map_value_compare<std::string_view, std::__value_type<std::string_view, FuzzTarget>, std::less<std::string_view>>, std::allocator<std::__value_type<std::string_view, FuzzTarget>>>::__emplace_unique_key_args<std::string_view, co
...

💬 ryanofsky commented on issue "Add bitcoind and bitcoin-cli to macOS release":
(https://github.com/bitcoin/bitcoin/issues/30262#issuecomment-2163278278)
re: https://github.com/bitcoin/bitcoin/issues/30262#issue-2344196072

> I vaguely remember an earlier discussion about this, but can't find it.

I'm not sure if this is is what you were thinking ofm but we previously discussed "adding helper applications alongside the main application" in a single DMG on macos in https://github.com/bitcoin-core/gui/pull/414. But these were testnet/regtest versions of the GUI, not command line apps.

👍 TheCharlatan approved a pull request: "Encapsulate warnings in generalized node::Warnings and remove globals"
(https://github.com/bitcoin/bitcoin/pull/30058#pullrequestreview-2113334850)
Re-ACK 4ca48b328ba601299fc5aa4efc9749420590a022

📝 ismaelsadeeq opened a pull request: "Fee Estimation: change `estimatesmartfee` default mode to `economical`"
(https://github.com/bitcoin/bitcoin/pull/30275)

Fixes #30009

This PR changes the `estimatesmartfee` default mode to `economical`. Since users are likely to use the default mode, this change will reduce overestimation for many users. The conservative mode remains available for those who wish to opt-in.

For an in-depth analysis of how significantly the `conservative` mode overestimates, see https://delvingbitcoin.org/t/bitcoind-policy-estimator-modes-analysis/964.

💬 achow101 commented on issue "Add bitcoind and bitcoin-cli to macOS release":
(https://github.com/bitcoin/bitcoin/issues/30262#issuecomment-2163314131)
> Including them in the normal download zip, perhaps in a folder called "Utilities", so that it's visually distinct from the GUI icon.

I think they would still have to be code signed and that could be slightly problematic? AFAIK code signing on MacOS currently requires extra data that exists in the app bundle that would not be available to standalone binaries.

💬 mzumsande commented on pull request "p2p: For assumeutxo, download snapshot chain before background chain":
(https://github.com/bitcoin/bitcoin/pull/29519#discussion_r1636684790)
> And you seem to be suggesting:
Loading a snapshot should force a node to only consider chains that include the snapshot block and ignore other chains, even if they have more work.

This was meant more as a description of the status quo than a description of the ideal world. I think that with today's logic we don't really have another choice but to do so - temporarily until the background sync has finished.

> but the snapshot block is not on the most-work chain seems different and worth t
...