💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2423172535)
> I'm a little unclear what this swap is trying to do.
> Also I don't understand why m_mutex is locked twice in this function with a notify in between

The idea behind the swap was more about ensuring the queue is empty after joining the threads. Mainly to avoid any lingering future that would block callers indefinitely. That's why I used the "sanity" wording there.
And did it after joining the threads because we are currently waiting for all pending tasks to be completed before stopping the
...

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2423193226)
> I don't understand the need for the sanity cleanup. Shouldn't the logic from `WorkerThread` guarantee that `m_work_queue` is empty at this point, so that we could just assert that here?

Yeah, right now it is not really necessary. Have explained the context and the rationale here: https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2423172535

💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#discussion_r2423212929)
nice catch. Fixed.

💬 andrewtoth commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2423229107)
A unit test would check for regression on every test run.

💬 andrewtoth commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2423229704)
Does this PR depend on #32313?

💬 l0rinc commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2423233247)
The two are related, but I made sure we can merge either of them - but the other one will likely need to be rebase after that

💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#discussion_r2423281826)
I think that's what I also wrote in the commit message, but rephrased it slightly, let me know if it's better.

💬 l0rinc commented on pull request "coins: fix `cachedCoinsUsage` accounting in `CCoinsViewCache`":
(https://github.com/bitcoin/bitcoin/pull/32313#issuecomment-3393840503)
Thanks for the comments, the latest push makes it even simpler, removed the temporary asserts, added unit tests instead (thanks @w0xlt), changed the order of the `AddCoin` and `EmplaceCoinInternalDANGER` commits which did indeed allow me to fixup the latest commit with the fuzz simplifications into the `AddCoin` one.

> In the last commit I'm still not sure why expected_code_path assert is being removed

The role of the variable was to verify that code execution follows only expected paths,
...

💬 l0rinc commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#issuecomment-3393852130)
code review reACK 023cd5a5469ad61205bf7bb1135895f2b4a20ea9

Thanks for the quick responses.
Build failures seem systemic and unrelated.

💬 l0rinc commented on pull request "coins: use number of dirty cache entries in flush warnings/logs":
(https://github.com/bitcoin/bitcoin/pull/33512#discussion_r2423304917)
Here I'm only modifying them because the build was failing otherwise, but I have indeed added tests to https://github.com/bitcoin/bitcoin/pull/32313 which will ideally be merged before this. Either way the other one needs rebasing.

💬 l0rinc commented on pull request "refactor: throw `std::string_view` instead of `const char*` in constexpr/consteval functions":
(https://github.com/bitcoin/bitcoin/pull/33569#discussion_r2423309528)
Different files though, but yeah, will adjust if I need to push again, thanks.

💬 fanquake commented on pull request "[29.x] Backport logging ratelimiting":
(https://github.com/bitcoin/bitcoin/pull/33225#discussion_r2423424121)
@Crypt-iQ It's still fine to open a PR against 29.x to adjust this.

⚠️ fanquake reopened an issue: "ci: remove third-party javascript usage from Windows CI"
(https://github.com/bitcoin/bitcoin/issues/32508)
See:

https://github.com/bitcoin/bitcoin/blob/c779ee3a4044df3a263394bbb8b104aeeaa7c727/.github/workflows/ci.yml#L189-L190

We shouldn't need to use a third-party repo, that runs some Javascript, to configure a command prompt. The comment in our code also doesn't explain why this is necessary. We should be able to drop this dependency.

Also discussed in #32396.

💬 fanquake commented on issue "Release Schedule for 30.0":
(https://github.com/bitcoin/bitcoin/issues/32275#issuecomment-3394088606)
Binaries are available: https://bitcoincore.org/bin/bitcoin-core-30.0/
Website has been updated: https://bitcoincore.org/en/releases/30.0/

💬 maflcko commented on pull request "[29.x] Backport logging ratelimiting":
(https://github.com/bitcoin/bitcoin/pull/33225#discussion_r2423464895)
Could do it for master and backport?

💬 maflcko commented on issue "Intermittent CI network issue downloading assets.json from GitHub":
(https://github.com/bitcoin/bitcoin/issues/33599#issuecomment-3394123866)
I guess the IPs are detected as and blocked as "LLM scrapers". Possible workarounds could be:

* Use a mirror/proxy to download the json file
* Use a git clone (sparse), which may not be rate limited

📝 prusnak opened a pull request: "contrib: add desktop file"
(https://github.com/bitcoin-core/gui/pull/902)
from https://github.com/bitcoin-core/packaging

=> https://github.com/bitcoin-core/packaging/blob/main/debian/bitcoin-qt.desktop

this simplifies packaging, because currently one needs to fetch a desktop file from another repo instead of using just the main source code one

💬 sipa commented on pull request "txgraph: randomize order of same-feerate distinct-cluster transactions":
(https://github.com/bitcoin/bitcoin/pull/33335#issuecomment-3394366181)
Rebased on #33157, as it looks that's close.

👍 TheCharlatan approved a pull request: "miner: empty mempool special case for waitNext()"
(https://github.com/bitcoin/bitcoin/pull/33566#pullrequestreview-3328593103)
ACK 2e8fff3f17366e9c2ba054023ad8bd89ac51d584

👍 TheCharlatan approved a pull request: "[IBD] coins: increase default UTXO flush batch size to 32 MiB"
(https://github.com/bitcoin/bitcoin/pull/31645#pullrequestreview-3328689546)
ACK b6f8c48946cbfceb066de660c485ae1bd2c27cc1

I don't think complicating this with a dynamic calculation makes sense either given the trade offs documented in this discussion.