💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1668259313)
```
// Create cluster D child ---> K
```

💬 dergoegge commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1668290789)
I think I'd be nicer to not change this interface and I believe it would be trivial to just use `SendMessages`? i.e. push the version message to outbound peers at the start of `SendMessages` (if it hasn't been pushed already).

📝 maflcko opened a pull request: "tidy: modernize-use-equals-default"
(https://github.com/bitcoin/bitcoin/pull/30406)
Prior to C++20, `modernize-use-equals-default` could have been problematic because it could turn a non-aggregate into an aggregate. The risk would be that designated initializers would be enabled where the author did not intend to enable them.

With C++20, aggregate for those is forbidden either way. (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1008r1.pdf)

So enabled it for code clarity, consistency, and possibly unlocking compiler optimizations. See https://clang.llvm.org/ext
...

💬 maflcko commented on pull request "BlockAssembler: return selected packages vsize and feerate":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1668302906)
> Apart from the fuzz test size can never be greater than `4,000,000`?

In blocks, yes. In the mempool, it may depend on the user's config.

💬 maflcko commented on pull request "Add clang-tidy check for thread_local vars":
(https://github.com/bitcoin/bitcoin/pull/30146#discussion_r1668305658)
> modernize-use-equals-default

See https://github.com/bitcoin/bitcoin/pull/30406

💬 maflcko commented on pull request "Use `WITH_LOCK` in `Warnings::Set`":
(https://github.com/bitcoin/bitcoin/pull/30404#issuecomment-2213518857)
lgtm ACK 6af51e819847e737449609daa214e16f9453e85d

💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1668292640)
There are 9 such assertion blocks with almost similar validations on ancestors and descendants.
If the author feels it's worth it, IMO this duplication can be eliminated by accepting few variables as the input in a function and calling it with different arguments.
```
validateTxRelations(txToValidate, ancestorTxs, descendantTxs)
```

👍 rkrux approved a pull request: "Fee Estimation: Ignore all transactions that are CPFP'd"
(https://github.com/bitcoin/bitcoin/pull/30079#pullrequestreview-2162718838)
tACK [cb95f95](https://github.com/bitcoin/bitcoin/pull/30079/commits/cb95f95907cf9fce3a18f2bbbb059e72a897e34e)

`make, test/functional` are successful.

Although, this round of review was focussed on [5e6ff0f](https://github.com/bitcoin/bitcoin/pull/30079/commits/5e6ff0f80e2ed7daadb3abbd5a773b542b9b0f6f)

💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1668268758)
```
// Create cluster D child ---> K
```

💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1668306440)
Interesting that `tx_set` is passed by value here and 2 separate copies are made at the time of calling, which is the need here for separate ancestors and descendants copies.

💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1668295683)
+1 on separating this function out in a file and adding focussed unit tests for it, I feel this is generic enough to be used in several places later.

🤔 glozow reviewed a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2162794247)
reACK 606a7ab

💬 ismaelsadeeq commented on pull request "BlockAssembler: return selected packages vsize and feerate":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1668354967)
thanks the commit is gone now.

🤔 glozow reviewed a pull request: "Use `WITH_LOCK` in `Warnings::Set`"
(https://github.com/bitcoin/bitcoin/pull/30404#pullrequestreview-2162862078)
ACK 6af51e819847e737449609daa214e16f9453e85d

Not familiar with this code at all, but it looks like the handler function `handleNotifyAlertChanged` calls `getStatusBarWarnings`
https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/qt/clientmodel.cpp#L256-L260
which calls `Node::getWarnings()`
https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/qt/clientmodel.cpp#L164-L167
which calls `GetMessages()`
https://github.com/bitcoi
...

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1668366154)
this should probably be moved to a [later commit](https://github.com/bitcoin/bitcoin/pull/28280/commits/92aa98712c604b3e424671b196b8425fabdf74d0#diff-095ce1081a930998a10b37358fae5499ac47f8cb6f25f5df5d88e920a54e0341R189) where you introduce the `Next()` method

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668367047)
> some lame form of authentication

https://github.com/miniupnp/miniupnp/issues/748 (I still need to test the fix PR)

💬 instagibbs commented on pull request "build: Bump clang minimum supported version to 16":
(https://github.com/bitcoin/bitcoin/pull/30263#issuecomment-2213646897)
concept ACK

💬 dergoegge commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2213654157)
It looks like we agree that in terms of efficiency the sidecar approach is not a problem, so I'll argue the other points.

> One important goal there is to support providing that work to untrusted open connections easily

From my perspective, the sidecar approach allows to solve this in a much nicer way (no additional burden on bitcoin core, sidecar can be written in a memory safe language, ...).

> We've seen time and time again "just run this sidecar software next to Bitcoin Core" is not
...

👍 dergoegge approved a pull request: "fuzz: bound some miniscript operations to avoid fuzz timeouts"
(https://github.com/bitcoin/bitcoin/pull/30197#pullrequestreview-2162956899)
utACK 178a1da9c4da955c3dc78e6b03f110f687e82508

💬 stickies-v commented on issue "Double lock detected in `Warnings::GetMessages()`":
(https://github.com/bitcoin/bitcoin/issues/30400#issuecomment-2213692311)
Thanks a lot for finding this @achow101

The bug was introduced in 9c4b0b7ce459765fa1a63b410c3423b90f0d2a5f and affects all `bitcoin-qt` instances on either net. If `bitcoin-qt` was compiled with `--enable-debug` it will lead to a crash, otherwise it will lead to the `scheduler` thread being deadlocked and effectively halting the node. `bitcoind` is not affected by this bug as far as I can see.

Another way to reproduce the bug quickly (on any net) is to:
1. manually shift your system cloc
...