💬 hebasto commented on issue "`WalletCreate{Encrypted/Plain}` benchmark crash on Windows":
(https://github.com/bitcoin/bitcoin/issues/29816#issuecomment-2114972539)
cc @furszy as an author of https://github.com/bitcoin/bitcoin/pull/28894.

💬 TheCharlatan commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1603176078)
Ok, I think I get the different mechanics involved here now and think if initially wiping an index is going to change, it should be done in a separate pull request. Will revert this change.

💬 glozow commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#issuecomment-2115000215)
> I'm not sure how to be confident that the UpdateBlockTipSync / hashRecentRejectsChainTip would be correct and didn't miss an edge case.

I (locally) split the commit into (1) update on validation interface callback and asserting that `hashRecentRejectsChainTip` is equal to the chain tip whenever we call `AlreadyHaveTx` + (2) removing `hashRecentRejectsChainTip`. I think if we see that everything runs fine with (1) it's probably correct.

I've (locally) hit a bug though, so will figure out
...

💬 TheCharlatan commented on pull request "kernel: De-globalize fReindex":
(https://github.com/bitcoin/bitcoin/pull/29817#issuecomment-2115009784)
Updated b47bd959207e82555f07e028cc2246943d32d4c3 -> b47bd959207e82555f07e028cc2246943d32d4c3 ([noGlobalReindex](https://github.com/TheCharlatan/bitcoin/tree/noGlobalReindex) -> [noGlobalReindex](https://github.com/TheCharlatan/bitcoin/tree/noGlobalReindex), [compare](https://github.com/TheCharlatan/bitcoin/compare/noGlobalReindex..noGlobalReindex))

* Reverted previous change in init code that would no longer wipe the indexes when restarting in the middle of a reindex without the `-reindex` fl
...

💬 dominicusadinfinitum commented on issue "Testsuite for Bitcoin Core 27.0.0 - FAIL: qt/test/test_bitcoin-qt":
(https://github.com/bitcoin/bitcoin/issues/30020#issuecomment-2115020459)
@hebasto
where or how do I get to the debug.log?

💬 TheCharlatan commented on pull request "rpc: avoid copying into UniValue":
(https://github.com/bitcoin/bitcoin/pull/30115#issuecomment-2115032923)
Concept ACK

Could this be a case for a clang-tidy plugin?

💬 kevkevinpal commented on pull request "test: Added test to ensure log and failure happen when work is less than active chainstate":
(https://github.com/bitcoin/bitcoin/pull/30105#discussion_r1603228739)
yes, we should be able to remove that TODO:

I used `n0` instead of `n1` because if we use `n1` it will properly work because `n1` is not synced up yet. So I used `n0` to `loadtxoutset` of a chainstate which is certain to be older than its current chainstate

💬 rkrux commented on pull request "test: Added test to ensure log and failure happen when work is less than active chainstate":
(https://github.com/bitcoin/bitcoin/pull/30105#discussion_r1603247602)
I see, though it seemed unusual initially that we are loading a snapshot in the same node. But now that I think more about it - this can happen in a real world scenario as well, right? Eg: When the node is re-started after a long time and needs to catch up to the latest blocks.

👍 rkrux approved a pull request: "test: Added test to ensure log and failure happen when work is less than active chainstate"
(https://github.com/bitcoin/bitcoin/pull/30105#pullrequestreview-2060598738)
tACK [ac1b0e3](https://github.com/bitcoin/bitcoin/pull/30105/commits/ac1b0e3ee9045bfa127b7a4bc291cacc98134a18)

There's a lint error though here, but otherwise LGTM.

📝 furszy opened a pull request: "bench: enable wallet creation benchmarks on all platforms"
(https://github.com/bitcoin/bitcoin/pull/30122)
Simple fix for #29816.

Instead of re-using a single directory to create a new wallet on each round, use a different one.
The benchmarking framework runs on top of the test framework, which cleans up the entire
directory after completion anyway.

💬 furszy commented on issue "`WalletCreate{Encrypted/Plain}` benchmark crash on Windows":
(https://github.com/bitcoin/bitcoin/issues/29816#issuecomment-2115180695)
@hebasto, #30122

💬 apoelstra commented on pull request "rpc: introduce getversion RPC":
(https://github.com/bitcoin/bitcoin/pull/30112#issuecomment-2115191801)
I'd be fine with that, though I'm not sure that there's a privacy loss to just throwing that extra info into the RPC.

At the very least, the RPC ought to indicate whether the wallet is *compiled* since that affects which RPC calls are available.

💬 brunoerg commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1603304486)
In "add bench for ShouldFanoutTo" 3f59bf83a41b7787f9c43c277b5e62f327a72c3e: `net.h` include seems unnecessary.

💬 fanquake commented on pull request "bench: enable wallet creation benchmarks on all platforms":
(https://github.com/bitcoin/bitcoin/pull/30122#issuecomment-2115216469)
The native Windows CI (https://github.com/bitcoin/bitcoin/actions/runs/9112695910/job/25052498545?pr=30122#step:24:64) is failing with:
```bash
Run src\bench_bitcoin.exe -sanity-check
Running with -sanity-check option, output is being suppressed as benchmark results will be useless.
Error: Process completed with exit code 1.
```

👍 TheCharlatan approved a pull request: "kernel: Streamline util library"
(https://github.com/bitcoin/bitcoin/pull/29015#pullrequestreview-2055052953)
Re-ACK b58156701ac132f87b8ef8da1c7d22158c804a81

Good improvements since my last review and having some form of external verification for the work being done is very nice. If other reviewers or maintainers are not happy with introducing a bash script, I could volunteer some time to translate it.

The includes could be cleaned up a bit, especially `util/string.h` and `util/strencodings.h` are missing in a bunch of places. It would also be nice if the newly introduced files had correct include
...

💬 TheCharlatan commented on pull request "kernel: Streamline util library":
(https://github.com/bitcoin/bitcoin/pull/29015#discussion_r1599843905)
I think rather than `test/` this should go into `contrib/devtools`, which contains functionally similar scripts like `symbol-check`, or `security-check`.

💬 hebasto commented on issue "`WalletCreate{Encrypted/Plain}` benchmark crash on Windows":
(https://github.com/bitcoin/bitcoin/issues/29816#issuecomment-2115302513)
My research shows that https://github.com/bitcoin/bitcoin/blob/dd42a5ddea6a72e1e9cad54f8352c76b0b701973/src/bench/wallet_create.cpp#L43-L44 does not trigger the `shared_ptr<CWallet>` deleter, which is `ReleaseWallet()`, because `wallet.use_count() == 3`.

💬 furszy commented on issue "`WalletCreate{Encrypted/Plain}` benchmark crash on Windows":
(https://github.com/bitcoin/bitcoin/issues/29816#issuecomment-2115320208)
> My research shows that
>
> https://github.com/bitcoin/bitcoin/blob/dd42a5ddea6a72e1e9cad54f8352c76b0b701973/src/bench/wallet_create.cpp#L43-L44
>
> does not trigger the `shared_ptr<CWallet>` deleter, which is `ReleaseWallet()`, because `wallet.use_count() == 3`.

Great 👍🏼 . #30122 releases the wallet too.

💬 ajtowns commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#issuecomment-2115335569)
> > I'm not sure how to be confident that the UpdateBlockTipSync / hashRecentRejectsChainTip would be correct and didn't miss an edge case.
>
> I (locally) split the commit into (1) update on validation interface callback and asserting that `hashRecentRejectsChainTip` is equal to the chain tip whenever we call `AlreadyHaveTx` + (2) removing `hashRecentRejectsChainTip`. I think if we see that everything runs fine with (1) it's probably correct.
>
> I've (locally) hit a bug though, so will f
...

👍 ryanofsky approved a pull request: "kernel: De-globalize fReindex"
(https://github.com/bitcoin/bitcoin/pull/29817#pullrequestreview-2060872465)
Code review ACK b47bd959207e82555f07e028cc2246943d32d4c3. I rereviewed the whole PR, but the only change since last review was reverting the bugfix https://github.com/bitcoin/bitcoin/pull/29817#discussion_r1578327024 and make the change a pure refactoring.

I think this change makes the code a lot less confusing by using separate names for the -reindex option and reindexing state, and could avoid bugs like the one mentioned in the future.