💬 sipa commented on pull request "validation: do not wipe utxo cache for stats/scans/snapshots":
(https://github.com/bitcoin/bitcoin/pull/30610#issuecomment-2284893423)
@paplorinc Yes, it is an optimization, but only affects the `scantxoutset` and `gettxoutsetinfo` RPCs. Normal synchronization should be unaffected.

💬 mzumsande commented on issue "Intermittent failures in interface_usdt_mempool.py":
(https://github.com/bitcoin/bitcoin/issues/27380#issuecomment-2284894942)
Maybe copying the string into a static or global variable would help rule out a lifetime issue?

```diff
diff --git a/src/validation.cpp b/src/validation.cpp
index 9126d8e928..683d13e0e5 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -1863,6 +1863,8 @@ MempoolAcceptResult AcceptToMemoryPool(Chainstate& active_chainstate, const CTra
CTxMemPool& pool{*active_chainstate.GetMempool()};

std::vector<COutPoint> coins_to_uncache;
+ static char string_buffer[120];

...

📝 theStack opened a pull request: "doc: add missing "testnet4" network string in RPC/init help texts"
(https://github.com/bitcoin/bitcoin/pull/30642)
The following bitcoind parameters / RPC calls still missed the "testnet4" network string:
- `-chain=` parameter
- `getblockchaininfo` RPC, `"chain"` result
- `getmininginfo` RPC, `"chain"` result

The occurences were found via `$ git grep \".*main.*test.*\"`.

📝 paplorinc opened a pull request: "coins: Add move operations to CCoinsCacheEntry"
(https://github.com/bitcoin/bitcoin/pull/30643)
Related to https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1703187118

Added defaulted move constructor and move assignment operator to `CCoinsCacheEntry`.
And to make sure the mentioned Sonar warnings aren't triggered by CI anymore, I've modernized the call-site minimally.

💬 paplorinc commented on pull request "DO NOT MERGE: CCoinsCacheEntry move constructor Sonar fix":
(https://github.com/bitcoin/bitcoin/pull/30641#issuecomment-2284920490)
I'm a bit surprised at these reactions, but opened https://github.com/bitcoin/bitcoin/pull/30643 with the proposed fix.

💬 furszy commented on pull request "wallet: fix blank legacy detection":
(https://github.com/bitcoin/bitcoin/pull/30621#discussion_r1714380473)
done

💬 furszy commented on pull request "wallet: fix blank legacy detection":
(https://github.com/bitcoin/bitcoin/pull/30621#discussion_r1714380532)
done

💬 andrewtoth commented on pull request "coins: Add move operations to CCoinsCacheEntry":
(https://github.com/bitcoin/bitcoin/pull/30643#issuecomment-2284929509)
Don't we also need to add move constructor to `Coin`? Its prevector is the only thing that really benefits from being moved I believe.

🤔 stickies-v reviewed a pull request: "refactor: Remove Span operator==, Use std::ranges::equal"
(https://github.com/bitcoin/bitcoin/pull/29071#pullrequestreview-2232822586)
Approach ACK

💬 stickies-v commented on pull request "refactor: Remove Span operator==, Use std::ranges::equal":
(https://github.com/bitcoin/bitcoin/pull/29071#discussion_r1713639323)
nit: no more need for `#include <algorithm>`

<details>
<summary>iwyu</summary>

```
span.h should add these lines:
#include <utility> // for declval, forward

span.h should remove these lines:
- #include <algorithm> // lines 8-8
- #include <cassert> // lines 9-9

The full include-list for span.h:
#include <cstddef> // for size_t, byte, nullptr_t
#include <span> // for span
#include <type_traits> // for remove_pointer, is_convertible, enable_if, enable_if_t,
...

👍 AngusP approved a pull request: "test: update satoshi_round function"
(https://github.com/bitcoin/bitcoin/pull/29566#pullrequestreview-2234072206)
ACK ec317bc44b0cb089419d809b5fef38ecb9423051

🤔 pablomartin4btc reviewed a pull request: "build: Introduce CMake-based build system"
(https://github.com/bitcoin/bitcoin/pull/30454#pullrequestreview-2234121687)
tACK 67b1e236334f38ec4e4d2251dbdfb790f20ed88b

Built with `GUI` on macOS 14.4 following the instructions in `/doc/build-osx.md` (part of this PR) and run `./build/qt/bitcoin-qt` successfully.

A few observations:

- Regarding the configuration: in the documentation says:
"_If `berkeley-db@4` is installed, then legacy wallet support will be built_."
But I have to use `-DWITH_BDB=ON` in order to get that functionality.
- After I compile with `cmake` I get this warning:
`ld: warning
...

🤔 tdb3 reviewed a pull request: "wallet: fix blank legacy detection"
(https://github.com/bitcoin/bitcoin/pull/30621#pullrequestreview-2234159075)
Approach ACK

Nice simplifications.

Retested (https://github.com/bitcoin/bitcoin/pull/30621#pullrequestreview-2231909317) with same result (migration allowed).

Saw some `may be used uninitialized` warnings when building, e.g.
```
inlined from ‘bool wallet::CWallet::IsSpentKey(const CScript&) const’ at wallet/wallet.cpp:1046:37:
./prevector.h:175:37: warning: ‘*(const prevector<28, unsigned char, unsigned int, int>*)((char*)&<unnamed> + offsetof(const std::CTxDestination, std::varian
...

👍 ryanofsky approved a pull request: "logging: use bitset for categories"
(https://github.com/bitcoin/bitcoin/pull/26697#pullrequestreview-2234186346)
Code review ACK d8c637a0ad704c30dc128d0c66215d5ac220f85d

I wouldn't have a problem just switching the code to use uint64_t but I think this approach is a nicer and more type safe. I like that this gets rid of all the bitwise operations and (1 << 28) cruft and stops hardcoding uint32_t various places. The atomic bitset class seems pretty simple to me and has a nice interface.

💬 ryanofsky commented on pull request "logging: use bitset for categories":
(https://github.com/bitcoin/bitcoin/pull/26697#discussion_r1714473985)
This is a good catch and I think it's worth commenting on, but I don't think it is a bug. Would suggest adding a comment like: "Checks if any bits are set. Note: Bits are checked independently, so result reflects the state of all bits, but not at a single instant in time."

💬 ryanofsky commented on pull request "logging: use bitset for categories":
(https://github.com/bitcoin/bitcoin/pull/26697#discussion_r1714475980)
In commit "logging: use bitset class for categories" (d8c637a0ad704c30dc128d0c66215d5ac220f85d)

Would be nice to rename `bits` member to `m_bits` for clarity I think

👍 ryanofsky approved a pull request: "fix: increase consistency of rpcauth parsing"
(https://github.com/bitcoin/bitcoin/pull/30401#pullrequestreview-2234212279)
Code review ACK f4b0b69a3e7df58430cb887028b13265834d8c7e.

This PR is now just a one line fix accompanied by various tests.

💬 ryanofsky commented on pull request "fix: increase consistency of rpcauth parsing":
(https://github.com/bitcoin/bitcoin/pull/30401#discussion_r1714491084)
In commit "fix: increase consistency of rpcauth parsing" (f4b0b69a3e7df58430cb887028b13265834d8c7e)

I think it would be nice if commit message said explicitly that previous behavior was to sometimes ignore empty `-rpcauth=` settings, and other times treat them as errors, and now they are consistently treated as errors.

Could also mention the details, but not important

> Previous behavior was nonsensical:
>
> - If an empty -rpcauth="" argument was specified as the last command line ar
...

💬 LarryRuane commented on pull request "logging: use bitset for categories":
(https://github.com/bitcoin/bitcoin/pull/26697#issuecomment-2285108664)
@ryanofsky - I took your suggestions (`bits` to `m_bits` and added the comment), and I noticed something else: By defining `LogCategoryMask` as comprising `uint64_t` instead of `uint32_t` base elements, the [non-atomic timing window problem](https://github.com/bitcoin/bitcoin/pull/26697#discussion_r1713192718) won't happen until we exceed 64 logging categories, because the loop in `any()` will iterate only once. That loop doesn't look at individual bits, it looks at one entire base element (now
...

💬 tdb3 commented on pull request "fix: increase consistency of rpcauth parsing":
(https://github.com/bitcoin/bitcoin/pull/30401#discussion_r1714507567)
Thanks. Amended the commit message to mention that the previous rpcauth behavior was inconsistent. Updated the PR description to list detailed behavior.

🤔 tdb3 reviewed a pull request: "test: assumeutxo: check that UTXO-querying RPCs operate on snapshot chainstate"
(https://github.com/bitcoin/bitcoin/pull/30636#pullrequestreview-2234279782)
Concept ACK

Great test additions.
Light code review and manual test. I concur with the nits from @fjahr.