💬 Sjors commented on pull request "rpc: use anti-fee-sniping in send, sendall and walletcreatefundedpsbt":
(https://github.com/bitcoin/bitcoin/pull/32892#issuecomment-3051457537)
Ok, so #28944 uses a completely different approach. It already has a lot of review, so I'll add mine there.
(https://github.com/bitcoin/bitcoin/pull/32892#issuecomment-3051457537)
Ok, so #28944 uses a completely different approach. It already has a lot of review, so I'll add mine there.
💬 Sjors commented on pull request "wallet, rpc: add anti-fee-sniping to `send` and `sendall`":
(https://github.com/bitcoin/bitcoin/pull/28944#issuecomment-3051460115)
Concept ACK. Discovered this after I tried to implement it myself in #32892 :-)
(https://github.com/bitcoin/bitcoin/pull/28944#issuecomment-3051460115)
Concept ACK. Discovered this after I tried to implement it myself in #32892 :-)
💬 Evoneem commented on pull request "rpc: use anti-fee-sniping in send, sendall and walletcreatefundedpsbt":
(https://github.com/bitcoin/bitcoin/pull/32892#discussion_r2194341665)
Anti free sniping on future merging please
(https://github.com/bitcoin/bitcoin/pull/32892#discussion_r2194341665)
Anti free sniping on future merging please
🤔 Evoneem reviewed a pull request: "rpc: use anti-fee-sniping in send, sendall and walletcreatefundedpsbt"
(https://github.com/bitcoin/bitcoin/pull/32892#pullrequestreview-3000475960)
Anti free sniping on merges
(https://github.com/bitcoin/bitcoin/pull/32892#pullrequestreview-3000475960)
Anti free sniping on merges
💬 fanquake commented on pull request "depends: fix libevent `_WIN32_WINNT` usage":
(https://github.com/bitcoin/bitcoin/pull/32837#issuecomment-3051653595)
Guix Build:
```bash
1057d4ff8082f78c85f283799c8ccd35096aa69600b5b655746b0c61b5511266 guix-build-2f51b7c5d61d/output/aarch64-linux-gnu/SHA256SUMS.part
17365752cefaec8fa2b46d8065d458d8c15bebd16dea79240ba082d837c1af04 guix-build-2f51b7c5d61d/output/aarch64-linux-gnu/bitcoin-2f51b7c5d61d-aarch64-linux-gnu-debug.tar.gz
762a75e7c752855a2e099a662e5d8cfdebebeacb35f1ab86a831e3310082f761 guix-build-2f51b7c5d61d/output/aarch64-linux-gnu/bitcoin-2f51b7c5d61d-aarch64-linux-gnu.tar.gz
51cfb4cc939d2530
...
(https://github.com/bitcoin/bitcoin/pull/32837#issuecomment-3051653595)
Guix Build:
```bash
1057d4ff8082f78c85f283799c8ccd35096aa69600b5b655746b0c61b5511266 guix-build-2f51b7c5d61d/output/aarch64-linux-gnu/SHA256SUMS.part
17365752cefaec8fa2b46d8065d458d8c15bebd16dea79240ba082d837c1af04 guix-build-2f51b7c5d61d/output/aarch64-linux-gnu/bitcoin-2f51b7c5d61d-aarch64-linux-gnu-debug.tar.gz
762a75e7c752855a2e099a662e5d8cfdebebeacb35f1ab86a831e3310082f761 guix-build-2f51b7c5d61d/output/aarch64-linux-gnu/bitcoin-2f51b7c5d61d-aarch64-linux-gnu.tar.gz
51cfb4cc939d2530
...
💬 fanquake commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#issuecomment-3051783625)
@ryanofsky Yea, it's a shame CMake doesn't have something built in to do this. I think this change, while not perfect, is better than the current state of having our own warning handling (where warnings are still easily missed (#31476), we have no option to turn them into errors, and someone still needs to pass `-Wdev` to get CMake warnings turned into errors). As well as the alternatives of, just adding even more options to the build system (#31669) which doesn't actually fix the issue generica
...
(https://github.com/bitcoin/bitcoin/pull/32865#issuecomment-3051783625)
@ryanofsky Yea, it's a shame CMake doesn't have something built in to do this. I think this change, while not perfect, is better than the current state of having our own warning handling (where warnings are still easily missed (#31476), we have no option to turn them into errors, and someone still needs to pass `-Wdev` to get CMake warnings turned into errors). As well as the alternatives of, just adding even more options to the build system (#31669) which doesn't actually fix the issue generica
...
💬 maflcko commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2194479995)
> Reintroducing this block + your patch will only do something if we also reintroduce the custom warning handling (`configure_warnings`
Yes, that was my suggestion: Replace the commit with a trivial one-line patch.
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2194479995)
> Reintroducing this block + your patch will only do something if we also reintroduce the custom warning handling (`configure_warnings`
Yes, that was my suggestion: Replace the commit with a trivial one-line patch.
💬 purpleKarrot commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#issuecomment-3051831634)
@fanquake, what exactly would fix the issue upstream? A generic `-Werror` option (without the `=` part) that simply turns all warnings into errors?
(https://github.com/bitcoin/bitcoin/pull/32865#issuecomment-3051831634)
@fanquake, what exactly would fix the issue upstream? A generic `-Werror` option (without the `=` part) that simply turns all warnings into errors?
💬 fanquake commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#issuecomment-3051852485)
> A generic -Werror option (without the = part) that simply turns all warnings into errors?
I think so. The downside in this PR is that we have to somewhat (even if semantically) misuse `AUTHOR_WARNING()`, to get the warning into error behaviour. Ideally we could just use `WARNING()`, and `-Werror` would turn those warnings into errors.
(https://github.com/bitcoin/bitcoin/pull/32865#issuecomment-3051852485)
> A generic -Werror option (without the = part) that simply turns all warnings into errors?
I think so. The downside in this PR is that we have to somewhat (even if semantically) misuse `AUTHOR_WARNING()`, to get the warning into error behaviour. Ideally we could just use `WARNING()`, and `-Werror` would turn those warnings into errors.
💬 Rahamath-unnisa commented on issue "init: torcontrol argument should be validated":
(https://github.com/bitcoin/bitcoin/issues/23589#issuecomment-3051989757)
Hi! I'm new to Bitcoin Core development and would like to work on this issue. Can I take it?
(https://github.com/bitcoin/bitcoin/issues/23589#issuecomment-3051989757)
Hi! I'm new to Bitcoin Core development and would like to work on this issue. Can I take it?
🤔 maflcko reviewed a pull request: "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError"
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-3000242855)
left some style nits, which are trivial to ignore, or follow-up with. Nothing blocking.
review ACK f47e2ea9137c3a832e07d6dd845c55d35d533fa9 🌬
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+
...
(https://github.com/bitcoin/bitcoin/pull/32604#pullrequestreview-3000242855)
left some style nits, which are trivial to ignore, or follow-up with. Nothing blocking.
review ACK f47e2ea9137c3a832e07d6dd845c55d35d533fa9 🌬
<details><summary>Show signature</summary>
Signature:
```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+
...
💬 maflcko commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194189826)
from the llm linter:
* +LogWarning, LogError, → LogWarning, LogError` [missing backtick before comma makes the inline code span invalid]
However, all of those are implementation details anyway, and it could make sense to remove those. End-users probably wonder what those mean for them, so it could make sense to instead mention the name of the setting that affects them:
"Unconditional logging to disk is now rate limited by giving each source location a quota of 1MiB per hour. Unconditional l
...
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194189826)
from the llm linter:
* +LogWarning, LogError, → LogWarning, LogError` [missing backtick before comma makes the inline code span invalid]
However, all of those are implementation details anyway, and it could make sense to remove those. End-users probably wonder what those mean for them, so it could make sense to instead mention the name of the setting that affects them:
"Unconditional logging to disk is now rate limited by giving each source location a quota of 1MiB per hour. Unconditional l
...
💬 maflcko commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194354583)
nit in https://github.com/bitcoin/bitcoin/commit/b5d0cc29ac378f360424472f637f6d6af74660a5: Could clarify in the comment "... of bytes *per source location* ...", but just a nit.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194354583)
nit in https://github.com/bitcoin/bitcoin/commit/b5d0cc29ac378f360424472f637f6d6af74660a5: Could clarify in the comment "... of bytes *per source location* ...", but just a nit.
💬 maflcko commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194330070)
nit in b5d0cc29ac378f360424472f637f6d6af74660a5: Can remove the trailing `\n` (it is added by logging).
Also, could use the Warning log level instead, because rate-limiting should never happen in practise and if it does, it seems like something that should be fixed by adjusting the logging at that location?
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194330070)
nit in b5d0cc29ac378f360424472f637f6d6af74660a5: Can remove the trailing `\n` (it is added by logging).
Also, could use the Warning log level instead, because rate-limiting should never happen in practise and if it does, it seems like something that should be fixed by adjusting the logging at that location?
💬 maflcko commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194249282)
nit: Probably wanted to say "... *to be able* to catch ..." or "to throw any exceptions." Otherwise this reads a bit as if `noexcept(false)` were to catch any thrown exceptions. Could also just remove the comment, but just a nit.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194249282)
nit: Probably wanted to say "... *to be able* to catch ..." or "to throw any exceptions." Otherwise this reads a bit as if `noexcept(false)` were to catch any thrown exceptions. Could also just remove the comment, but just a nit.
💬 maflcko commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194621936)
nit in 6e47dad25c5a167cd171e5c11d51a7bee7c8d3c6: Could use the `HasReason` helper, but up to you.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194621936)
nit in 6e47dad25c5a167cd171e5c11d51a7bee7c8d3c6: Could use the `HasReason` helper, but up to you.
💬 maflcko commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194367325)
nit in https://github.com/bitcoin/bitcoin/commit/b5d0cc29ac378f360424472f637f6d6af74660a5: I don't think the `ll` is needed to silence the w-sign-compare warning. I'd go with just `0u`, or if you want to fully specify the type `uint64_t{0}`.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2194367325)
nit in https://github.com/bitcoin/bitcoin/commit/b5d0cc29ac378f360424472f637f6d6af74660a5: I don't think the `ll` is needed to silence the w-sign-compare warning. I'd go with just `0u`, or if you want to fully specify the type `uint64_t{0}`.
💬 Sjors commented on pull request "ipc: Handle unclean shutdowns better":
(https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-3052127988)
It's still marked as draft though.
(https://github.com/bitcoin/bitcoin/pull/32345#issuecomment-3052127988)
It's still marked as draft though.
💬 fanquake commented on issue "qt: translation related warnings":
(https://github.com/bitcoin/bitcoin/issues/32710#issuecomment-3052248717)
@hebasto If this is an actual issue can it be fixed up? Otherwise, it'd be good if there was a way to avoid this much build log spam. ie:
```bash
CMake Warning at depends/arm64-apple-darwin/native/lib/cmake/Qt6LinguistTools/Qt6LinguistToolsMacros.cmake:418 (message):
Failed to resolve language code for
/root/ci_scratch/src/qt/locale/bitcoin_am.ts. Please update
CFBundleLocalizations in your Info.plist manually.
Call Stack (most recent call first):
depends/arm64-apple-darwin/native/lib/c
...
(https://github.com/bitcoin/bitcoin/issues/32710#issuecomment-3052248717)
@hebasto If this is an actual issue can it be fixed up? Otherwise, it'd be good if there was a way to avoid this much build log spam. ie:
```bash
CMake Warning at depends/arm64-apple-darwin/native/lib/cmake/Qt6LinguistTools/Qt6LinguistToolsMacros.cmake:418 (message):
Failed to resolve language code for
/root/ci_scratch/src/qt/locale/bitcoin_am.ts. Please update
CFBundleLocalizations in your Info.plist manually.
Call Stack (most recent call first):
depends/arm64-apple-darwin/native/lib/c
...
💬 TheCharlatan commented on pull request "Add a "tx output spender" index":
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2194840399)
The way I read it you are currently just checking if the field is set, not if it is set to true. At least providing "return_spending_tx":false in the cli still provides the full transaction.
(https://github.com/bitcoin/bitcoin/pull/24539#discussion_r2194840399)
The way I read it you are currently just checking if the field is set, not if it is set to true. At least providing "return_spending_tx":false in the cli still provides the full transaction.