💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2606802006)
> it looks like this only happens with powershell, not with the classic windows shell

In that case I wouldn't worry about it.

I'll retest.

💬 willcl-ark commented on issue "Enable PCP by default?":
(https://github.com/bitcoin/bitcoin/issues/31663#issuecomment-2606831902)
This does not seem to fully work for me on a [Unifi Cloud Gateway Ultra](https://techspecs.ui.com/unifi/unifi-cloud-gateways/ucg-ultra?subcategory=all-unifi-cloud-gateways). It does appear to get an ipv4 mapping, but then fails whilst trying to get an ipv6 version:

```log
2025-01-22T09:42:37Z [net] portmap: gateway [IPv4]: 10.0.0.1
2025-01-22T09:42:37Z [net] pcp: Requesting port mapping for addr 0.0.0.0 port 8444 from gateway 10.0.0.1
2025-01-22T09:42:37Z [net] pcp: Internal address after conne
...

💬 fanquake commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2606846188)
> The redundancy was already there with getdifficulty. That said, I don't personally need gettarget so I could drop it if people prefer.

Some redundancy already existing isn't a reason to make things worse? We don't usually add new RPCs for fun.

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2606863459)
I'll drop the `gettarget` RPC by modifying 98e8c8fafc96e856bb95bb6e68e54d054e73128d to only introduce the `GetTarget` helper.

📝 fanquake converted_to_draft a pull request: "[RFC] Align debugging flags to `-O0`"
(https://github.com/bitcoin/bitcoin/pull/29796)
A depends build with `DEBUG=1` and using `cmake -B build -DCMAKE_BUILD_TYPE=Debug` do not align, which is inconsistent/confusing. The depends build will use `-O1`, while `CMAKE_BUILD_TYPE=Debug` will set `-O0`.

I'm not completely sure yet what the right choice is (previously it seems that `-Og` was unusable with Clang? (note that for Clang it looks like `-Og` and `-O1` are basically equivalent), but we should at least align the two systems to be using the same thing for debugging.

Configur
...

🤔 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.