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

💬 sipa commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1668632974)
I don't seem to be able to get it to work. It may be that's because the iterator doesn't actually encapsulate a pointer iterator, but a pointer to where in the hashtable the entry exists or so. If so, then that just isn't possible.

💬 ryanofsky commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1668634381)
> I wrote a simple patch to make the existing `Result` more flexible (allow inner error types other than `bilingual_str`), for places that want to use something like `std::expected<A, B>` in code today by using `Result<A, B>`.

I think it could be a good idea to introduce a `util::Expected<A, B>` class if you see places in our code that need to return structured failure information but do not need to return error messages along with the failure data. If we have a `util::Expected` class it shou
...

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

This seems quite tricky to have test coverage for (since it only manifests in `bitcoin-qt`), and unfortunately the `EXCLUSIVE_LOCKS_REQUIRED(!m_mutex)` annotations didn't catch this either, so I'm not sure if there's much more we can do to prevent regressions?

Review note: easy steps to reproduce are [here](https://github.com/bitcoin/bitcoin/issues/30400#issuecomment-2213692311). Thanks again for finding this @achow101 .

💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#issuecomment-2214110228)
> Needs rebase (for #29625)

Currently working on some simulations, will rebase soon