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

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#issuecomment-2213708169)
Compared to faacd264417bd7f3f08c5ff497458030b3a54fbc I'm now getting "Could not determine [IPv4/IPv6] default gateway". The changes to `defined(__APPLE__)` in `netif.cpp` don't give me any immediate hint of what could explain this.

💬 dergoegge commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1668437385)
You mean test a global and local cache? We shouldn't fuzz globals (unless we can reset their state).

The reason I commented is that globals make fuzz tests non-deterministic. Let's say the harness is called (in-process) with input A, B and then C. It crashes on input C (which will be the input that the fuzz engine reports to you) but it only crashed because of the data stored in the global cache from input A and B. Giving the harness just input C won't make it crash, because it depends on the
...

💬 maflcko commented on pull request "policy: Add PayToAnchor(P2A), `OP_TRUE <0x4e73>` as a standard output script for spending":
(https://github.com/bitcoin/bitcoin/pull/30352#discussion_r1668438746)
5e7e767bc79682de3879b8546294d50542570092: Intentional to make p2sh-p2a spend standard as well? Would be good to have a test as documentation, so that reviewers know what to expect and don't have to guess.

💬 fjahr commented on pull request "test: Remove already resolved assumeutxo todo comment":
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2213718691)
> I wonder if this is a good place to discuss other (potentially addressed) TODOs?

Right, good idea. I avoided this for a while because I thought I might miss something because I misinterpret them but since we had these detailed discussions in #29996 I am now pretty confident we can remove some that are addressed.

> For example, interesting starting states could be loading a snapshot when the current chain tip is:
>
> `- TODO: An ancestor of snapshot block`: isn't this already addressed
...

💬 fjahr commented on pull request "test: Remove already resolved assumeutxo todo comment":
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2213733504)
I have amended the commit to remove the previously named comments + made @alfonsoromanz co-author.

In the second commit I now resolve one of the few remaining cases where the tip is on the same block as the snapshot. This is a special case of the "more work" case so this was quite simple to do.

I think this means with this and the remaining open test PRs from @alfonsoromanz and @mzumsande we could resolve this list completely :)