💬 pinheadmz commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3031861731)
Opened https://github.com/bitcoin/bitcoin/pull/32862 with the suggested approach, closing this in favor of that.
(https://github.com/bitcoin/bitcoin/pull/32796#issuecomment-3031861731)
Opened https://github.com/bitcoin/bitcoin/pull/32862 with the suggested approach, closing this in favor of that.
📝 fanquake opened a pull request: "[29.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/32863)
Backports
- #32716
- #32823
- #32841
- #32850
- #32859
(https://github.com/bitcoin/bitcoin/pull/32863)
Backports
- #32716
- #32823
- #32841
- #32850
- #32859
📝 kilavvy opened a pull request: "fix typos and improve clarity in documentation"
(https://github.com/bitcoin/bitcoin/pull/32864)
---
**Description:**
This pull request corrects minor typos and improves clarity in the following documentation files:
- build-osx.md: Fixes `useable` to `usable`
- tor.md: Fixes `occurences` to `occurrences`
- tracing.md: Fixes `calulation` to `calculation`
---
(https://github.com/bitcoin/bitcoin/pull/32864)
---
**Description:**
This pull request corrects minor typos and improves clarity in the following documentation files:
- build-osx.md: Fixes `useable` to `usable`
- tor.md: Fixes `occurences` to `occurrences`
- tracing.md: Fixes `calulation` to `calculation`
---
💬 fanquake commented on pull request "depends: Override host compilers for FreeBSD and OpenBSD":
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-3031890700)
Backported to 29.x in #32863.
(https://github.com/bitcoin/bitcoin/pull/32716#issuecomment-3031890700)
Backported to 29.x in #32863.
💬 fanquake commented on pull request "test: Fix wait_for_getheaders() call in test_outbound_eviction_blocks_relay_only()":
(https://github.com/bitcoin/bitcoin/pull/32823#issuecomment-3031891294)
Backported to 29.x in #32863.
(https://github.com/bitcoin/bitcoin/pull/32823#issuecomment-3031891294)
Backported to 29.x in #32863.
💬 fanquake commented on pull request "test: check P2SH sigop count for coinbase tx":
(https://github.com/bitcoin/bitcoin/pull/32850#issuecomment-3031892789)
Backported to 29.x in #32863.
(https://github.com/bitcoin/bitcoin/pull/32850#issuecomment-3031892789)
Backported to 29.x in #32863.
💬 fanquake commented on pull request "feature_taproot: sample tx version border values more":
(https://github.com/bitcoin/bitcoin/pull/32841#issuecomment-3031893606)
Backported to 29.x in #32863.
(https://github.com/bitcoin/bitcoin/pull/32841#issuecomment-3031893606)
Backported to 29.x in #32863.
💬 hebasto commented on pull request "ci: update pwsh to use custom shell that fails-fast":
(https://github.com/bitcoin/bitcoin/pull/32672#issuecomment-3031898351)
Concept ACK.
Defining a new custom `job.defaults.run.shell` for all Windows jobs allows us to revert f8619196ceb5c6a58125506d276d9515837f043a and use the same shell across all steps. This is especially convenient for reproducing failures locally.
(https://github.com/bitcoin/bitcoin/pull/32672#issuecomment-3031898351)
Concept ACK.
Defining a new custom `job.defaults.run.shell` for all Windows jobs allows us to revert f8619196ceb5c6a58125506d276d9515837f043a and use the same shell across all steps. This is especially convenient for reproducing failures locally.
📝 fanquake opened a pull request: "cmake: Use `AUTHOR_WARNING` for warnings"
(https://github.com/bitcoin/bitcoin/pull/32865)
Rather than create our own warning logging/handling, which isn't ideal (#31476), use CMakes [`AUTHOR_WARNING`](https://cmake.org/cmake/help/latest/command/message.html) (also pointed out by purpleKarrot here: https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2667723356), and turn on `-Werror=dev` in the CI.
Would result in failures like:
```bash
-- Performing Test LINKER_SUPPORTS__WL__Z_SEPARATE_CODE - Success
-- Could NOT find Python3 (missing: Python3_EXECUTABLE Interpreter) (Re
...
(https://github.com/bitcoin/bitcoin/pull/32865)
Rather than create our own warning logging/handling, which isn't ideal (#31476), use CMakes [`AUTHOR_WARNING`](https://cmake.org/cmake/help/latest/command/message.html) (also pointed out by purpleKarrot here: https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2667723356), and turn on `-Werror=dev` in the CI.
Would result in failures like:
```bash
-- Performing Test LINKER_SUPPORTS__WL__Z_SEPARATE_CODE - Success
-- Could NOT find Python3 (missing: Python3_EXECUTABLE Interpreter) (Re
...
✅ fanquake closed a pull request: "fix typos and improve clarity in documentation"
(https://github.com/bitcoin/bitcoin/pull/32864)
(https://github.com/bitcoin/bitcoin/pull/32864)
💬 fanquake commented on pull request "fix typos and improve clarity in documentation":
(https://github.com/bitcoin/bitcoin/pull/32864#issuecomment-3031908342)
Thanks. I'll squash this and pull it into another branch.
(https://github.com/bitcoin/bitcoin/pull/32864#issuecomment-3031908342)
Thanks. I'll squash this and pull it into another branch.
💬 kilavvy commented on pull request "fix typos and improve clarity in documentation":
(https://github.com/bitcoin/bitcoin/pull/32864#issuecomment-3031914311)
> Thanks. I'll squash this and pull it into another branch.
@fanquake why not merged?
(https://github.com/bitcoin/bitcoin/pull/32864#issuecomment-3031914311)
> Thanks. I'll squash this and pull it into another branch.
@fanquake why not merged?
💬 l0rinc commented on pull request "fix typos and improve clarity in documentation":
(https://github.com/bitcoin/bitcoin/pull/32864#discussion_r2182542317)
https://www.collinsdictionary.com/dictionary/english/useable
Try to contribute something that provides more value
(https://github.com/bitcoin/bitcoin/pull/32864#discussion_r2182542317)
https://www.collinsdictionary.com/dictionary/english/useable
Try to contribute something that provides more value
💬 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_r2182544165)
I added this recursive `LogPrintStr_` because I think it's better to have separate log statements than inserting into the existing string, and added the `should_ratelimit=false` which makes it safe for infinite recursion. I forgot to add the clang-tidy suppression though, [causing this CI failure](https://github.com/bitcoin/bitcoin/pull/32604/checks?check_run_id=45248600899). It seems like we can only suppress this by marking all of `LogPrintStr_` as `NOLINT`, instead of just this single source,
...
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2182544165)
I added this recursive `LogPrintStr_` because I think it's better to have separate log statements than inserting into the existing string, and added the `should_ratelimit=false` which makes it safe for infinite recursion. I forgot to add the clang-tidy suppression though, [causing this CI failure](https://github.com/bitcoin/bitcoin/pull/32604/checks?check_run_id=45248600899). It seems like we can only suppress this by marking all of `LogPrintStr_` as `NOLINT`, instead of just this single source,
...
💬 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_r2182461594)
This is causing a [TSan issue](https://cirrus-ci.com/task/6014464291504128?logs=ci#L2383):
<details>
<summary> logs </summary>
```
[18:53:46.472] WARNING: ThreadSanitizer: data race (pid=9715)
[18:53:46.472] Read of size 8 at 0x723c00000068 by thread T1 (mutexes: write M0):
[18:53:46.472] #0 std::__1::unique_ptr<BCLog::LogRateLimiter, std::__1::default_delete<BCLog::LogRateLimiter>>::operator bool[abi:ne200100]() const /usr/lib/llvm-20/bin/../include/c++/v1/__memory/unique_ptr.h:
...
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2182461594)
This is causing a [TSan issue](https://cirrus-ci.com/task/6014464291504128?logs=ci#L2383):
<details>
<summary> logs </summary>
```
[18:53:46.472] WARNING: ThreadSanitizer: data race (pid=9715)
[18:53:46.472] Read of size 8 at 0x723c00000068 by thread T1 (mutexes: write M0):
[18:53:46.472] #0 std::__1::unique_ptr<BCLog::LogRateLimiter, std::__1::default_delete<BCLog::LogRateLimiter>>::operator bool[abi:ne200100]() const /usr/lib/llvm-20/bin/../include/c++/v1/__memory/unique_ptr.h:
...
💬 l0rinc commented on pull request "test: check P2SH sigop count for coinbase tx":
(https://github.com/bitcoin/bitcoin/pull/32850#issuecomment-3031930388)
post-merge crACK
(https://github.com/bitcoin/bitcoin/pull/32850#issuecomment-3031930388)
post-merge crACK
💬 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_r2182549098)
Sorry, please disregard. I think I misread this as `std::string, threadname` and probably didn't even try compiling the suggestion.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2182549098)
Sorry, please disregard. I think I misread this as `std::string, threadname` and probably didn't even try compiling the suggestion.
💬 kilavvy commented on pull request "fix typos and improve clarity in documentation":
(https://github.com/bitcoin/bitcoin/pull/32864#issuecomment-3031934039)
@l0rinc I can revert the change if needed, no problem. I also fixed a typo in the function name — it's now correctly spelled as `expensive_calculation`. I do believe that documentation and naming consistency are important, especially in a project like Bitcoin, where clarity helps both users and developers avoid confusion.
(https://github.com/bitcoin/bitcoin/pull/32864#issuecomment-3031934039)
@l0rinc I can revert the change if needed, no problem. I also fixed a typo in the function name — it's now correctly spelled as `expensive_calculation`. I do believe that documentation and naming consistency are important, especially in a project like Bitcoin, where clarity helps both users and developers avoid confusion.
💬 maflcko commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2182569619)
should this also be enabled in `.github/ci-test-each-commit-exec.py`, `.github/workflows/ci.yml`, and `contrib/guix/libexec/build.sh`? Otherwise, it is possible that warnings are missed in those contexts?
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2182569619)
should this also be enabled in `.github/ci-test-each-commit-exec.py`, `.github/workflows/ci.yml`, and `contrib/guix/libexec/build.sh`? Otherwise, it is possible that warnings are missed in those contexts?
💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2182576679)
Will implement. I should have caught this as well -- I am glad we have TSAN in CI. I will test the branch locally under TSAN.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2182576679)
Will implement. I should have caught this as well -- I am glad we have TSAN in CI. I will test the branch locally under TSAN.