💬 ismaelsadeeq commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2259731802)
In "[test] check miner doesn't select 0fee transactions"
2778e4691664c55d5699bca2b68f15796e8a9a75
Maybe we should test both ways.
1. When the `blockmintxfee_btc_kvb` is > 0 we verify that we never create 0 or less fee tx.
2. When a 0 or less fee tx is created it is because of `blockmintxfee_btc_kvb` being 0.
```suggestion
for txid in block_template_txids:
for txid in block_template_txids:
fee = node.getmempoolentry(txid)['fees']['base'
...
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2259731802)
In "[test] check miner doesn't select 0fee transactions"
2778e4691664c55d5699bca2b68f15796e8a9a75
Maybe we should test both ways.
1. When the `blockmintxfee_btc_kvb` is > 0 we verify that we never create 0 or less fee tx.
2. When a 0 or less fee tx is created it is because of `blockmintxfee_btc_kvb` being 0.
```suggestion
for txid in block_template_txids:
for txid in block_template_txids:
fee = node.getmempoolentry(txid)['fees']['base'
...
💬 ismaelsadeeq commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2259885809)
In "[test] check bypass of minrelay for various minrelaytxfee settings" d40de30fdf0470a4ec6712aef4d5e8ed40e5ea91
We dont need the cleanup here I think
```suggestion
def test_minrelay_in_package_combos(self)
```
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2259885809)
In "[test] check bypass of minrelay for various minrelaytxfee settings" d40de30fdf0470a4ec6712aef4d5e8ed40e5ea91
We dont need the cleanup here I think
```suggestion
def test_minrelay_in_package_combos(self)
```
👍 ismaelsadeeq approved a pull request: "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee"
(https://github.com/bitcoin/bitcoin/pull/33106#pullrequestreview-3096282456)
Code review ACK a43e1b28b2899e1707e7867fd46efe9fcc08f241
Although it's worth mentioning that there is a scenario were the price could crash to the point where the default min relay allows for free relay or atleast close to free, but a counter argument to that is that even current default is susceptible to such scenario.
> minimum feerate returned by fee estimator: should be done later. In the past, we've excluded new policy defaults from fee estimation until we feel confident they represen
...
(https://github.com/bitcoin/bitcoin/pull/33106#pullrequestreview-3096282456)
Code review ACK a43e1b28b2899e1707e7867fd46efe9fcc08f241
Although it's worth mentioning that there is a scenario were the price could crash to the point where the default min relay allows for free relay or atleast close to free, but a counter argument to that is that even current default is susceptible to such scenario.
> minimum feerate returned by fee estimator: should be done later. In the past, we've excluded new policy defaults from fee estimation until we feel confident they represen
...
💬 ismaelsadeeq commented on pull request "policy: lower the default blockmintxfee, incrementalrelayfee, minrelaytxfee":
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2260507858)
I think `blockmintxfee` should be deprecated and eventually be removed.
What should be in the block template should just depends on the mempool transactions
(https://github.com/bitcoin/bitcoin/pull/33106#discussion_r2260507858)
I think `blockmintxfee` should be deprecated and eventually be removed.
What should be in the block template should just depends on the mempool transactions
💬 maflcko commented on pull request "ci: Migrate CI to hosted Cirrus Runners":
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2260658713)
Would it not be easier for Cirrus to just fix the reverse-age order to pick the latest instead? Or somehow work around this by embedding the epoch seconds in the ccache blob name?
(https://github.com/bitcoin/bitcoin/pull/32989#discussion_r2260658713)
Would it not be easier for Cirrus to just fix the reverse-age order to pick the latest instead? Or somehow work around this by embedding the epoch seconds in the ccache blob name?
💬 hebasto commented on issue "cmake: Errors not actually errors?":
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3164662786)
> Also, the error string CMake is throwing, is also incorrect, as the compiler does support LTO.
CMake doesn't blame the compiler, rather itself: "CMake doesn't support IPO for current compiler".
For some reason, this occurs with the "Unix Makefiles" generator but not with the "Ninja" generator.
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3164662786)
> Also, the error string CMake is throwing, is also incorrect, as the compiler does support LTO.
CMake doesn't blame the compiler, rather itself: "CMake doesn't support IPO for current compiler".
For some reason, this occurs with the "Unix Makefiles" generator but not with the "Ninja" generator.
💬 hebasto commented on issue "cmake: Errors not actually errors?":
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3164664412)
cc @purpleKarrot
(https://github.com/bitcoin/bitcoin/issues/33153#issuecomment-3164664412)
cc @purpleKarrot
💬 maflcko commented on pull request "threading: remove ancient CRITICAL_SECTION macros":
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-3164698282)
This is tagged for 30.0, but feature freeze is in less than two weeks, and it still needs rebase, so it'll likely miss the milestone.
(https://github.com/bitcoin/bitcoin/pull/32592#issuecomment-3164698282)
This is tagged for 30.0, but feature freeze is in less than two weeks, and it still needs rebase, so it'll likely miss the milestone.
💬 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