💬 Sjors commented on pull request "psbt: add non-default sighash types to PSBTs and unify sighash type match checking":
(https://github.com/bitcoin/bitcoin/pull/31622#discussion_r1924993691)
In other words: for all functions that take a sighash argument, you either pass the correct sighash or nothing.

💬 l0rinc commented on pull request "optimization: `CheckBlock` input duplicate detection":
(https://github.com/bitcoin/bitcoin/pull/31682#discussion_r1925006647)
Fixed, thanks

💬 l0rinc commented on pull request "optimization: `CheckBlock` input duplicate detection":
(https://github.com/bitcoin/bitcoin/pull/31682#issuecomment-2606765689)
I pushed a new version that is based on https://github.com/bitcoin/bitcoin/pull/31699 - to separate the testing/benching from the consensus change. This will remain in draft to gather comments.

----

I've also retriggered an IBD (simulated via multiple ` -reindex-chainstate` runs for stability).
Here I've compared the performance of this PR after applying my other IBD optimizations (https://github.com/bitcoin/bitcoin/pull/31144 and https://github.com/bitcoin/bitcoin/pull/31645) to measure
...

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1925023954)
Ok, I suppose we could drop the code that does that eventually, but not here.

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1925034229)
> Miniscript is a language for writing (a subset of) Bitcoin Scripts

https://bitcoin.sipa.be/miniscript/

So when I read "containing miniscript" I interpret that as containing a descriptor using the miniscript language. But these are legacy wallets, which don't have descriptors, so they can only contain script itself.

💬 Sjors commented on pull request "wallet: Utilize IsMine() and CanProvide() in migration to cover edge cases":
(https://github.com/bitcoin/bitcoin/pull/31495#discussion_r1925037538)
That's not going to obvious for most people looking at this in the future, so I think it's worth a comment (a test might be overkill).

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