💬 Sjors commented on pull request "assumeutxo: net_processing changes":
(https://github.com/bitcoin/bitcoin/pull/24008#discussion_r1195472070)
058fecdc7d: could still move `if (can_serve` out of the loop to reduce brackets.

⚠️ sdaftuar opened an issue: "Proposal for a new mempool design"
(https://github.com/bitcoin/bitcoin/issues/27677)
@sipa and I gave a presentation to some other developers recently about some work we've been doing to redesign how the mempool works, and I wanted to share a writeup outlining our thoughts with a broader audience to further the discussion. I've also attached a PDF of slides that go along with this: [Reinventing the Mempool.pdf](https://github.com/bitcoin/bitcoin/files/11490409/Reinventing.the.Mempool.pdf).

--------

# Summary

The current mempool is primarily designed around maintaining
...

🤔 Sjors reviewed a pull request: "assumeutxo: net_processing changes"
(https://github.com/bitcoin/bitcoin/pull/24008#pullrequestreview-1429084243)
Happy with the improvements in ac9adf012925c770dfe523c5b57451f313cc8be5.

I wrote:

> The first commit tends to confuses me

Could use some thoughts on that comment (from anyone really).

@sdaftuar wrote:

> The first commit "validation: introduce `ChainstateManager::GetChainstateForNewBlock"` seems like something we don't conceptually need.

I'm pretty close to wrapping my head around it, so would be OK with merging it once I do _and then_ refactor it. Avoiding the complexity in the
...

💬 mzumsande commented on pull request "p2p: skip netgroup diversity follow-up":
(https://github.com/bitcoin/bitcoin/pull/27467#issuecomment-1550098252)
Code Review ACK 11bb31c1c43b5da36ca8509b5747abfb3278ffcd

This fixes an outdated doc, so it's an improvement.

> The naming is now misleading.

I don't think the previous naming was too bad: #27343 added an explanatory comment after all. If anything, it was much more misleading before #27343 when there was no comment, because the name `setConnected` doesn't imply "netgroups of outbound peers from all networks" any more than it implies "netgroups of ipv4/ipv6 outbound peers" - after all we
...

💬 MarcoFalke commented on pull request "ci: Run iwyu on all src files":
(https://github.com/bitcoin/bitcoin/pull/27571#discussion_r1195497290)
because it crashes. See:


```
# python3 "/include-what-you-use/iwyu_tool.py" -p . ./src/wallet/walletutil.cpp -j 9 -- -Xiwyu --cxx17ns -Xiwyu --mapping_file="$PWD/contrib/devtools/iwyu/bitcoin.core.imp"
In file included from wallet/walletutil.cpp:5:
In file included from ./wallet/walletutil.h:8:
In file included from ./script/descriptor.h:8:
In file included from ./outputtype.h:9:
In file included from ./script/signingprovider.h:10:
In file included from ./key.h:10:
In file include
...

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1550107951)
Re https://github.com/bitcoin/bitcoin/pull/27636#pullrequestreview-1428804291
> I think the last commit could just be dropped, or I don't understand what problem it is solving.

The rationale for the commit was to preserve `bitcoin-chainstate` shutting down when a fatal error is encountered. I thought this through again, and I agree that this does not make a lot of sense. The function where this fatal error encountered should always be bubbling it up anyway, so this is not needed. Dropping th
...

💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#discussion_r1195519598)
Reproduced and fixed!

👍 hebasto approved a pull request: "ci: Run iwyu on all src files"
(https://github.com/bitcoin/bitcoin/pull/27571#pullrequestreview-1429129896)
ACK ddddf4957b02c83ed9b6c46b35d8ae1e137889d2

💬 MarcoFalke commented on pull request "build: Drop support for g++-8":
(https://github.com/bitcoin/bitcoin/pull/27662#issuecomment-1550157077)
> Could pull in this commit, to remove the GCC 8 -lstdc++fs check: https://github.com/fanquake/bitcoin/commit/b2632e7c7b7dfc5487629d80e11a8cdeaa649ed9.

My preference would be to also bump clang and then just remove the whole check instead of fiddling with it, when all supported distros ship clang-10 or higher already.

💬 darosior commented on pull request "MiniTapscript: port Miniscript to Tapscript":
(https://github.com/bitcoin/bitcoin/pull/27255#issuecomment-1550158814)
Rebased and updated.
- I addressed @josibake's comments. (thanks for the review!)
- I fixed the `multi_a` satisfaction to use an algorithm similar to `multi`'s.
- Modified the maximum script size to account for some breathing room for the spending transaction size.
- Made the execution size accounting easier by not tracking the size of the stack after execution of a fragment, instead assuming the maximum amount of stack elements the fragment can consume were consumed. The number of items pus
...

💬 fanquake commented on pull request "build: Drop support for g++-8":
(https://github.com/bitcoin/bitcoin/pull/27662#issuecomment-1550173042)
Are you going to pull a Clang bump into this PR? I don't see the point of leaving redundant code, especially when the diff is trivial to review.

💬 MarcoFalke commented on pull request "Raise on invalid -debug and -loglevel config options":
(https://github.com/bitcoin/bitcoin/pull/27632#discussion_r1195604095)
unrelated: A minimal (but hacky) way to add support for `void` to `Result` would be:


```diff
diff --git a/src/util/result.h b/src/util/result.h
index 972b1aada0..b99995c7e5 100644
--- a/src/util/result.h
+++ b/src/util/result.h
@@ -31,16 +31,19 @@ struct Error {
//! `std::optional<T>` can be updated to return `util::Result<T>` and return
//! error strings usually just replacing `return std::nullopt;` with `return
//! util::Error{error_string};`.
-template <class T>
+template <c
...

💬 theuni commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1550239909)
> Note that you'll also need to update our minimum version check in:
>
> https://github.com/bitcoin/bitcoin/blob/904631e0fc00ac9c8a03d1ce226d071bf88c00db/contrib/devtools/symbol-check.py#L235

Thanks. Done. I used #22993 as a template for bumping, we'll see if c-i is happy.

📝 brunoerg opened a pull request: "fuzz: net, add `recv_flood_size`, `prefer_evict` in `ConsumeNode`"
(https://github.com/bitcoin/bitcoin/pull/27678)
This PR adds `recv_flood_size` and `prefer_evict` in `CNodeOptions` when creating a new CNode in `ConsumeNode`. I noticed they were missing while working on an improvement for net fuzzing.

Checked that #27324 added `recv_flood_size` into `CNodeOptions` and #25962 added`prefer_evict`.

💬 TheCharlatan commented on pull request "kernel: Remove interface_ui, util/system from kernel library":
(https://github.com/bitcoin/bitcoin/pull/27636#issuecomment-1550261815)
Thank you for the review,

Updated 2c58fbf816d73395167a3046c4ce957829bf45f9 -> b29eab3fc68de28fe2aa67700fd99c6744e37f61 ([chainstateRmKernelUiInterface_4](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_4) -> [chainstateRmKernelUiInterface_5](https://github.com/TheCharlatan/bitcoin/commits/chainstateRmKernelUiInterface_5), [compare](https://github.com/TheCharlatan/bitcoin/compare/chainstateRmKernelUiInterface_4..chainstateRmKernelUiInterface_5)).

* Addressed @r
...

👍 theStack approved a pull request: "test: Return dict in MiniWallet::send_to"
(https://github.com/bitcoin/bitcoin/pull/27640#pullrequestreview-1429300073)
Code-review ACK faf4315c88d8c81c2ff84870bc81aef3cf719816

💬 joostjager commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1550285007)
What are the current use cases for transaction packages enabled by this PR that are more complicated than child-with-parents-tree-only?

📝 pinheadmz opened a pull request: "ZMQ: Support UNIX domain sockets"
(https://github.com/bitcoin/bitcoin/pull/27679)
This is a follow-up to https://github.com/bitcoin/bitcoin/pull/27375, allowing ZMQ notifications to be published to a UNIX domain socket, adding one more commit.

Fortunately, libzmq handles unix sockets already, all we really have to do to support it is allow the format in the actual option. libzmq uses the prefix `ipc://` as opposed to `unix:` which is used by Tor and by #27375 so, for now I'm testing with both being allowed.

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1550292949)
@joostjager could be as simple as any wallet that does CPFP bumping in their coin selection algorithm(chaining unconfirmed spends). The ideal is to support as many usage patters exist in practice, ancestor packages is just a large-ish subset of that that from a CPFP point of view.

💬 joostjager commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#issuecomment-1550315212)
I am trying to get a feel for how big of a problem it is that this PR solves. In Lightning the problem is very real and potentially exposing lots of users to coin loss because of the combination of pre-signed transactions and time sensitivity, but the topology is of the simplest kind. I am not sure if the wallet example that you give is of the same order because RBF is also an option and timing may not be as critical?

Of course it is great to support as many patterns as possible, but it also
...