💬 Eunovo commented on pull request "wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs":
(https://github.com/bitcoin/bitcoin/pull/32618#issuecomment-3023002708)
Here is a diff removing `include_watchonly=True` from other functional tests, except `wallet_migration.py`
```diff
diff --git a/test/functional/wallet_importprunedfunds.py b/test/functional/wallet_importprunedfunds.py
index cb052e4511..d45be4aa18 100755
--- a/test/functional/wallet_importprunedfunds.py
+++ b/test/functional/wallet_importprunedfunds.py
@@ -89,7 +89,7 @@ class ImportPrunedFundsTest(BitcoinTestFramework):
wwatch = self.nodes[1].get_wallet_rpc('wwatch')
wwa
...
(https://github.com/bitcoin/bitcoin/pull/32618#issuecomment-3023002708)
Here is a diff removing `include_watchonly=True` from other functional tests, except `wallet_migration.py`
```diff
diff --git a/test/functional/wallet_importprunedfunds.py b/test/functional/wallet_importprunedfunds.py
index cb052e4511..d45be4aa18 100755
--- a/test/functional/wallet_importprunedfunds.py
+++ b/test/functional/wallet_importprunedfunds.py
@@ -89,7 +89,7 @@ class ImportPrunedFundsTest(BitcoinTestFramework):
wwatch = self.nodes[1].get_wallet_rpc('wwatch')
wwa
...
📝 vtjl10 opened a pull request: "Rename CMake required flags variable in crc32c"
(https://github.com/bitcoin/bitcoin/pull/32847)
```markdown
This pull request fixes a typo in the variable name used to restore CMake required flags in `src/crc32c/CMakeLists.txt`
- Corrected `REQURED` → `REQUIRED`
- Ensures the correct variable is referenced when resetting CMake flags
```
(https://github.com/bitcoin/bitcoin/pull/32847)
```markdown
This pull request fixes a typo in the variable name used to restore CMake required flags in `src/crc32c/CMakeLists.txt`
- Corrected `REQURED` → `REQUIRED`
- Ensures the correct variable is referenced when resetting CMake flags
```
✅ fanquake closed a pull request: "Rename CMake required flags variable in crc32c"
(https://github.com/bitcoin/bitcoin/pull/32847)
(https://github.com/bitcoin/bitcoin/pull/32847)
💬 fanquake commented on pull request "Rename CMake required flags variable in crc32c":
(https://github.com/bitcoin/bitcoin/pull/32847#issuecomment-3023047673)
Changes to this subtree should go to https://github.com/bitcoin-core/crc32c-subtree.
(https://github.com/bitcoin/bitcoin/pull/32847#issuecomment-3023047673)
Changes to this subtree should go to https://github.com/bitcoin-core/crc32c-subtree.
💬 bigspider commented on pull request "doc: clarify that the "-j N" goes after the "--build build" part":
(https://github.com/bitcoin/bitcoin/pull/32846#issuecomment-3023135741)
Done, and rebased.
I also noticed that the all the analogous comments were capitalized in the `build-*.md` files, so I capitalized them in `build-unix.md` as well.
(https://github.com/bitcoin/bitcoin/pull/32846#issuecomment-3023135741)
Done, and rebased.
I also noticed that the all the analogous comments were capitalized in the `build-*.md` files, so I capitalized them in `build-unix.md` as well.
💬 hebasto commented on pull request "cmake: Improve robustness and usability":
(https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2177110134)
These custom targets are meaningful and run only within the context of the Guix environment. [Currently](https://github.com/bitcoin/bitcoin/pull/32782#issuecomment-3018586630), no tests are run there.
(https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2177110134)
These custom targets are meaningful and run only within the context of the Guix environment. [Currently](https://github.com/bitcoin/bitcoin/pull/32782#issuecomment-3018586630), no tests are run there.
💬 hebasto commented on pull request "cmake: Improve robustness and usability":
(https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2177114584)
Sure. However, this is no longer relevant following the recent branch update.
(https://github.com/bitcoin/bitcoin/pull/31233#discussion_r2177114584)
Sure. However, this is no longer relevant following the recent branch update.
👍 hebasto approved a pull request: "doc: clarify that the "-j N" goes after the "--build build" part"
(https://github.com/bitcoin/bitcoin/pull/32846#pullrequestreview-2974701234)
ACK 0e9f409db3b7b08aef75ce39765b018b69cc8e9d.
(https://github.com/bitcoin/bitcoin/pull/32846#pullrequestreview-2974701234)
ACK 0e9f409db3b7b08aef75ce39765b018b69cc8e9d.
💬 stickies-v commented on pull request "rpc, test: Fix JSON parsing errors in unloadwallet and getdescriptoractivity RPCs":
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2177179314)
This should be `&&` instead of `||`. And the reason that the functional test didn't fail with this change is an indication that the `.isNull()` check is not really reliable either, because it will happily accept an empty array.
Do we actually need to ensure that at least one such (valid) parameter is provided? If so, we should probably check that `blockindexes_sorted` and `scripts_to_watch` are not both empty. Otherwise, I think just making sure we only execute the `request.params[n].get_arra
...
(https://github.com/bitcoin/bitcoin/pull/32845#discussion_r2177179314)
This should be `&&` instead of `||`. And the reason that the functional test didn't fail with this change is an indication that the `.isNull()` check is not really reliable either, because it will happily accept an empty array.
Do we actually need to ensure that at least one such (valid) parameter is provided? If so, we should probably check that `blockindexes_sorted` and `scripts_to_watch` are not both empty. Otherwise, I think just making sure we only execute the `request.params[n].get_arra
...
👋 hebasto's pull request is ready for review: "cmake: Improve robustness and usability"
(https://github.com/bitcoin/bitcoin/pull/31233)
(https://github.com/bitcoin/bitcoin/pull/31233)
💬 hebasto commented on pull request "cmake: Improve robustness and usability":
(https://github.com/bitcoin/bitcoin/pull/31233#issuecomment-3023332643)
Rebased and undrafted. The PR description has been updated as well.
Friendly ping @maflcko and @purpleKarrot :)
(https://github.com/bitcoin/bitcoin/pull/31233#issuecomment-3023332643)
Rebased and undrafted. The PR description has been updated as well.
Friendly ping @maflcko and @purpleKarrot :)
👍 stickies-v approved a pull request: "doc: clarify that the "-j N" goes after the "--build build" part"
(https://github.com/bitcoin/bitcoin/pull/32846#pullrequestreview-2974825230)
ACK 0e9f409db3b7b08aef75ce39765b018b69cc8e9d
(https://github.com/bitcoin/bitcoin/pull/32846#pullrequestreview-2974825230)
ACK 0e9f409db3b7b08aef75ce39765b018b69cc8e9d
💬 stickies-v commented on pull request "doc: clarify that the "-j N" goes after the "--build build" part":
(https://github.com/bitcoin/bitcoin/pull/32846#discussion_r2177208310)
review note: `gmake` seems fine with placing `-j` earlier, so technically this change isn't necessary, but it's probably better to be consistent anyway.
(https://github.com/bitcoin/bitcoin/pull/32846#discussion_r2177208310)
review note: `gmake` seems fine with placing `-j` earlier, so technically this change isn't necessary, but it's probably better to be consistent anyway.
💬 hebasto commented on pull request "cmake: Improve robustness and usability":
(https://github.com/bitcoin/bitcoin/pull/31233#issuecomment-3023345698)
> Is this PR still an alternative to #31669.
It's related, but I wouldn't consider it an alternative.
(https://github.com/bitcoin/bitcoin/pull/31233#issuecomment-3023345698)
> Is this PR still an alternative to #31669.
It's related, but I wouldn't consider it an alternative.
💬 bigspider commented on pull request "doc: clarify that the "-j N" goes after the "--build build" part":
(https://github.com/bitcoin/bitcoin/pull/32846#discussion_r2177223248)
Good point. Same for `ctest`, I think.
(https://github.com/bitcoin/bitcoin/pull/32846#discussion_r2177223248)
Good point. Same for `ctest`, I think.
👍 dergoegge approved a pull request: "fuzz: Make process_message(s) more deterministic"
(https://github.com/bitcoin/bitcoin/pull/32822#pullrequestreview-2974855838)
utACK fa30966d7d881f5f299a531bd83a666fa15f4563
(https://github.com/bitcoin/bitcoin/pull/32822#pullrequestreview-2974855838)
utACK fa30966d7d881f5f299a531bd83a666fa15f4563
🤔 hebasto reviewed a pull request: "depends: fix libevent `_WIN32_WINNT` usage"
(https://github.com/bitcoin/bitcoin/pull/32837#pullrequestreview-2974868613)
My Guix build:
```
aarch64
70a6898fa1a63cccb6926d319fbd1be2264f19a5763e7064060964581eda25b6 guix-build-336cab98eb04/output/aarch64-linux-gnu/SHA256SUMS.part
2519f262eaf2e96f3060b41c79d90ff01225939503247bae772d851a4eb092af guix-build-336cab98eb04/output/aarch64-linux-gnu/bitcoin-336cab98eb04-aarch64-linux-gnu-debug.tar.gz
13b69188ff681af2e6dabc9fb2c79112993ca7752348224f0776ce0b2e5d1720 guix-build-336cab98eb04/output/aarch64-linux-gnu/bitcoin-336cab98eb04-aarch64-linux-gnu.tar.gz
69b05e8a
...
(https://github.com/bitcoin/bitcoin/pull/32837#pullrequestreview-2974868613)
My Guix build:
```
aarch64
70a6898fa1a63cccb6926d319fbd1be2264f19a5763e7064060964581eda25b6 guix-build-336cab98eb04/output/aarch64-linux-gnu/SHA256SUMS.part
2519f262eaf2e96f3060b41c79d90ff01225939503247bae772d851a4eb092af guix-build-336cab98eb04/output/aarch64-linux-gnu/bitcoin-336cab98eb04-aarch64-linux-gnu-debug.tar.gz
13b69188ff681af2e6dabc9fb2c79112993ca7752348224f0776ce0b2e5d1720 guix-build-336cab98eb04/output/aarch64-linux-gnu/bitcoin-336cab98eb04-aarch64-linux-gnu.tar.gz
69b05e8a
...
💬 purpleKarrot commented on pull request "cmake: Improve robustness and usability":
(https://github.com/bitcoin/bitcoin/pull/31233#issuecomment-3023424198)
ACK 67dc7523f3e103c8359b546d38f28c1feb2b9b34
The issue is specific to python. I leave it up to you and other reviewers whether that should be reworded or not.
(https://github.com/bitcoin/bitcoin/pull/31233#issuecomment-3023424198)
ACK 67dc7523f3e103c8359b546d38f28c1feb2b9b34
The issue is specific to python. I leave it up to you and other reviewers whether that should be reworded or not.
💬 marcofleon commented on pull request "refactor: Convert GenTxid to `std::variant`":
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2177272359)
Took this suggestion. After staring at `txrequest` some more, I agree that it makes more sense conceptually to keep functions that are only used to look up announcements as `uint256`. You're right that there is already a distinction made on master between a `GenTxid` and a `txhash` so I don't see a reason to change that in this PR (other than making the actual `Announcement` type safe).
(https://github.com/bitcoin/bitcoin/pull/32631#discussion_r2177272359)
Took this suggestion. After staring at `txrequest` some more, I agree that it makes more sense conceptually to keep functions that are only used to look up announcements as `uint256`. You're right that there is already a distinction made on master between a `GenTxid` and a `txhash` so I don't see a reason to change that in this PR (other than making the actual `Announcement` type safe).
💬 stickies-v commented on pull request "doc: clarify that the "-j N" goes after the "--build build" part":
(https://github.com/bitcoin/bitcoin/pull/32846#discussion_r2177273789)
Oh you're right!
(https://github.com/bitcoin/bitcoin/pull/32846#discussion_r2177273789)
Oh you're right!