🤔 hodlinator reviewed a pull request: "util: explicitly close all AutoFiles that have been written"
(https://github.com/bitcoin/bitcoin/pull/29307#pullrequestreview-2288411339)
Concept ACK

Appreciate the intent of being rigorous in reporting issues with flushing file content to disk.

#### Alternative implementation

The current PR which produces a *run-time failure* in the `AutoFile` destructor upon forgetting to explicitly call `fclose()`. I attempted an alternative implementation ([branch](https://github.com/bitcoin/bitcoin/compare/master...hodlinator:bitcoin:pr/29307_suggestions_prime)) [inspired by ryanofsky's comment](https://github.com/bitcoin/bitcoin/pul
...

💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1748968105)
Less error-prone to use the error value returned from `fclose()` rather than relying on global `errno` which might have been mutated by `remove()` etc.
```suggestion
if (int err{fileout.fclose()}; err != 0) {
remove(pathTmp);
LogError("Failed to close file %s: %s", fs::PathToString(pathTmp), SysErrorString(err));
```
(Also dropped pre-`-logsourcelocations` `__func__` and removed superfluous newline).

💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1748973642)
Rather than silence errors, it might be good to at least assert against errors here and elsewhere in benchmarks/tests as we want to ensure we are benchmarking/testing properly? (Not for some fuzz-cases though [as you already found out](https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1465997866)).
```suggestion
assert(file.fclose() != 0);
```
That's already how you handled *src/wallet/test/fuzz/wallet_bdb_parser.cpp*.

💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1748972711)
Could use `SystemErr(err)` here?
```suggestion
if (int err{fileout.fclose()}; err != 0) {
LogPrintf("%s: Failed to close filter file %d: %s\n", __func__, pos.nFile, SysErrorString(err));
```
Could be done in other files too (*net.cpp*, *blockstorage.cpp*). (I can understand not wanting to do it further above in this specific file though).

💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1924324131)
Should also explicitly `fclose()` `afile` from line 2790/2791.

https://github.com/bitcoin/bitcoin/blob/dba783538683cfc6af209c640c2d019648493f31/src/rpc/blockchain.cpp#L2790-L2791

💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1924262048)
nit: Might take the opportunity to improve grammar and modernize log:
```suggestion
LogWarning("Failed to write fee estimates to %s. Continuing anyway.", fs::PathToString(m_estimation_filepath));
```

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