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

✅ andrewtoth closed a pull request: "validation: periodically flush dbcache during reindex-chainstate"
(https://github.com/bitcoin/bitcoin/pull/32414)

💬 andrewtoth commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#issuecomment-2848863531)
> is this comment still accurate? https://github.com/bitcoin/bitcoin/blob/master/src/validation.cpp#L2782 Guess it just refers to the size based trigger, not the time based - which is probably fine given its small context.

I think it's still accurate, and #30611 doesn't make it stale. It could possibly be updated to

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

💬 andrewtoth commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#discussion_r2072487540)
Actually we can't do that because the lock can't be held for `m_chainman.snapshot_download_completed();` :/.

💬 jamesob commented on pull request "scripted-diff: adapt script error constant names in feature_taproot.py":
(https://github.com/bitcoin/bitcoin/pull/32415#issuecomment-2848902968)
crACK https://github.com/bitcoin/bitcoin/pull/32415/commits/b5f580c580257d28d295cae3f787b55eb1863f16

Scripted-diff/func-test-only makes this an easy review. Good cleanup for this daunting file.

🤔 shahsb reviewed a pull request: "build: replace header checks with `__has_include`"
(https://github.com/bitcoin/bitcoin/pull/32405#pullrequestreview-2813356567)
Concept ACK.

This improves code readability and simplicity. This is indeed a nice feature from portability and backward compatibility perspective. Thanks for making the changes!!

💬 shahsb commented on pull request "build: replace header checks with `__has_include`":
(https://github.com/bitcoin/bitcoin/pull/32405#issuecomment-2848993111)
ACK https://github.com/bitcoin/bitcoin/pull/32405/commits/e1f543823b300b28c9edaf5d1a3e1e9badde471b

💬 l0rinc commented on pull request "validation: periodically flush dbcache during reindex-chainstate":
(https://github.com/bitcoin/bitcoin/pull/32414#issuecomment-2849074101)
You didn't mean to close it, right?

------

I can confirm that this change retains the flushing behavior during IBD as well as far as I can tell from the plots:


---


I'll compare the `reindex-chainstate` performance next, before & after, let's see if there's a regression (we do ex
...

💬 rebroad commented on pull request "feefrac: add support for evaluating at given size":
(https://github.com/bitcoin/bitcoin/pull/30535#discussion_r2072559328)
snake_case?

💬 TheCharlatan commented on pull request "kernel: Flush in ChainstateManager destructor":
(https://github.com/bitcoin/bitcoin/pull/31382#discussion_r2072583637)
Thanks for taking a look :)

I think the problem here is that we are moving the mempool pointer around between different objects, while they don't really share ownership of it at any one time. It not having a value at some points in time does have a logical meaning in the current code. In general I don't like shared pointers for a number of reasons. In this instance I agree that it would solve the problem of not knowing when to destruct the mempool. I am a bit apprehensive to commit this chang
...

👍 theStack approved a pull request: "crypto: Use secure_allocator for `AES256_ctx`"
(https://github.com/bitcoin/bitcoin/pull/31774#pullrequestreview-2813438811)
Code-review ACK 3d7d2c37f7fe10e77d50c8b8fa4d6c74ad52a3c6