💬 hodlinator commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1658606208)
`LogPrintf` is deprecated since f7ce5ac08c669ac763e275bb7c82dcfb2b1b6c33 / August 2023. Should use `LogInfo`.

💬 hodlinator commented on pull request "Testnet4 including PoW difficulty adjustment fix":
(https://github.com/bitcoin/bitcoin/pull/29775#discussion_r1658532351)
*node 2

💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1658781284)
I believe the idea behind these nonces is to introduce diversity so that everyone has a different cache. That way there's no possibility of poisoning the "global" cache.

👍 stickies-v approved a pull request: "kernel: remove mempool_persist"
(https://github.com/bitcoin/bitcoin/pull/30344#pullrequestreview-2148087355)
ACK f1478c05458562a9bef5c2ba43959d758e7b4745

I think kernel should have an interface that (indirectly) allows users to dump/load mempool to/from disk (or elsewhere), but this `mempool_perist` implementation seems too node opinionated to be included in kernel.

💬 darosior commented on pull request "Wallet: don't underestimate the fees when spending a Taproot output":
(https://github.com/bitcoin/bitcoin/pull/26573#issuecomment-2196984111)
CI is erroring on
```
2024-06-28T11:41:00.1099453Z 5/309 - [1mwallet_fundrawtransaction.py --descriptors [0m failed, Duration: 30 s
2024-06-28T11:41:00.1100206Z
2024-06-28T11:41:00.1100438Z [1mstdout:
2024-06-28T11:41:00.1101244Z [0m2024-06-28T11:40:29.220000Z TestFramework (INFO): PRNG seed is: 3960890322720202555
2024-06-28T11:41:00.1108001Z 2024-06-28T11:40:29.221000Z TestFramework (INFO): Initializing test directory /home/runner/work/_temp/ci/scratch/test_runner/test_runner_₿_🏃_20
...

💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1658791383)
Nit: Commit message still says CSignatureCache :)

💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1658797600)
Nit: I don't see the harm in keeping it movable even if we don't currently need to move it. As a reader of the code, if I needed to move it in the future, I'd be afraid to remove these because "they're probably there for a reason".

Unless there actually is a reason?

💬 theuni commented on pull request "kernel: De-globalize validation caches":
(https://github.com/bitcoin/bitcoin/pull/30141#discussion_r1658806068)
Same comment here about deleted moves.

💬 brunoerg commented on pull request "fuzz: wallet: add target for `CreateTransaction`":
(https://github.com/bitcoin/bitcoin/pull/29936#issuecomment-2197038945)
> It is not being funded, I will add it. Thanks.

Force-pushed adding it.

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