💬 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
(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.
(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.
(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?
(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
(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.
(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,
...
(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.
(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.
(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.
(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.
(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.
(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/
(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?
(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
(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
(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.
(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
(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.
(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.
🤔 hebasto reviewed a pull request: "ci: Use native platform for win-cross task"
(https://github.com/bitcoin/bitcoin/pull/33558#pullrequestreview-3328936219)
> * Unlock the CI task to run on riscv64 at all
Isn't this relevant to other tasks as well, for example, `ci/test/00_setup_env_arm.sh`?
(https://github.com/bitcoin/bitcoin/pull/33558#pullrequestreview-3328936219)
> * Unlock the CI task to run on riscv64 at all
Isn't this relevant to other tasks as well, for example, `ci/test/00_setup_env_arm.sh`?