💬 fanquake commented on pull request "guix: Use LTO to build releases":
(https://github.com/bitcoin/bitcoin/pull/25391#issuecomment-2278201819)
> Would be nice to extend the motivation (pull request description) a bit about the benefits and risks.
Thanks. Drafted for the moment, but going to come back here soon with much more information.
(https://github.com/bitcoin/bitcoin/pull/25391#issuecomment-2278201819)
> Would be nice to extend the motivation (pull request description) a bit about the benefits and risks.
Thanks. Drafted for the moment, but going to come back here soon with much more information.
📝 fanquake converted_to_draft a pull request: "guix: Use LTO to build releases"
(https://github.com/bitcoin/bitcoin/pull/25391)
Changes required to use LTO in Guix (release) builds.
(https://github.com/bitcoin/bitcoin/pull/25391)
Changes required to use LTO in Guix (release) builds.
💬 hebasto commented on pull request "build: Introduce CMake-based build system":
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711674603)
> are we planning on updating them here or in a separate pr, e.g. when removing Autotools?
Here. Feel free to open a PR in https://github.com/hebasto/bitcoin/pulls against the `cmake-staging` branch. After merging, the changes will be pulled in here during the next update.
(https://github.com/bitcoin/bitcoin/pull/30454#discussion_r1711674603)
> are we planning on updating them here or in a separate pr, e.g. when removing Autotools?
Here. Feel free to open a PR in https://github.com/hebasto/bitcoin/pulls against the `cmake-staging` branch. After merging, the changes will be pulled in here during the next update.
💬 fanquake commented on pull request "[RFC] Switch and/or align debugging flags (back) to `-Og`":
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2278208327)
@theuni reminder that you might want to followup here at some point, re recent discussion/debugging.
(https://github.com/bitcoin/bitcoin/pull/29796#issuecomment-2278208327)
@theuni reminder that you might want to followup here at some point, re recent discussion/debugging.
💬 paplorinc commented on pull request "refactor: Implement missing error checking for ArgsManager flags":
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1711689242)
Pulled these to another PR: [`d1e3f0e` (#30618)](https://github.com/bitcoin/bitcoin/pull/30618/commits/d1e3f0e8a9df410317a120b81087fa4a0cddc480)
note1: nit: `// Enable BOOST_CHECK_EQUAL for enum class types` could be put inside the `std` now that we have multiple
note2: we might want to move the remaining ones in the cpp file here as well (inline-ing them all, of course)
(https://github.com/bitcoin/bitcoin/pull/16545#discussion_r1711689242)
Pulled these to another PR: [`d1e3f0e` (#30618)](https://github.com/bitcoin/bitcoin/pull/30618/commits/d1e3f0e8a9df410317a120b81087fa4a0cddc480)
note1: nit: `// Enable BOOST_CHECK_EQUAL for enum class types` could be put inside the `std` now that we have multiple
note2: we might want to move the remaining ones in the cpp file here as well (inline-ing them all, of course)
💬 ryanofsky commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#issuecomment-2278235877)
Rebased cf9aaf4b7f27b38bf201aa39c56c9967f3b2a904 -> 4137419b0170fa1e9ff9daacde57933f7c995b76 ([`pr/ipc-bind.3`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-bind.3) -> [`pr/ipc-bind.4`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-bind.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-bind.3-rebase..pr/ipc-bind.4)) with fix for new test that was failing when run in the multiprocess CI container https://github.com/bitcoin/bitcoin/pull/30509#issuecomment-2276739868
(https://github.com/bitcoin/bitcoin/pull/30509#issuecomment-2278235877)
Rebased cf9aaf4b7f27b38bf201aa39c56c9967f3b2a904 -> 4137419b0170fa1e9ff9daacde57933f7c995b76 ([`pr/ipc-bind.3`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-bind.3) -> [`pr/ipc-bind.4`](https://github.com/ryanofsky/bitcoin/commits/pr/ipc-bind.4), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ipc-bind.3-rebase..pr/ipc-bind.4)) with fix for new test that was failing when run in the multiprocess CI container https://github.com/bitcoin/bitcoin/pull/30509#issuecomment-2276739868
🤔 mzumsande reviewed a pull request: "validation: write chainstate to disk every hour"
(https://github.com/bitcoin/bitcoin/pull/30611#pullrequestreview-2230536256)
Did you check if this would slow down IBD noticeably when running with a large dbcache?
If i understand it right, coins that are created and destroyed more than 1 hour of syncing apart would not have been written to disk before, but will be written and deleted after this PR - though presumably this is not a frequent event, so there wouldn't be much of an impact?!
(https://github.com/bitcoin/bitcoin/pull/30611#pullrequestreview-2230536256)
Did you check if this would slow down IBD noticeably when running with a large dbcache?
If i understand it right, coins that are created and destroyed more than 1 hour of syncing apart would not have been written to disk before, but will be written and deleted after this PR - though presumably this is not a frequent event, so there wouldn't be much of an impact?!
💬 glozow commented on pull request "cluster mempool: optimized candidate search":
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1711719710)
Oh of course, I was confusing myself :+1:
(https://github.com/bitcoin/bitcoin/pull/30286#discussion_r1711719710)
Oh of course, I was confusing myself :+1:
📝 paplorinc opened a pull request: "Change `nproc` in docs to `getconf _NPROCESSORS_ONLN`"
(https://github.com/bitcoin/bitcoin/pull/30619)
Split out of https://github.com/hebasto/bitcoin/pull/316/files#r1711537463
nproc is linux only, getconf _NPROCESSORS_ONLN is used in the Linux kernel for the same task: https://github.com/torvalds/linux/blob/master/tools/testing/selftests/rcutorture/bin/kvm-build.sh#L44
On linux:
```
# nproc
8
root@rescue ~ # getconf _NPROCESSORS_ONLN
8
```
Note that in the productivity doc the recommended core count is one less than before, seems simpler to me this way.
(https://github.com/bitcoin/bitcoin/pull/30619)
Split out of https://github.com/hebasto/bitcoin/pull/316/files#r1711537463
nproc is linux only, getconf _NPROCESSORS_ONLN is used in the Linux kernel for the same task: https://github.com/torvalds/linux/blob/master/tools/testing/selftests/rcutorture/bin/kvm-build.sh#L44
On linux:
```
# nproc
8
root@rescue ~ # getconf _NPROCESSORS_ONLN
8
```
Note that in the productivity doc the recommended core count is one less than before, seems simpler to me this way.
👍 itornaza approved a pull request: "Stratum v2 Noise Protocol"
(https://github.com/bitcoin/bitcoin/pull/29346#pullrequestreview-2230564992)
Code review ACK e73740fde107f79eeee8e55c6bed156713abe8a5
This pr closely follows the intersection of the original [noise protocol paper](https://noiseprotocol.org/noise.pdf) and the [Protocol Security section](https://github.com/stratum-mining/sv2-spec/blob/main/04-Protocol-Security.md) of the Stratum V2 Specification to provide an implementation of the Noise Protocol on Bitcoin Core. In that way we can now step by step integrate the Stratum protocol Template Provider and Job Declaration Clie
...
(https://github.com/bitcoin/bitcoin/pull/29346#pullrequestreview-2230564992)
Code review ACK e73740fde107f79eeee8e55c6bed156713abe8a5
This pr closely follows the intersection of the original [noise protocol paper](https://noiseprotocol.org/noise.pdf) and the [Protocol Security section](https://github.com/stratum-mining/sv2-spec/blob/main/04-Protocol-Security.md) of the Stratum V2 Specification to provide an implementation of the Noise Protocol on Bitcoin Core. In that way we can now step by step integrate the Stratum protocol Template Provider and Job Declaration Clie
...
💬 ajtowns commented on pull request "contrib/signet/miner updates":
(https://github.com/bitcoin/bitcoin/pull/28417#discussion_r1711764521)
Added a commit to use mutually exclusive groups here and in a few other places.
(https://github.com/bitcoin/bitcoin/pull/28417#discussion_r1711764521)
Added a commit to use mutually exclusive groups here and in a few other places.
💬 achow101 commented on pull request "node: reduce unsafe uint256S usage":
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2278320928)
> `-assumevalid` will raise an error when input contains invalid hex characters (including `0x` prefix), or when it is not exactly 64 characters long
Why not allow `0x` prefixes as `-minimumchainwork` does? It seems plausible to me that someone might have put a `0x` prefixed block hash in their conf file and this change breaks that.
(https://github.com/bitcoin/bitcoin/pull/30569#issuecomment-2278320928)
> `-assumevalid` will raise an error when input contains invalid hex characters (including `0x` prefix), or when it is not exactly 64 characters long
Why not allow `0x` prefixes as `-minimumchainwork` does? It seems plausible to me that someone might have put a `0x` prefixed block hash in their conf file and this change breaks that.
👍 pablomartin4btc approved a pull request: "Migrate legacy wallets that are not loaded"
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2230620169)
> > Tested [d45ac03](https://github.com/bitcoin-core/gui/commit/d45ac03a89cc500cd461f878f092cf8ec99e7760). The "Migrate Wallet" menu item is still disabled when no wallet is loaded.
>
> Should be fixed now.
>
re-ACK bcf47b9f34e4f21e5ac75521ddd4f25ff3812412
(https://github.com/bitcoin-core/gui/pull/824#pullrequestreview-2230620169)
> > Tested [d45ac03](https://github.com/bitcoin-core/gui/commit/d45ac03a89cc500cd461f878f092cf8ec99e7760). The "Migrate Wallet" menu item is still disabled when no wallet is loaded.
>
> Should be fixed now.
>
re-ACK bcf47b9f34e4f21e5ac75521ddd4f25ff3812412
💬 achow101 commented on pull request "doc, chainparams: 29775 release notes and follow-ups":
(https://github.com/bitcoin/bitcoin/pull/30604#issuecomment-2278345688)
ACK 92c1d7d1f8664fe2d259cc609c87f603609b2b60
(https://github.com/bitcoin/bitcoin/pull/30604#issuecomment-2278345688)
ACK 92c1d7d1f8664fe2d259cc609c87f603609b2b60
💬 sipa commented on pull request "Fixes for GCC 15 compatibility":
(https://github.com/bitcoin/bitcoin/pull/30612#discussion_r1711792420)
I don't have much of an opinion about how valuable this comment is, but for the same of argument, I would not say that "Disallow silent float -> int conversion" carries no meaning beyond what `std::integral I` implies.
`std::integral I` means that the function cannot be instantiated with I anything but an integer. The comment additionally clarifies that it's there because (a) specifically we do not want to allow floats and (b) that this is because of unexpected effects from silently convertin
...
(https://github.com/bitcoin/bitcoin/pull/30612#discussion_r1711792420)
I don't have much of an opinion about how valuable this comment is, but for the same of argument, I would not say that "Disallow silent float -> int conversion" carries no meaning beyond what `std::integral I` implies.
`std::integral I` means that the function cannot be instantiated with I anything but an integer. The comment additionally clarifies that it's there because (a) specifically we do not want to allow floats and (b) that this is because of unexpected effects from silently convertin
...
💬 andrewtoth commented on pull request "validation: write chainstate to disk every hour":
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2278349582)
> Before someone suggests that it should do this every block, a reason to not do this too much is that delaying writes prevents many writes from ever happening at all because many outputs are quickly spent.
> If i understand it right, coins that are created and destroyed more than 1 hour of syncing apart would not have been written to disk before, but will be written and deleted after this PR
Correct, there will be significantly less utxo cut through with this PR.
> I don't see a partic
...
(https://github.com/bitcoin/bitcoin/pull/30611#issuecomment-2278349582)
> Before someone suggests that it should do this every block, a reason to not do this too much is that delaying writes prevents many writes from ever happening at all because many outputs are quickly spent.
> If i understand it right, coins that are created and destroyed more than 1 hour of syncing apart would not have been written to disk before, but will be written and deleted after this PR
Correct, there will be significantly less utxo cut through with this PR.
> I don't see a partic
...
🚀 achow101 merged a pull request: "doc, chainparams: 29775 release notes and follow-ups"
(https://github.com/bitcoin/bitcoin/pull/30604)
(https://github.com/bitcoin/bitcoin/pull/30604)
👍 andrewtoth approved a pull request: "optimization: Reduce cache lookups in CCoinsViewCache::FetchCoin"
(https://github.com/bitcoin/bitcoin/pull/30326#pullrequestreview-2230764899)
re-ACK 204ca67bba263018374fe86d7a6867362d09536f
(https://github.com/bitcoin/bitcoin/pull/30326#pullrequestreview-2230764899)
re-ACK 204ca67bba263018374fe86d7a6867362d09536f
💬 furszy commented on pull request "wallet: Fix listwalletdir listing of migrated default wallets and generated backup files":
(https://github.com/bitcoin/bitcoin/pull/30265#discussion_r1711906769)
this line does not seems to be related to a700b303fcba7915f.
(https://github.com/bitcoin/bitcoin/pull/30265#discussion_r1711906769)
this line does not seems to be related to a700b303fcba7915f.
💬 whitslack commented on pull request "Fixes for GCC 15 compatibility":
(https://github.com/bitcoin/bitcoin/pull/30612#discussion_r1711941341)
`std::integral` already means you specifically do not want to allow floats. If you did want to allow floats, you would have used `std::is_arithmetic_v`, but you didn't, so therefore you specifically want to exclude floats.
I'll concede the point that deleting the comment loses the information that you've previously had issues with silent conversions, although the presence of the constraint guarantees that you'll never have such issues again, so signaling about the previous issues would only s
...
(https://github.com/bitcoin/bitcoin/pull/30612#discussion_r1711941341)
`std::integral` already means you specifically do not want to allow floats. If you did want to allow floats, you would have used `std::is_arithmetic_v`, but you didn't, so therefore you specifically want to exclude floats.
I'll concede the point that deleting the comment loses the information that you've previously had issues with silent conversions, although the presence of the constraint guarantees that you'll never have such issues again, so signaling about the previous issues would only s
...
⚠️ wydengyre opened an issue: "wallet: setting changes are subject to race conditions"
(https://github.com/bitcoin/bitcoin/issues/30620)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Creating two wallets with `load_on_startup=true` simultaneously will result in only one of the wallets being added to the startup file.
### Expected behaviour
Both wallets should be added.
### Steps to reproduce
I don't have an easy script to reproduce this, but the issue is quite visible in the code:
https://github.com/bitcoin/bitcoin/blob/389cf32aca0be6c4b01151c92cc3be79c120f197/src
...
(https://github.com/bitcoin/bitcoin/issues/30620)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
Creating two wallets with `load_on_startup=true` simultaneously will result in only one of the wallets being added to the startup file.
### Expected behaviour
Both wallets should be added.
### Steps to reproduce
I don't have an easy script to reproduce this, but the issue is quite visible in the code:
https://github.com/bitcoin/bitcoin/blob/389cf32aca0be6c4b01151c92cc3be79c120f197/src
...