💬 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 :)

💬 fjahr commented on pull request "assumeutxo: Don't load a snapshot if it's not in the best header chain":
(https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1668453002)
Actually also this TODO, I think these basically mean the same thing: `- TODO: Not an ancestor or a descendant of the snapshot block and has more work`

💬 fjahr commented on pull request "test, assumeutxo: Remove resolved todo comments and add new test":
(https://github.com/bitcoin/bitcoin/pull/30403#issuecomment-2213758446)
To give more detail where they are resolved: I think #30320 resolves two of the remaining three, it's just not part of the PR yet, see: https://github.com/bitcoin/bitcoin/pull/30320#discussion_r1665811272 and #29996 removes the third as already done in the commit here: https://github.com/bitcoin/bitcoin/pull/29996/commits/5b7f70ba2661a194a3c476b81e03785feddb4e1c.

The last one to get merged gets to remove the whole comment section 🥳

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668523385)
9a067b5ac154154ec9008a0e8f2d8f03e994a589: this is triggered on my Intel macOS 14.5 machine. If I comment out the `if` statement it works fine.

💬 TheBlueMatt commented on pull request "Stratum v2 Template Provider (take 3)":
(https://github.com/bitcoin/bitcoin/pull/29432#issuecomment-2213907464)
> This seems to be the main argument and I don't find this to be true at all. All lightning implementations, ecash mints, electrum servers, alternative GUIs and block explorers are sidecars. Sure, putting all of these into Bitcoin Core would be convenient for users but it would be an absolute nightmare for Bitcoin Core as a software project. I think this just boils down to how useful the sidecar actually is: if there is demand for the sidecar then users will run it.

And compared to Bitcoin Core
...

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668527084)
```
msg_pos=0 sizeof(rt_msghdr)=92 buf.size()=10948
```

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668531523)
For IPv4:

```
sa->sa_len=16 sa_pos=92 next_msg_pos=144
sa->sa_len=16 sa_pos=108 next_msg_pos=144
sa->sa_len=0 sa_pos=124 next_msg_pos=144
2024-07-08T12:24:02Z [mapport] [net] portmap: Could not determine IPv4 default gateway
```

For IPv6:

```
sa->sa_len=28 sa_pos=92 next_msg_pos=180
sa->sa_len=28 sa_pos=120 next_msg_pos=180
sa->sa_len=0 sa_pos=148 next_msg_pos=180
2024-07-08T12:24:02Z [mapport] [net] portmap: Could not determine IPv6 default gateway
```

👍 alfonsoromanz approved a pull request: "test, assumeutxo: Remove resolved todo comments and add new test"
(https://github.com/bitcoin/bitcoin/pull/30403#pullrequestreview-2163153435)
Re ACK 29d19c414c9e138c99f8de84e398528db92ff07e. The test execution is successful.

I agree that this PR, along with [#30320](https://github.com/bitcoin/bitcoin/pull/30320) and [#29996](https://github.com/bitcoin/bitcoin/pull/29996), would resolve the TODO list completely.

Thanks!

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668537577)
Maybe replacing `return std::nullopt;` with `continue;`?

💬 fjahr commented on pull request "contrib: asmap-tool - Compare ASMaps with respect to specific addresses":
(https://github.com/bitcoin/bitcoin/pull/30246#discussion_r1668537763)
It's fine to leave it as is. It was just a nit :)

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668563657)
Or maybe the following:

```cpp
if (rt->rtm_addrs & (1 << i)) {
// 2 is just sa_len + sa_family, the theoretical minimum size of a socket address.
if ((sa_pos + 2) > next_msg_pos) return std::nullopt;
const struct sockaddr* sa = (const struct sockaddr*)(buf.data() + sa_pos);
fprintf(stderr, "i=%d sa->sa_len=%hhu sa_pos=%lu next_msg_pos=%zu\n", i, sa->sa_len, sa_pos, next_msg_pos);
if (sa->sa_len >
...

💬 fjahr commented on pull request "contrib: asmap-tool - Compare ASMaps with respect to specific addresses":
(https://github.com/bitcoin/bitcoin/pull/30246#issuecomment-2213984176)
tACK 5215c925d1382e71c9e1d642fced8a152c629c7f

I played around with a few different input files, results look reasonable and I think this is a very helpful for analysis of generated asmap files.

For anyone who wants to test but is missing asmap files, you can use the ones here: https://github.com/fjahr/asmap-data

💬 andrewtoth commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1668598683)
Right, thanks!

👍 willcl-ark approved a pull request: "Use `WITH_LOCK` in `Warnings::Set`"
(https://github.com/bitcoin/bitcoin/pull/30404#pullrequestreview-2163288986)
ACK 6af51e819847e737449609daa214e16f9453e85d

It's generally best-practice to hold locks for the shortest possible time to minimize contention.

💬 Sjors commented on pull request "refactor: add coinbase constraints to BlockAssembler::Options":
(https://github.com/bitcoin/bitcoin/pull/30356#issuecomment-2214078752)
> This is just limiting the max block weight

This had me confused initially, and I tried to combine them, but there's really two different things:

1. The max block weight demanded by the user via `-blockmaxweight`
2. Weight units reserved for the coinbase

With stratum v1 the distinction isn't very important. We just subtract (2) from (1). We could indeed do that at initialization.

But with Stratum v2 (2) is a per-client setting communicated to us (the Template Provider) via the [Coi
...