💬 maflcko commented on pull request "Several randomness improvements":
(https://github.com/bitcoin/bitcoin/pull/29625#issuecomment-2197042027)
ACK 42757ae119b8bee208c7c997cb77470c8e624522 🏓

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: ACK 42757ae119b8bee208c7c997cb
...

🤔 ryanofsky reviewed a pull request: "Improve new LogDebug/Trace/Info/Warning/Error Macros"
(https://github.com/bitcoin/bitcoin/pull/29256#pullrequestreview-2148139535)
re: https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-2195631997

> The fact that there's something behind the immediate macro level preventing LogInfo/LogError/LogWarning from being filtered is a weaker argument to use in pushing against added logging outside of LogDebug/LogTrace statements, than them not even accepting a category to potentially filter on. Gotta respect an opinionated API. :)

Understood. If the current restrictions which prevent categories from being filtered out
...

💬 ryanofsky commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1658818412)
re: https://github.com/bitcoin/bitcoin/pull/29256#discussion_r1657145216

> I suggested `constexpr auto EXPECTED = std::to_array({` above but maybe it doesn't work for you? [Should support `constexpr` that way under C++20 as far as I can tell](https://en.cppreference.com/w/cpp/container/array/to_array). Are you using some fancy interface to GitHub that doesn't yet show suggestion-embeds?

No sorry, that works perfectly. I was just careless and only noticed `constexpr` without seeing the rest
...

💬 ryanofsky commented on pull request "wallet, logging: Replace WalletLogPrintf() with LogInfo()":
(https://github.com/bitcoin/bitcoin/pull/30343#issuecomment-2197070499)
Updated and rebased db8c6fc2ca5e1ad99aeda741e3b453f1f7bf7955 -> 29eaec25af32e543df3cff091938c3cb8f7cce17 ([`pr/gwlog.3`](https://github.com/ryanofsky/bitcoin/commits/pr/gwlog.3) -> [`pr/gwlog.4`](https://github.com/ryanofsky/bitcoin/commits/pr/gwlog.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/gwlog.3-rebase..pr/gwlog.4)) with suggestion to use `constexpr`

💬 ryanofsky commented on pull request "Make GUI and CLI tools use the same datadir":
(https://github.com/bitcoin/bitcoin/pull/27409#issuecomment-2197085092)
re: https://github.com/bitcoin/bitcoin/pull/27409#issuecomment-2196478179

> @ryanofsky What is the status of this?

It's a change I'd really like to get back to, but is not ready now. The last time I worked on it, which was months ago, I started writing a Qt test for it but there are so many permutations of configurations (presence/absense of different data directory locations) that I was struggling to write it. I would like to get back to this at some point and I do have work in progress,
...

👍 hebasto approved a pull request: "build: Bump clang minimum supported version to 16"
(https://github.com/bitcoin/bitcoin/pull/30263#pullrequestreview-2148237278)
ACK fa8f53273c7e5965620d31a8c3fe5f223cb76888, I have reviewed the code and it looks OK.

🤔 paplorinc requested changes to a pull request: "Don't empty dbcache on prune flushes: >30% faster IBD"
(https://github.com/bitcoin/bitcoin/pull/28280#pullrequestreview-2138727793)
I have only reviewed half of the PR, will continue soon

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1652853326)
would it make sense to use `cacheCoins.try_emplace` without the `forward_as_tuple` parts (similarly to what I've tried [in a related PR](https://github.com/bitcoin/bitcoin/pull/30326/files#diff-f0ed73d62dae6ca28ebd3045e5fc0d5d02eaaacadb4c2a292985a3fbd7e1c77cR44)) (in a separate commit or PR)

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658390684)
nit:
```suggestion
* write without clearing the cache (CCoinsViewCache::Sync), we must also
```

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658394351)
nit: redundant comment

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658417894)
nit:
```suggestion
// Check that calling `ClearFlags` with 0 flags has no effect
```

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658399777)
Probably not a realistic scenario (may require multithreaded access because of the `m_flags` guard, not sure) , but `m_prev` will never be `null` when we get here, right?

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658743023)
I find it quite confusing that `AddFlags` modifies the linked list as well (vs `GetFlags` only returning the flags, of course). Could make sense to split it to multiple methods or add the side-effect to the name

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658391340)
Nit:
```suggestion
* the parent cache for batch writing. This is a performance optimization
```

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658765626)
Since iteration could misbehave severely and this would pass (e.g. if `Next()` would return `null` we wouldn't even enter the loop), it could make more sense to iterate over the target nodes instead, something like:
```C++
auto node{head.second.Next()};
for (const auto& expected : nodes) {
BOOST_CHECK_EQUAL(&expected, node);
node = node->second.Next();
}
BOOST_CHECK_EQUAL(nullptr, node);
```

(as an added benefit, this has a single `Next()` call)

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1658694832)
TODO: not yet sure why iteration is tied to mutation here

💬 sr-gi commented on pull request "net_processing: make any misbehavior trigger immediate discouragement":
(https://github.com/bitcoin/bitcoin/pull/29575#discussion_r1658897389)
I took a look at this to see if it would make sense to remove the comment, but looks like this is still consistent with the original approach, even after splitting the logic in two for txs and blocks:

https://github.com/bitcoin/bitcoin/commit/ef54b486d5333dfc85c56e6b933c81735196a25d

To me, it seems that, at the very least, this comment would need to be moved one case down, given `BlockValidationResult::BLOCK_MISSING_PREV` triggers misbehavior, so the comment doesn't apply there.

💬 Sjors commented on pull request "Stratum v2 Transport":
(https://github.com/bitcoin/bitcoin/pull/30315#issuecomment-2197155437)
I dropped the refactoring commits e18887d3c45d4c0b2d414392ec33dbc36c0d8df7, cf67a72aea67b8ecf2c24200f69c68d1e7f4de4b and 912b800581ba6c21b0ed43832c6307f98e7a3af1 which move Transport and its prerequisites to `libbitcoin_common`. See https://github.com/Sjors/bitcoin/pull/47 for a more ambitious attempt to introduce a whole new `libbitcoin_net`, but became real big and distracting real fast :-)

🤔 vasild reviewed a pull request: "net: Replace libnatpmp with built-in PCP+NATPMP implementation"
(https://github.com/bitcoin/bitcoin/pull/30043#pullrequestreview-2141814858)
Approach ACK 23916f2a7717b463799207b4b81b5795f2e9d7e3

💬 vasild commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1655068312)
nit: maybe worth logging also `num_tries`, e.g. "retrying 1/5", "retrying 2/5", ...