💬 hebasto commented on issue "cmake: Errors not actually errors?":
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3164733222)
It seems that the `$<TARGET_OBJECTS:..>` added to `bitcoinkernel` sources in #33077 are causing the issue.
cc @theuni
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3164733222)
It seems that the `$<TARGET_OBJECTS:..>` added to `bitcoinkernel` sources in #33077 are causing the issue.
cc @theuni
🤔 pinheadmz reviewed a pull request: "Introduce SockMan ("lite"): low-level socket handling for HTTP"
(https://github.com/bitcoin/bitcoin/pull/32747#pullrequestreview-3097669539)
3e7abceecf -> 598bee6bd5
Address nits from @jonatack including commit messages that indicate code is copied from CConnman and will be modernized in following commits.
(https://github.com/bitcoin/bitcoin/pull/32747#pullrequestreview-3097669539)
3e7abceecf -> 598bee6bd5
Address nits from @jonatack including commit messages that indicate code is copied from CConnman and will be modernized in following commits.
💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2260675972)
fixed, thanks
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2260675972)
fixed, thanks
💬 pinheadmz commented on pull request "Introduce SockMan ("lite"): low-level socket handling for HTTP":
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2260702590)
Thanks, removed the years entirely which seems to be the style for new files.
(https://github.com/bitcoin/bitcoin/pull/32747#discussion_r2260702590)
Thanks, removed the years entirely which seems to be the style for new files.
💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-3164755376)
@tdb3 @hodlinator @Sjors I'm hoping to turn your concept ACKs into reviews on the stripped-down version of this PR: https://github.com/bitcoin/bitcoin/pull/32747
(https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-3164755376)
@tdb3 @hodlinator @Sjors I'm hoping to turn your concept ACKs into reviews on the stripped-down version of this PR: https://github.com/bitcoin/bitcoin/pull/32747
👍 ryanofsky approved a pull request: "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet"
(https://github.com/bitcoin/bitcoin/pull/32489#pullrequestreview-3094757593)
Code review ACK 84d3bc68cc4d23ab9f7a91a955cd0e26ffb1b6e4. This is great. Nice feature cleanly implemented with very good test coverage. I left a bunch of comments but none are critical.
(https://github.com/bitcoin/bitcoin/pull/32489#pullrequestreview-3094757593)
Code review ACK 84d3bc68cc4d23ab9f7a91a955cd0e26ffb1b6e4. This is great. Nice feature cleanly implemented with very good test coverage. I left a bunch of comments but none are critical.
💬 ryanofsky commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2260608137)
In commit "wallet: Add CWallet::ExportWatchOnly" (236970d25cc45b02678e410cfdc7e064da22cf11)
No error is returned here if this fails. If I test with an unwritable path like `bitcoin-cli exportwatchonlywallet /foo` I see the RPC return success, but no backup file is created and `SQLite Error. Code: 14. Message: os_unix.c:44906: (2) open(/foo) - No such file or directory` is in the debug log.
Would be good to fix, and may also want to add `[[nodiscard]]` to the BackupWallet method to prevent
...
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2260608137)
In commit "wallet: Add CWallet::ExportWatchOnly" (236970d25cc45b02678e410cfdc7e064da22cf11)
No error is returned here if this fails. If I test with an unwritable path like `bitcoin-cli exportwatchonlywallet /foo` I see the RPC return success, but no backup file is created and `SQLite Error. Code: 14. Message: os_unix.c:44906: (2) open(/foo) - No such file or directory` is in the debug log.
Would be good to fix, and may also want to add `[[nodiscard]]` to the BackupWallet method to prevent
...
💬 ryanofsky commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2260598786)
In commit "wallet: Add CWallet::ExportWatchOnly" (236970d25cc45b02678e410cfdc7e064da22cf11)
Calling `fs::canonical` on the destination parent path seems like a nice way of making sure the parent path exists and throwing an error early if it doesn't. But it doesn't handle other problems like the destination path not being writable. Also I'm wary of code that manipulates paths unnecessarily instead of passing them directly to the OS.
Not critical, but I'd suggest a change like the following
...
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2260598786)
In commit "wallet: Add CWallet::ExportWatchOnly" (236970d25cc45b02678e410cfdc7e064da22cf11)
Calling `fs::canonical` on the destination parent path seems like a nice way of making sure the parent path exists and throwing an error early if it doesn't. But it doesn't handle other problems like the destination path not being writable. Also I'm wary of code that manipulates paths unnecessarily instead of passing them directly to the OS.
Not critical, but I'd suggest a change like the following
...
💬 ryanofsky commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2260646957)
In commit "wallet: Add CWallet::ExportWatchOnly" (236970d25cc45b02678e410cfdc7e064da22cf11)
I don’t think fs::remove_all is appropriate here or elsewhere in wallet code. We should have a wallet delete function that explicitly removes known database and log files, then removes the directory, rather than recursively deleting everything. remove_all is fine in the happy case, but if there’s a bug (e.g. an extra parent_path call) or if users mistakenly store other data or bind mounts in the wallet
...
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2260646957)
In commit "wallet: Add CWallet::ExportWatchOnly" (236970d25cc45b02678e410cfdc7e064da22cf11)
I don’t think fs::remove_all is appropriate here or elsewhere in wallet code. We should have a wallet delete function that explicitly removes known database and log files, then removes the directory, rather than recursively deleting everything. remove_all is fine in the happy case, but if there’s a bug (e.g. an extra parent_path call) or if users mistakenly store other data or bind mounts in the wallet
...
💬 ryanofsky commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2258638700)
In commit "wallet: Move listdescriptors retrieving from RPC to CWallet" (3c5cdae7a15cb046010d753b918b313423861582)
Since these are errors are pretty technical and obscure, seems like it probably makes sense to use `Untranslated` instead of `_` to avoid these becoming translated strings.
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2258638700)
In commit "wallet: Move listdescriptors retrieving from RPC to CWallet" (3c5cdae7a15cb046010d753b918b313423861582)
Since these are errors are pretty technical and obscure, seems like it probably makes sense to use `Untranslated` instead of `_` to avoid these becoming translated strings.
💬 ryanofsky commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2260690524)
In commit "wallet, rpc: Add exportwatchonlywallet RPC" (71e25ee9ad52d8b9a763782b795690a196f218ef)
Seems like a mistake here, did you mean to write `"\"home\\user\\backup.dat\""`?
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2260690524)
In commit "wallet, rpc: Add exportwatchonlywallet RPC" (71e25ee9ad52d8b9a763782b795690a196f218ef)
Seems like a mistake here, did you mean to write `"\"home\\user\\backup.dat\""`?
💬 ryanofsky commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2260733624)
In commit "test: Test for exportwatchonlywallet" (84d3bc68cc4d23ab9f7a91a955cd0e26ffb1b6e4)
This chunk of code is repeated many times. Maybe factor out into an export_and_restore help method
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2260733624)
In commit "test: Test for exportwatchonlywallet" (84d3bc68cc4d23ab9f7a91a955cd0e26ffb1b6e4)
This chunk of code is repeated many times. Maybe factor out into an export_and_restore help method
💬 ryanofsky commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2258663429)
re: https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2230275124
In commit "wallet: Write new descriptor's cache in AddWalletDescriptor" (fcf7dfe08519aefd193d12958070bfc5ea52f090)
Can confirm if I revert this change I see a `Wallet corrupted (-4)
` error from `self.online.restorewallet("imports_watchonly", res["exported_file"])` running the test with `Error: Unable to expand wallet descriptor from cache` in the logs
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2258663429)
re: https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2230275124
In commit "wallet: Write new descriptor's cache in AddWalletDescriptor" (fcf7dfe08519aefd193d12958070bfc5ea52f090)
Can confirm if I revert this change I see a `Wallet corrupted (-4)
` error from `self.online.restorewallet("imports_watchonly", res["exported_file"])` running the test with `Error: Unable to expand wallet descriptor from cache` in the logs
💬 ryanofsky commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2258660270)
re: https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2101952111
In commit "wallet: Use Descriptor::CanSelfExpand() in CanGetAddresses()" (1170ade3a6d1fbb50ee2a83b906bb83560b0484d)
Can confirm without this change the test fails as expected with `self.funds.sendtoaddress(online_wallet.getnewaddress(), 10)` throwing "No addresses available"
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2258660270)
re: https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2101952111
In commit "wallet: Use Descriptor::CanSelfExpand() in CanGetAddresses()" (1170ade3a6d1fbb50ee2a83b906bb83560b0484d)
Can confirm without this change the test fails as expected with `self.funds.sendtoaddress(online_wallet.getnewaddress(), 10)` throwing "No addresses available"
💬 ryanofsky commented on pull request "wallet: Add `exportwatchonlywallet` RPC to export a watchonly version of a wallet":
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2260742888)
In commit "test: Test for exportwatchonlywallet" (84d3bc68cc4d23ab9f7a91a955cd0e26ffb1b6e4)
Does this need a sync_mempools like the other case below to make sure offline wallet receives the funds?
Also would be nice here and other places to replace 0, 1 with self.online.index and self.offline.index
(https://github.com/bitcoin/bitcoin/pull/32489#discussion_r2260742888)
In commit "test: Test for exportwatchonlywallet" (84d3bc68cc4d23ab9f7a91a955cd0e26ffb1b6e4)
Does this need a sync_mempools like the other case below to make sure offline wallet receives the funds?
Also would be nice here and other places to replace 0, 1 with self.online.index and self.offline.index
💬 D33r-Gee commented on pull request "Interfaces: Expose UTXO Snapshot Loading and Add Progress Notifications":
(https://github.com/bitcoin/bitcoin/pull/33117#issuecomment-3164996064)
with [8bc4035](https://github.com/bitcoin/bitcoin/commit/8bc4035570acfe512c756c6de30d6440d322d514) it has been rebased and the fuzzing CI error has been addressed
(https://github.com/bitcoin/bitcoin/pull/33117#issuecomment-3164996064)
with [8bc4035](https://github.com/bitcoin/bitcoin/commit/8bc4035570acfe512c756c6de30d6440d322d514) it has been rebased and the fuzzing CI error has been addressed
🤔 glozow reviewed a pull request: "[29.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33074#pullrequestreview-3097984714)
ACK b9e637bd0ee4d1da5f587ec33cbed9ee28c07daf
Compared the backport, didn't test
(https://github.com/bitcoin/bitcoin/pull/33074#pullrequestreview-3097984714)
ACK b9e637bd0ee4d1da5f587ec33cbed9ee28c07daf
Compared the backport, didn't test
💬 l0rinc commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#issuecomment-3165029728)
> it has internal noise higher than the savings here
Sure, but I'm just running them a lot of times, the trends are obvious this way.
> will run the same for the full test suite
I have measured running the tests with the 3 setups with 1-6x the nproc for parallelism.
The 3 solutions all scale well beyond the number of cpus (I'm now running the same until 8x on a different platform to confirm the findings).
<details>
<summary>Details</summary>
```
COMMITS="d767503b6a2618e0c99407
...
(https://github.com/bitcoin/bitcoin/pull/33141#issuecomment-3165029728)
> it has internal noise higher than the savings here
Sure, but I'm just running them a lot of times, the trends are obvious this way.
> will run the same for the full test suite
I have measured running the tests with the 3 setups with 1-6x the nproc for parallelism.
The 3 solutions all scale well beyond the number of cpus (I'm now running the same until 8x on a different platform to confirm the findings).
<details>
<summary>Details</summary>
```
COMMITS="d767503b6a2618e0c99407
...
💬 maflcko commented on pull request "test: Remove polling loop from test_runner (take 2)":
(https://github.com/bitcoin/bitcoin/pull/33141#issuecomment-3165069155)
> Which indicates to me that the new solution scales a bit better - but since we haven't even hit the bottom yet (surprisingly), I'll post those in a follow-up.
Thanks for the benchmarks, but honestly, I can't see that the new solution scales better. You only did 3 runs for each point, and the error bars overlap with the other points each time. Also the hyperfine output has warnings about outliers. I guess you'd have to run at least 100 times for each data point (not saying you should :sweat_
...
(https://github.com/bitcoin/bitcoin/pull/33141#issuecomment-3165069155)
> Which indicates to me that the new solution scales a bit better - but since we haven't even hit the bottom yet (surprisingly), I'll post those in a follow-up.
Thanks for the benchmarks, but honestly, I can't see that the new solution scales better. You only did 3 runs for each point, and the error bars overlap with the other points each time. Also the hyperfine output has warnings about outliers. I guess you'd have to run at least 100 times for each data point (not saying you should :sweat_
...
💬 pythcoiner commented on pull request "wallet: support bip388 policy with external signer":
(https://github.com/bitcoin/bitcoin/pull/33008#issuecomment-3165080503)
> At least in the case of Ledger (haven't tested the others) this requires us to hold on to an hmac, which proves to the device that the user previously reviewed and approved it.
In the range of devices supporting miniscript, only Ledger requires the software to store the Proof Of Registration, other devices stores it internally(SpecterDiy, BitBox, Coldcard, Jade) or not at all (Krux).
(https://github.com/bitcoin/bitcoin/pull/33008#issuecomment-3165080503)
> At least in the case of Ledger (haven't tested the others) this requires us to hold on to an hmac, which proves to the device that the user previously reviewed and approved it.
In the range of devices supporting miniscript, only Ledger requires the software to store the Proof Of Registration, other devices stores it internally(SpecterDiy, BitBox, Coldcard, Jade) or not at all (Krux).