💬 hebasto commented on issue "windows: depends config fails":
(https://github.com/bitcoin/bitcoin/issues/32578#issuecomment-2931010856)
> Download failed, halting portfile.

means some network issues.

If it is not intermittent, please report the issue [upstream](https://github.com/microsoft/vcpkg).

💬 brunoerg commented on pull request "Fuzz: extend CConnman tests":
(https://github.com/bitcoin/bitcoin/pull/28584#discussion_r2121335303)
d790b83996ae0d0bc4fe9723606c8c923cec1bcc: nit: It seems the `ConsumeServiceVector` is always called with the `max_vector_size` being 5? Maybe it could be the default value since it's only used here.

👍 brunoerg approved a pull request: "Fuzz: extend CConnman tests"
(https://github.com/bitcoin/bitcoin/pull/28584#pullrequestreview-2888696559)
reACK 582d9e3dbdf5b64272b65844d679942c5aca643f

Just generated this coverage report with some hours of running: https://brunoerg.xyz/bitcoin-core-coverage/28584/coverage_report/

💬 mzumsande commented on pull request "p2p: Add mutation check inside compact block's FillBlock":
(https://github.com/bitcoin/bitcoin/pull/32646#discussion_r2121338410)
For the sake of disconnecting the peer? I think we could use the flags set or not set in the block to learn which mutation check (`CheckMerkleRoot()` or `CheckWitnessMalleation()`) has failed.

👍 l0rinc approved a pull request: "rpc: Use type-safe HelpException"
(https://github.com/bitcoin/bitcoin/pull/32660#pullrequestreview-2888696257)
code review ACK fa0cf42380dec17e73d38aa69dd70662a0bc344a

💬 l0rinc commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#discussion_r2121346010)
👍, alternatively, if you think it's clearer:
```suggestion
for (const auto& pcmd : vCommands | std::views::values) {
```

💬 l0rinc commented on pull request "rpc: Use type-safe HelpException":
(https://github.com/bitcoin/bitcoin/pull/32660#discussion_r2121336417)
nits:
* formatting is usually different for classes/structs - but maybe this style was chosen to align with the rest of the file
* can be `final`
```suggestion
struct HelpException final : std::runtime_error
{
explicit HelpException(const std::string& msg) : std::runtime_error{msg} {}
};
```

🤔 rkrux reviewed a pull request: "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC"
(https://github.com/bitcoin/bitcoin/pull/32593#pullrequestreview-2888425586)
> Keeping track of whether a locked coin was persisted is also useful
information for future PRs.

Is the only benefit that while exporting watch only version of the wallet, the code need not read the locked coin from the disk and instead can read from the `m_locked_coins` map?

https://github.com/bitcoin/bitcoin/pull/32489/files#diff-1f2db0e4d5c12d109c7f0962333c245b49b696cb39ff432da048e9d6c08944d8R4604-R4608

Earlier I had few concerns regarding the persistence of the locked coins incase
...

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121257035)
Should it not check the `persistent` boolean parameter of the locked coin just like it's checked in `UnlockCoin`? Otherwise, it tries to erase a locked coin that was not persisted to disk.

Scenario: I lock a coin by calling `lockunspent` RPC with `unlock: false` and `persistent: false`. I then try to unlock all coins by calling `lockunspent` RPC with `unlock: true` and letting the `persistent` param being null.
Though the RPC still returns `true` probably because of the SQLITE's design: htt
...

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121160285)
This looks better to me from readability POV. Earlier it seemed to "lock coin" again right after loading the locked coin and didn't write it again in the disk based on the DB batch not being passed in the second argument that was not intuitive while reading.

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121260886)
Though it was present in previous code as well, I find the usage of bitwise `and` in boolean operations undesirable.

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121285665)
This is the only place where I find the usage of `LoadLockedCoin` unusual because it comes prior to actually locking the coin in disk. But seems unavoidable because persisting in disk is conditional.

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121271089)
This is much cleaner! This PR's intent seems fine to me as it does indeed improve code readability.

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121204919)
I see the previous code also needed to figure out whether the locked coin was persisted on disk while unlocking, but it did so in a slightly complicated way as it depended on whether the DB batch was passed in the unlock flow. Theoretically it could be possible that the code forgets to pass the said batch in the unlock flow leading to unintended scenarios.
The new code seems more robust to me as it explicitly tracks whether the coin was persisted on disk.

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121359136)
Unrelated: I find the `lockunspent` RPC request params unusual though. The second arg is called `transactions` but expects transaction outputs. The RPC is called `lockunspent` but the first arg is called `unlock` that leads me to straightaway do mental negation calculation while calling the RPC that seems undesirable to me.

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121323995)
The (un)lockCoin functions maintain their own db batches now. I thought it would have led to a decoupling of database read/write operations from the callers but I don't think that's the case because of "each read and write being its own transaction" already unless `TxBegin/Commit()` is used, which is not the case here.

https://github.com/bitcoin/bitcoin/blob/1c6602399be6de0148938a3fd7b51e6c48b985d2/src/wallet/walletdb.h#L182-L185

💬 rkrux commented on pull request "wallet, rpc: Move (Un)LockCoin WalletBatch creation out of RPC":
(https://github.com/bitcoin/bitcoin/pull/32593#discussion_r2121331765)
This is fine because earlier `LockCoin` didn't pass a DB batch here that meant not to persist.

💬 pinheadmz commented on pull request "[28.x] Backport #31407":
(https://github.com/bitcoin/bitcoin/pull/32563#issuecomment-2931222187)
I think all macos signing is working now.

Detached sigs for this commit: https://github.com/pinheadmz/bitcoin-detached-sigs/tree/fanquake-backport_codesigning-b1f694fce2

Tested signed binaries on macos/arm64:

```
--> ./bitcoind --version
Bitcoin Core version v28.2.0rc1
Copyright (C) 2009-2025 The Bitcoin Core developers
```

<img width="592" alt="Screenshot 2025-06-02 at 10 13 38 AM" src="https://github.com/user-attachments/assets/9fae02f8-348e-4824-85d4-e21c21b36c1d" />


code
...

👍 pinheadmz approved a pull request: "[28.x] Backport #31407"
(https://github.com/bitcoin/bitcoin/pull/32563#pullrequestreview-2888909946)
ACK b1f694fce276d68a5b983c187a4efbb231d83f79


<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK b1f694fce276d68a5b983c187a4efbb231d83f79
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmg9wI8ACgkQ5+KYS2KJ
yTqh2g//VaBp+xI3eip80BQVqdSq0zaeMnvE+n92xXkb2FQANL7PR+qvxjYVU0KA
+dQ5yqJZH4QZQA4WnZhsM9ZQAzWQ+BKCeoQGyzw13YUU3vX7qBxqJxFeXgOb5b3a
EeDW6EKhPTdtekHTaHhyqlBklL6RXQpepMVbp3CAZsUBpzlRz/8r0q7oZTqzHS8Z
UADA
...

💬 fanquake commented on pull request "contrib: turn off compression of macOS SDK to fix determinism (across distros)":
(https://github.com/bitcoin/bitcoin/pull/32009#issuecomment-2931231983)
Rebased and added a change to skip more files to the first commit, which reduces the final uncompressed size from ~230mb to ~157mb.