💬 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.
💬 fanquake commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#issuecomment-3031968877)
cc @purpleKarrot
(https://github.com/bitcoin/bitcoin/pull/32865#issuecomment-3031968877)
cc @purpleKarrot
💬 hebasto commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2182601204)
All warnings were intentionally printed just after the summary to draw the user's attention to them.
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2182601204)
All warnings were intentionally printed just after the summary to draw the user's attention to them.
📝 rkrux opened a pull request: "doc: add note for watch-only wallet migration"
(https://github.com/bitcoin/bitcoin/pull/32866)
This was suggested in a previous PR #31423.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
*
...
(https://github.com/bitcoin/bitcoin/pull/32866)
This was suggested in a previous PR #31423.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
*
...
💬 rkrux commented on pull request "wallet: migration, avoid creating spendable wallet from a watch-only legacy wallet":
(https://github.com/bitcoin/bitcoin/pull/31423#issuecomment-3031999844)
I did the TODO mentioned in the PR description in PR #32866.
(https://github.com/bitcoin/bitcoin/pull/31423#issuecomment-3031999844)
I did the TODO mentioned in the PR description in PR #32866.
💬 instagibbs commented on issue "Notes on Compact Block getdata fallback responses":
(https://github.com/bitcoin/bitcoin/issues/13370#issuecomment-3032029389)
Is it a protocol violation, or otherwise dangerous, to just serve the block you advertised even though you later found it was invalid?
(https://github.com/bitcoin/bitcoin/issues/13370#issuecomment-3032029389)
Is it a protocol violation, or otherwise dangerous, to just serve the block you advertised even though you later found it was invalid?
📝 rkrux opened a pull request: "doc: mention key removal in rpc interface modification"
(https://github.com/bitcoin/bitcoin/pull/32867)
A discussion in a previous PR 32618 prompted me to add this note.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experienc
...
(https://github.com/bitcoin/bitcoin/pull/32867)
A discussion in a previous PR 32618 prompted me to add this note.
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experienc
...
💬 rkrux commented on pull request "doc: mention key removal in rpc interface modification":
(https://github.com/bitcoin/bitcoin/pull/32867#issuecomment-3032050281)
I want to mention about how long is the deprecation/grace period generally but I don't know right now. Suggestions welcome.
(https://github.com/bitcoin/bitcoin/pull/32867#issuecomment-3032050281)
I want to mention about how long is the deprecation/grace period generally but I don't know right now. Suggestions welcome.
📝 theStack opened a pull request: "test: refactor: overhaul block hash determination for `CBlock{,Header}` objects"
(https://github.com/bitcoin/bitcoin/pull/32868)
Similar to what #32421 did for `CTransaction` instances, this PR aims to improve the block hash determination of `CBlockHeader`/`CBlock` (the latter is a subclass of the former) instances by removing the block header caching mechanism and introducing consistent naming. Without the statefulness, sneaky testing bugs like #32742 and #32823 are less likely to happen in the future. Note that performance is even less of an issue here compared to `CTransaction`, as we only need to hash 80 bytes, which
...
(https://github.com/bitcoin/bitcoin/pull/32868)
Similar to what #32421 did for `CTransaction` instances, this PR aims to improve the block hash determination of `CBlockHeader`/`CBlock` (the latter is a subclass of the former) instances by removing the block header caching mechanism and introducing consistent naming. Without the statefulness, sneaky testing bugs like #32742 and #32823 are less likely to happen in the future. Note that performance is even less of an issue here compared to `CTransaction`, as we only need to hash 80 bytes, which
...
🤔 hebasto reviewed a pull request: "ci: update pwsh to use custom shell that fails-fast"
(https://github.com/bitcoin/bitcoin/pull/32672#pullrequestreview-2983019444)
Left a couple of notes regarding the current implementation:
1. The output in the "Get tool information" step has changed:
```diff
--- old 2025-07-03 13:37:57.062194420 +0100
+++ new 2025-07-03 13:35:57.800242845 +0100
@@ -1,3 +1,15 @@
+
+Name Value
+---- -----
+PSVersion 7.4.10
+PSEdition Core
+GitCommitId 7.4.10
+OS Microsoft Windows 10.0.20
...
(https://github.com/bitcoin/bitcoin/pull/32672#pullrequestreview-2983019444)
Left a couple of notes regarding the current implementation:
1. The output in the "Get tool information" step has changed:
```diff
--- old 2025-07-03 13:37:57.062194420 +0100
+++ new 2025-07-03 13:35:57.800242845 +0100
@@ -1,3 +1,15 @@
+
+Name Value
+---- -----
+PSVersion 7.4.10
+PSEdition Core
+GitCommitId 7.4.10
+OS Microsoft Windows 10.0.20
...
💬 fanquake commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2182696708)
I've added `.github/ci-test-each-commit-exec.py` & `.github/workflows/ci.yml`. I'll add a commit for Guix once the build passes.
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2182696708)
I've added `.github/ci-test-each-commit-exec.py` & `.github/workflows/ci.yml`. I'll add a commit for Guix once the build passes.
💬 Sjors commented on pull request "wallet: allow skipping script paths":
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3032145576)
The `walletprocesspsbt` RPC now also has this option. Added test coverage by expanding `wallet_taproot.py`.
(https://github.com/bitcoin/bitcoin/pull/32857#issuecomment-3032145576)
The `walletprocesspsbt` RPC now also has this option. Added test coverage by expanding `wallet_taproot.py`.
💬 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_r2182726209)
Unfortunate that it can only be resolved by marking the function as `NOLINT`. Addressed.
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2182726209)
Unfortunate that it can only be resolved by marking the function as `NOLINT`. Addressed.
💬 maflcko commented on pull request "rpc: use CScheduler for HTTPRPCTimer":
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2182756040)
Just for additional reference, the check is already sometimes failing on current master: https://github.com/bitcoin/bitcoin/issues/30797#issue-2503043392
(https://github.com/bitcoin/bitcoin/pull/32796#discussion_r2182756040)
Just for additional reference, the check is already sometimes failing on current master: https://github.com/bitcoin/bitcoin/issues/30797#issue-2503043392
💬 hebasto commented on pull request "cmake: Create subdirectories in build tree in advance":
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3032234676)
Rebased.
@janb84
> Any hints how to verify / test this ?
>
> Have tried to see any difference between this PR and master but it's not clear to me what difference I should see. When used on #32697 there is a difference that without this PR `build/test/functional/data/util/*` is populated with this PR the util directory is not being created (looking at the source this is logical but unsure if there needs to be things done after 32697 is merged or not having the directory is intentional )
...
(https://github.com/bitcoin/bitcoin/pull/32773#issuecomment-3032234676)
Rebased.
@janb84
> Any hints how to verify / test this ?
>
> Have tried to see any difference between this PR and master but it's not clear to me what difference I should see. When used on #32697 there is a difference that without this PR `build/test/functional/data/util/*` is populated with this PR the util directory is not being created (looking at the source this is logical but unsure if there needs to be things done after 32697 is merged or not having the directory is intentional )
...
💬 pinheadmz commented on issue "intermittent issue in wallet_keypool.py: assert_equal(nodes[0].getwalletinfo()["unlocked_until"], 0) AssertionError: not(1725366101 == 0)":
(https://github.com/bitcoin/bitcoin/issues/30797#issuecomment-3032235787)
Oh ha! I should've checked closed issues before opening #32793. I didn't see any other recent failures outside my own PR so I suspected it was just me.
(https://github.com/bitcoin/bitcoin/issues/30797#issuecomment-3032235787)
Oh ha! I should've checked closed issues before opening #32793. I didn't see any other recent failures outside my own PR so I suspected it was just me.
💬 fanquake commented on pull request "cmake: Use `AUTHOR_WARNING` for warnings":
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2182775275)
Added a commit for Guix as well.
(https://github.com/bitcoin/bitcoin/pull/32865#discussion_r2182775275)
Added a commit for Guix as well.