💬 davidgumberg commented on pull request "deps: Bump lief to 0.16.6":
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2921038966)
- Rebased against master to include https://github.com/bitcoin/bitcoin/pull/32634/commits/dbb2d4c3d54759f8c346882b6f98d26d5bb8e816
- Bumped LIEF to 0.16.6 which resolves https://github.com/lief-project/LIEF/issues/1217
- Added a workaround for https://github.com/lief-project/LIEF/pull/1218, this is acceptable and won't need to be resolved once the upstream patch is merged, since this change tightens our MachO `has_nx` checks to both the stack and heap.
- Added a commit to enable a previously
...
(https://github.com/bitcoin/bitcoin/pull/32431#issuecomment-2921038966)
- Rebased against master to include https://github.com/bitcoin/bitcoin/pull/32634/commits/dbb2d4c3d54759f8c346882b6f98d26d5bb8e816
- Bumped LIEF to 0.16.6 which resolves https://github.com/lief-project/LIEF/issues/1217
- Added a workaround for https://github.com/lief-project/LIEF/pull/1218, this is acceptable and won't need to be resolved once the upstream patch is merged, since this change tightens our MachO `has_nx` checks to both the stack and heap.
- Added a commit to enable a previously
...
👋 davidgumberg's pull request is ready for review: "deps: Bump lief to 0.16.6"
(https://github.com/bitcoin/bitcoin/pull/32431)
(https://github.com/bitcoin/bitcoin/pull/32431)
💬 Sjors commented on pull request "Update libmultiprocess subtree to fix clang-tidy errors":
(https://github.com/bitcoin/bitcoin/pull/32641#issuecomment-2921262265)
ACK 9f6565488fc169898abc5f7ee620ce7a9a4e5e50
Tested with `git-subtree-check.sh -r`
(https://github.com/bitcoin/bitcoin/pull/32641#issuecomment-2921262265)
ACK 9f6565488fc169898abc5f7ee620ce7a9a4e5e50
Tested with `git-subtree-check.sh -r`
💬 Sjors commented on pull request "Add checkBlock() to Mining interface":
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2115179447)
At least we have a test that checks the UTXO set isn't updated: https://github.com/bitcoin/bitcoin/blob/1def3bc3773b3013a92f54daa75acb82c30beaac/test/functional/mining_template_verification.py#L145-L156
But that doesn't cover all conceivable future ways that we might accidentally flush.
If we do want to add a belt-and-suspenders way to prevent such a flush, it seems better for a followup.
> there could be better ways of preventing it like adding a read-only option to `CCoinsViewCache` t
...
(https://github.com/bitcoin/bitcoin/pull/31981#discussion_r2115179447)
At least we have a test that checks the UTXO set isn't updated: https://github.com/bitcoin/bitcoin/blob/1def3bc3773b3013a92f54daa75acb82c30beaac/test/functional/mining_template_verification.py#L145-L156
But that doesn't cover all conceivable future ways that we might accidentally flush.
If we do want to add a belt-and-suspenders way to prevent such a flush, it seems better for a followup.
> there could be better ways of preventing it like adding a read-only option to `CCoinsViewCache` t
...
💬 Sjors commented on pull request "cmake: Move internal binaries from bin/ to libexec/":
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-2921299759)
Just #28710, no conflict with #31375? Will re-review shortly.
(https://github.com/bitcoin/bitcoin/pull/31679#issuecomment-2921299759)
Just #28710, no conflict with #31375? Will re-review shortly.
🚀 fanquake merged a pull request: "Update libmultiprocess subtree to fix clang-tidy errors"
(https://github.com/bitcoin/bitcoin/pull/32641)
(https://github.com/bitcoin/bitcoin/pull/32641)
⚠️ fanquake opened an issue: "fs: use `ftruncate` on OpenBSD in `AllocateFileRange`"
(https://github.com/bitcoin/bitcoin/issues/32643)
OpenBSD doesn't have `fallocate` or `posix_fallocate`, which means it's hitting our fallback. We should instead have it use `ftruncate`. See related discussion in #32460.
cc @theStack.
(https://github.com/bitcoin/bitcoin/issues/32643)
OpenBSD doesn't have `fallocate` or `posix_fallocate`, which means it's hitting our fallback. We should instead have it use `ftruncate`. See related discussion in #32460.
cc @theStack.
💬 fanquake commented on pull request "fs: remove `_POSIX_C_SOURCE` defining":
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2921749068)
Opened #32643 to followup.
(https://github.com/bitcoin/bitcoin/pull/32460#issuecomment-2921749068)
Opened #32643 to followup.
🚀 fanquake merged a pull request: "fs: remove `_POSIX_C_SOURCE` defining"
(https://github.com/bitcoin/bitcoin/pull/32460)
(https://github.com/bitcoin/bitcoin/pull/32460)
📝 fanquake converted_to_draft a pull request: "threading: remove ancient CRITICAL_SECTION macros"
(https://github.com/bitcoin/bitcoin/pull/32592)
Now that #32467 is merged, the only remaining usage of our old `CRITICAL_SECTION` macros (other than tests) is in `getblocktemplate()` and it can safely be replaced with a `REVERSE_LOCK`.
This PR makes that replacement, replaces the old `CRITICAL_SECTION` macro usage in tests, then deletes the macros themselves.
While testing this a few weeks ago, I noticed that `REVERSE_LOCK` does not currently work properly with our thread-safety annotations as after the `REVERSE_LOCK` is acquired, clang
...
(https://github.com/bitcoin/bitcoin/pull/32592)
Now that #32467 is merged, the only remaining usage of our old `CRITICAL_SECTION` macros (other than tests) is in `getblocktemplate()` and it can safely be replaced with a `REVERSE_LOCK`.
This PR makes that replacement, replaces the old `CRITICAL_SECTION` macro usage in tests, then deletes the macros themselves.
While testing this a few weeks ago, I noticed that `REVERSE_LOCK` does not currently work properly with our thread-safety annotations as after the `REVERSE_LOCK` is acquired, clang
...
💬 fanquake commented on pull request "test: update BIP340 test vectors and implementation (variable-length messages)":
(https://github.com/bitcoin/bitcoin/pull/32642#issuecomment-2921839411)
cc @sipa @real-or-random @jonasnick
(https://github.com/bitcoin/bitcoin/pull/32642#issuecomment-2921839411)
cc @sipa @real-or-random @jonasnick
💬 Sjors commented on pull request "Add bitcoin-{node,gui} to release binaries for IPC":
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2921871074)
https://github.com/bitcoin-core/libmultiprocess/pull/172 landed and the subtree was updated in #32641.
Marking ready to review again.
Although #31679 is orthogonal, it's also worth reviewing. There should be no impact in user experience, because thanks to https://github.com/bitcoin/bitcoin/pull/31375 `bitcoin -m` will find `bitcoin-node` whether it's in `/usr/local/bin` or `/usr/local/libexec`. But an upgrade would leave one binary in both paths, so it's nicer to get that PR in the same re
...
(https://github.com/bitcoin/bitcoin/pull/31802#issuecomment-2921871074)
https://github.com/bitcoin-core/libmultiprocess/pull/172 landed and the subtree was updated in #32641.
Marking ready to review again.
Although #31679 is orthogonal, it's also worth reviewing. There should be no impact in user experience, because thanks to https://github.com/bitcoin/bitcoin/pull/31375 `bitcoin -m` will find `bitcoin-node` whether it's in `/usr/local/bin` or `/usr/local/libexec`. But an upgrade would leave one binary in both paths, so it's nicer to get that PR in the same re
...
👋 Sjors's pull request is ready for review: "Add bitcoin-{node,gui} to release binaries for IPC"
(https://github.com/bitcoin/bitcoin/pull/31802)
(https://github.com/bitcoin/bitcoin/pull/31802)
📝 fanquake opened a pull request: "doc: miscellaneous changes"
(https://github.com/bitcoin/bitcoin/pull/32644)
A round up of #32629 + some other changes that had previously been PR'd.
(https://github.com/bitcoin/bitcoin/pull/32644)
A round up of #32629 + some other changes that had previously been PR'd.
🚀 fanquake merged a pull request: "[28.x] Backport guix: accomodate migration to codeberg"
(https://github.com/bitcoin/bitcoin/pull/32639)
(https://github.com/bitcoin/bitcoin/pull/32639)
💬 TheCharlatan commented on pull request "headerssync: Keep tests ahead of increasing params":
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-2921899092)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/32579#issuecomment-2921899092)
Concept ACK
🚀 fanquake merged a pull request: "wallet, rpc, gui: List legacy wallets with a message about migration"
(https://github.com/bitcoin/bitcoin/pull/32619)
(https://github.com/bitcoin/bitcoin/pull/32619)
🤔 stickies-v reviewed a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2880741415)
Concept ACK on locking down this attack vector, and the code changes required seem acceptable. Also nice to start using `std::source_location` here.
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-2880741415)
Concept ACK on locking down this attack vector, and the code changes required seem acceptable. Also nice to start using `std::source_location` here.
💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115514944)
nit: I think a brief docstring on their suggested usage here would be helpful here
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115514944)
nit: I think a brief docstring on their suggested usage here would be helpful here
💬 stickies-v commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115526293)
Is there a good use case for disabling rate limiting? I can't think of one. Letting the user define the amount of rate limiting is an alternative, but even then, I'm not sure. If an attacker or bug is able to reach the sane default maximum, then is the user really going to get any benefit from being to inspect the additional volume of un-ratelimited logs? I'm not sure. If so, I think it would make sense to not have this as a configurable option.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2115526293)
Is there a good use case for disabling rate limiting? I can't think of one. Letting the user define the amount of rate limiting is an alternative, but even then, I'm not sure. If an attacker or bug is able to reach the sane default maximum, then is the user really going to get any benefit from being to inspect the additional volume of un-ratelimited logs? I'm not sure. If so, I think it would make sense to not have this as a configurable option.