Bitcoin Core Github
44 subscribers
120K links
Download Telegram
🤔 BrandonOdiwuor reviewed a pull request: "rpc, doc: clarify watch-only wallets balances in RPCHelp"
(https://github.com/bitcoin/bitcoin/pull/32761#pullrequestreview-2974424721)
Code Review ACK cb44680db40c85ba338f2a054511cd68ba6e89ba after reviewing https://github.com/bitcoin/bitcoin/pull/32618
💬 Eunovo commented on pull request "wallet: Remove ISMINE_WATCHONLY and watchonly from RPCs":
(https://github.com/bitcoin/bitcoin/pull/32618#issuecomment-3022945375)
Consider cleaning up `wallet_balance.py` too, like this:
```diff
diff --git a/test/functional/wallet_balance.py b/test/functional/wallet_balance.py
index 4efc0169eb..ac0374c3fa 100755
--- a/test/functional/wallet_balance.py
+++ b/test/functional/wallet_balance.py
@@ -80,9 +80,9 @@ class WalletTest(BitcoinTestFramework):
assert_equal(self.nodes[0].getbalance("*"), 50)
assert_equal(self.nodes[0].getbalance("*", 1), 50)
assert_equal(self.nodes[0].getbalance(minco
...
💬 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
...
📝 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

```
fanquake closed a pull request: "Rename CMake required flags variable in crc32c"
(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.
💬 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.
💬 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.
💬 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.
👍 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.
💬 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
...
👋 hebasto's pull request is ready for review: "cmake: Improve robustness and usability"
(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 :)
👍 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
💬 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.
💬 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.
💬 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.
👍 dergoegge approved a pull request: "fuzz: Make process_message(s) more deterministic"
(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
...
💬 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.