💬 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.
(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.
(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.
(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.
(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.
(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.
(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
(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.
(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
...
(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
...
(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.
(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.
💬 pinheadmz commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2931233423)
> This PR will not queue new private broadcast connections if the same tx is submitted again.
Where is this implemented in the PR?
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2931233423)
> This PR will not queue new private broadcast connections if the same tx is submitted again.
Where is this implemented in the PR?
🤔 maflcko reviewed a pull request: "ci, iwyu: Treat warnings as errors for specific directories"
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-2888771192)
review ACK e9143a68f34f350d7fa4405389222da4dfa9a394 🎰
<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+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK e9143a68f34f
...
(https://github.com/bitcoin/bitcoin/pull/31308#pullrequestreview-2888771192)
review ACK e9143a68f34f350d7fa4405389222da4dfa9a394 🎰
<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+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK e9143a68f34f
...
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121478117)
5cd4bbb67c3c7e651b3d83c77ffb936f60a2c496: Looks wrong. This should be a fwd dcl
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121478117)
5cd4bbb67c3c7e651b3d83c77ffb936f60a2c496: Looks wrong. This should be a fwd dcl
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121385023)
nit in the first commit: Seems odd to exclude this without explanation in the code. Would an alternative be:
```diff
diff --git a/ci/test/03_test_script.sh b/ci/test/03_test_script.sh
index 3394c50b8b..db3d3d64de 100755
--- a/ci/test/03_test_script.sh
+++ b/ci/test/03_test_script.sh
@@ -33,7 +33,9 @@ echo "=== BEGIN env ==="
env
echo "=== END env ==="
-(
+if [ "$RUN_TIDY" != "true" ]; then
+ # The tidy task has no UB detector, so skip the patch there to avoid issues
+ # late
...
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121385023)
nit in the first commit: Seems odd to exclude this without explanation in the code. Would an alternative be:
```diff
diff --git a/ci/test/03_test_script.sh b/ci/test/03_test_script.sh
index 3394c50b8b..db3d3d64de 100755
--- a/ci/test/03_test_script.sh
+++ b/ci/test/03_test_script.sh
@@ -33,7 +33,9 @@ echo "=== BEGIN env ==="
env
echo "=== END env ==="
-(
+if [ "$RUN_TIDY" != "true" ]; then
+ # The tidy task has no UB detector, so skip the patch there to avoid issues
+ # late
...
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121492797)
nit in eb4be21cc0bb93b0931a61b2af342946f09a9721: Wrong section and should be cstring to avoid confusing with string.h in our repo?
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121492797)
nit in eb4be21cc0bb93b0931a61b2af342946f09a9721: Wrong section and should be cstring to avoid confusing with string.h in our repo?
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121493309)
same...
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121493309)
same...
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121507603)
e9143a68f34f350d7fa4405389222da4dfa9a394: Why not just `git diff --no-pager --exit-code`?
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121507603)
e9143a68f34f350d7fa4405389222da4dfa9a394: Why not just `git diff --no-pager --exit-code`?
💬 maflcko commented on pull request "ci, iwyu: Treat warnings as errors for specific directories":
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121387189)
q in the first commit: This is no longer needed, because cmake uses absolute paths in the compile commands?
(https://github.com/bitcoin/bitcoin/pull/31308#discussion_r2121387189)
q in the first commit: This is no longer needed, because cmake uses absolute paths in the compile commands?
💬 andrewtoth commented on pull request "Broadcast own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2931272929)
@pinheadmz in [ScheduleTxForPrivateBroadcast](https://github.com/bitcoin/bitcoin/pull/29415/files#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R2295-R2313) we check if the tx was inserted into the private broadcast queue, and only then do we queue more private broadcast connections to open. The tx is removed from the private broadcast queue when we receive the tx in our mempool [here](https://github.com/bitcoin/bitcoin/pull/29415/files#diff-6875de769e90cec84d2e8a9c1b962cd
...
(https://github.com/bitcoin/bitcoin/pull/29415#issuecomment-2931272929)
@pinheadmz in [ScheduleTxForPrivateBroadcast](https://github.com/bitcoin/bitcoin/pull/29415/files#diff-6875de769e90cec84d2e8a9c1b962cdbcda44d870d42e4215827e599e11e90e3R2295-R2313) we check if the tx was inserted into the private broadcast queue, and only then do we queue more private broadcast connections to open. The tx is removed from the private broadcast queue when we receive the tx in our mempool [here](https://github.com/bitcoin/bitcoin/pull/29415/files#diff-6875de769e90cec84d2e8a9c1b962cd
...