💬 hodlinator commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2072389405)
In that case it would be better to call it `bool AutoFile::Close()` rather than `AutoFile::fclose`.

💬 rkrux commented on pull request "psbt: MuSig2 Fields":
(https://github.com/bitcoin/bitcoin/pull/31247#discussion_r2072389863)
I did this in #32412.

📝 andrewtoth opened a pull request: "validation: periodically flush dbcache during reindex-chainstate"
(https://github.com/bitcoin/bitcoin/pull/32414)
After #30611 we periodically do a non-erasing flush of the dbcache to disk roughly every hour during IBD.
The intention was to also do this periodic flush during reindex-chainstate, so we would not risk losing progress during a system failure when reindexing with a high dbcache value.

It was discovered that reindex-chainstate does not perform a PERIODIC flush until it has already reached the tip. Since reindexing to tip usually happens within 24 hours, this behaviour was unnoticed with the p
...

💬 andrewtoth commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#issuecomment-2848617247)
Thanks to @l0rinc for discovering this behaviour, and @mzumsande for the suggested fix.

💬 l0rinc commented on pull request "util: explicitly close all AutoFiles that have been written":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r2072395486)
Agree, it's what I wrote in the review as well:
> the current `fclose` returns a fake error code, but is actually a boolean - may make sense to rename it (e.g. `Close`, similarly to how we've wrapped `ftruncate` with `TruncateFile`) and migrate to returning a boolean to avoid the current confusion.

👋 l0rinc's pull request is ready for review: "log: print reason when writing chainstate"
(https://github.com/bitcoin/bitcoin/pull/32404)

💬 l0rinc commented on pull request "log: print reason when writing chainstate":
(https://github.com/bitcoin/bitcoin/pull/32404#issuecomment-2848685153)
The new logs can help visualize the flush times and reasons during IBD now - reindex-chainstate periodic flushes require https://github.com/bitcoin/bitcoin/pull/32414


🤔 l0rinc requested changes to a pull request: "validation: periodically flush dbcache during reindex-chainstate"
(https://github.com/bitcoin/bitcoin/pull/32414#pullrequestreview-2813262035)
I'm verifying is this fixes things or not, whether it slows down anything - or if it changes caching behavior in any other way.

Regardless, I think we should cover this with a test - do you think we could extend e.g. https://github.com/bitcoin/bitcoin/blob/master/test/functional/feature_reindex.py with an `assert_debug_log` checking the periodic write logs of https://github.com/bitcoin/bitcoin/pull/32404?

💬 l0rinc commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#issuecomment-2848739902)
I can confirm that this fixes the periodic flushes for `-reindex-chainstate`:

<details>
<summary>Details</summary>

```
cat debug-7fc8d7f9c1f0fe3795b397327e38465ee6f76b83-1746277780.log | grep FlushStateToDisk
2025-05-03T14:14:56Z [coindb] FlushStateToDisk write: flush mode=PERIODIC, prune=0, cache_large=0, cache_critical=0, periodic=1
2025-05-03T15:16:31Z [coindb] FlushStateToDisk write: flush mode=PERIODIC, prune=0, cache_large=0, cache_critical=0, periodic=1
2025-05-03T16:16:31Z [co
...

📝 theStack opened a pull request: "scripted-diff: adapt script error constant names in feature_taproot.py"
(https://github.com/bitcoin/bitcoin/pull/32415)
While reviewing #31622 I noticed that the constant name `(SCRIPT_)ERR_SIG_HASHTYPE` is used for two different script verification error codes, namely one for legacy and one for Schnorr signatures:

https://github.com/bitcoin/bitcoin/blob/eba5f9c4b63fe46261fbb3e71b9a94832d105b23/src/script/script_error.cpp#L56-L57
https://github.com/bitcoin/bitcoin/blob/eba5f9c4b63fe46261fbb3e71b9a94832d105b23/test/functional/feature_taproot.py#L600

In order to resolve this confusion, this PR adapts all scr
...

💬 jonatack commented on pull request "net: improve the interface around FindNode() and avoid a recursive mutex lock":
(https://github.com/bitcoin/bitcoin/pull/32326#discussion_r2072443916)
I prefer the original function naming `AlreadyConnectedToAddress` and it looks like retaining it would also reduce the diff (`IsConnectedToAddrPort` could be `AlreadyConnectedToAddressPort` instead).

⚠️ Ari4ka opened an issue: "https://cryptotab.farm/download/farm_miner_th3osQ"
(https://github.com/bitcoin/bitcoin/issues/32416)
You created a miner on the CryptoTab Farm.
To download the Miner visit: https://cryptotab.farm/download/farm_miner_UOdDsM
Or open the download page: https://cryptotab.farm/download and enter your Miner ID: UOdDsM
For more information: https://cryptotab.farm

✅ willcl-ark closed an issue: "https://cryptotab.farm/download/farm_miner_th3osQ"
(https://github.com/bitcoin/bitcoin/issues/32416)

💬 rebroad commented on pull request "Enhanced Traffic Graph Widget with Multi-timeframe Support and Data Persistence":
(https://github.com/bitcoin-core/gui/pull/866#issuecomment-2848779371)
Have just added some more GUI improvements - the text on the graph now has a black outline around it making it easier to read (and is printed AFTER the graph rather than before) .Also, the grey line at the bottom of the graph is now printed after the graph also, causing the graph not to spill onto that line.

Also, the bright green and red lines no longer surround the sides of the graph but only draw an outline along actual graph values. This makes it easier to distinguish about the graph goin
...

💬 TheCharlatan commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072462850)
Previously we didn't flush before calling `CheckBlockIndex`, but I don't think that would degrade the usefulness of the check. I am not sure though about taking the lock here again. If I read this right the lock is taken three times now on every iteration of the loop with little work being done in between. Might it be useful to consolidate these?

💬 sipa commented on pull request "scripted-diff: adapt script error constant names in feature_taproot.py":
(https://github.com/bitcoin/bitcoin/pull/32415#issuecomment-2848808851)
Concept ACK

👍 theStack approved a pull request: "subprocess: Backport upstream changes"
(https://github.com/bitcoin/bitcoin/pull/32358#pullrequestreview-2813317800)
Light ACK cd95c9d6a7ec08cca0f9c98328c759be586720f8

Thanks for following up! Can't comment much on the concrete code changes as I'm not too familiar with Windows development, but verified that all the upstream PRs are now included and were applied correctly (as a note for other reviewers, sometimes the diff is smaller here, as some commits touches code for areas that we have removed).

💬 andrewtoth commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072482783)
> Previously we didn't flush before calling `CheckBlockIndex`

We do flush for every block via `ActivateBestChain` -> `ActivateBestChainStep` -> `ConnectTip` -> `FlushStateToDisk`. However, it is only flushed `IF_NEEDED`, and here we do it `PERIODIC`.

💬 andrewtoth commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072484132)
> If I read this right the lock is taken three times now on every iteration of the loop with little work being done in between. Might it be useful to consolidate these?

Not a bad idea. How does d2163bd4ccf133bd52f1585e8ef53037afd74df9 look?

🤔 furszy reviewed a pull request: "qt, wallet: Convert uint256 to Txid"
(https://github.com/bitcoin/bitcoin/pull/32238#pullrequestreview-2813321727)
Would be good to register this new type into the Qt's meta-object system. Look at the `RegisterMetaTypes()` function on `src/qt/bitcoin.cpp`.
If we don't do it, the framework will (should) throw an error "not registered object type" when this new type is used as an argument in a signal or slot, or wrapped into a `QVariant` response (if I recall correctly, this was due to framework's internal dynamic casting).