💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1925147841)
Do you mean checking that the difficulty is 1 after the genesis block? That seems overkill, since we already check the last block of the first retarget period (2015).

✅ fanquake closed an issue: "fuzz: `scriptpubkeyman`: heap-buffer-overflow miniscript.cpp in CScript BuildScript"
(https://github.com/bitcoin/bitcoin/issues/30864)

🚀 fanquake merged a pull request: "descriptor: Add proper Clone function to miniscript::Node"
(https://github.com/bitcoin/bitcoin/pull/30866)

🤔 theStack reviewed a pull request: "wallet: remove BDB dependency from wallet migration benchmark"
(https://github.com/bitcoin/bitcoin/pull/31241#pullrequestreview-2566950442)
Concept ACK

💬 theStack commented on pull request "wallet: remove BDB dependency from wallet migration benchmark":
(https://github.com/bitcoin/bitcoin/pull/31241#discussion_r1925166647)
Why is this reset not needed anymore?

💬 Sjors commented on pull request "rpc: add target to getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2607000049)
Along similar lines I also dropped the change that added `next` to `getdifficulty`; miners can use `getmininginfo`. For that I moved the `NextEmptyBlockIndex` helper from f408fdef874b6aa5d9fcb6968a13e5ebdf57b01a to 7921c0b1e82427b8129b21931cb83f45222820e4 -> cf0a62878be214cd4ec779aab214221b27b769b6.

We can probably deprecate `getdifficulty` since it seems completely useless, but I won't do that here.

💬 maflcko commented on issue "Fuzzer enhancement: Explicitly check output for uninitialized memory":
(https://github.com/bitcoin/bitcoin/issues/22064#issuecomment-2607120689)
> But starting with clang 16, at least MSan gets us closer to this. Returning an uninitialized variables from a function, or passing uninitialized values to a function as a parameter is now considered a "use" of uninitialized memory, and MSan will report it by default.

From a quick test, it looks like the check may be skipped when the optimization is greater than `0`. Playground: https://godbolt.org/z/bbsGcfvsa

So I think running msan now with `-O0` should be good enough? I can't really imagin
...

🤔 marcofleon reviewed a pull request: "multi-peer orphan resolution followups"
(https://github.com/bitcoin/bitcoin/pull/31666#pullrequestreview-2567077216)
Nice improvements in `txdownloadman_impl`, definitely easier to read.

code review ACK 47f269b3375288c53a02bdb894de83cfa39c81c4

💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31668#discussion_r1925324713)
Hi @maflcko something like`{"timestamp | \"now\"|\"never\"", "integer / string"}` here never means no scanning. Please suggest your expectation.

🤔 l0rinc requested changes to a pull request: "Use number of dirty cache entries in flush warnings/logs"
(https://github.com/bitcoin/bitcoin/pull/31703#pullrequestreview-2566967750)
Concept ACK

I have tried reproducing it via AssumeUTXO but the warning is never triggered.
That's my main concern (it's probably my mistake in the related PR), other than not applying it in other cases where the sizes are displayed.

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1925176427)
This warning became more useful now - a few questions:
* Would it make sense to mention the coin count and do the warning based on that instead? May be too much information, not sure, it's just an idea:
```C++
“Flushing %s dirty coins to disk...”
```
* If we keep GiB (while `double` is likely still precise enough for the billions range), I wonder if the users are really interested in fraction of GiB here
* The limit for triggering this has changed here - does the `WARN_FLUSH_COINS_SIZE` th
...

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1925193678)
We're already in a `Cache` object, maybe we could clarify that this is a running count:
```suggestion
/* Running count of dirty Coin entries. */
```

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1925195638)
I understand that the hard-coded value was here before, and was wondering if we could explain how we got to this magic number (and not mention the value in comments at all since code can express that better), maybe something like:
```C++
static constexpr size_t COIN_DISK_USAGE_BYTES{sizeof(Coin)}; // Approximate size on disk
```

----

Loading the UTXO set to memory:
> 2025-01-22T11:56:22Z [snapshot] loaded 176948713 (27440 MB) coins from snapshot 0000000000000000000320283a032748cef82278
...

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1925224383)
I don't mind either the conditional increment or the implicit boolean-to-int conversion, but I get distracted when both are used without an easy to understand reason.
Unless there's a good reason here, could we change this to:
```suggestion
m_dirty -= current.second.IsDirty();
```

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1925215393)
Super-nit: `m_num_dirty` increment is related to the `SetDirty` call, not the `Clear` call, if you think it makes sense, we could group them as such (like we do in `EmplaceCoinInternalDANGER` and `BatchWrite`)

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1925198387)
given that this is mainly used for logging, do we want to crash the client when this isn't accurate?

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1925202151)
Nit 1: `num` doesn't obviate what the number represents, just its type. It could mean bytes or count or an int representation of a boolean. Given that in other cases we called this `DirtyCount`, could we use that here as well?
Nit 2: not sure it matters, but in other cases here we didn't add the "Get" prefix, I don't see the value it adds (and it kinda' gets confusing when we're mutating it in `SingleEntryCacheTest`), might as well trim it I guess.
```suggestion
size_t& DirtyCount() const
...

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1925248327)
Slightly unrelated, but we probably need something similar for memory as well.
Currently, there's no explicit warning when `-dbcache` is set to a value exceeding available system memory (especially problematic after https://github.com/bitcoin/bitcoin/pull/28358).
A similar warning in this scenario would be very helpful - but likely outside the scope of this PR.

💬 l0rinc commented on pull request "Use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/31703#discussion_r1925236472)
Nit: it's test code so it's not super-important, but we're basically returning the state of a modified parameter here - seems a bit hacky to me.
I think we could return the same value as before and call `IsDirty()` on the call site, e.g.
```C++
size_t usage{cache_coin ? InsertCoinsMapEntry(map, sentinel, *cache_coin) : 0};
size_t dirty{cache_coin->IsDirty()};
```
and
```C++
cache.usage() += InsertCoinsMapEntry(cache.map(), cache.sentinel(), *cache_coin);
cache.GetNumDirty() += cache_coi
...

💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2607354185)
tACK 0b503d792f0b6255f6dc5b0e4ee7ce2b7b667e90

Also tested on macOS.

The logic in `ExecCommand()` easier to follow compared to my previous review.

I did not study the Windows `ExecVp` implementation in f006565ad220154c6e402c99074d326699d62d3e. I don't expect anyone to use it until Windows multiprocess works (and is in a release). It would be good to call the wrapper from the Windows CI.

💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#discussion_r1925331073)
a1d960e03be53517e80d36c257941ff407a6f61f: unrelated nit: `struct` is unused