💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1925038708)
But then the script just disappears, shouldn't we warn about that or even abort?

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