💬 achow101 commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1446448724)
I think UniValue type interpretation would be fine.
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1446448724)
I think UniValue type interpretation would be fine.
📝 brunoerg opened a pull request: "fuzz: fix `connman` initialization"
(https://github.com/bitcoin/bitcoin/pull/29211)
Fixes https://github.com/bitcoin/bitcoin/pull/29172#issuecomment-1883547121
(https://github.com/bitcoin/bitcoin/pull/29211)
Fixes https://github.com/bitcoin/bitcoin/pull/29172#issuecomment-1883547121
💬 brunoerg commented on pull request "fuzz: fix `connman` initialization":
(https://github.com/bitcoin/bitcoin/pull/29211#issuecomment-1883603293)
cc: @achow101
(https://github.com/bitcoin/bitcoin/pull/29211#issuecomment-1883603293)
cc: @achow101
💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1883611431)
https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1855815358 exists, still, as well.
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1883611431)
https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1855815358 exists, still, as well.
💬 maflcko commented on issue "fuzz: Fix timeouts":
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1883614535)
[clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt](https://github.com/bitcoin/bitcoin/files/13877884/clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt)
(https://github.com/bitcoin/bitcoin/issues/28812#issuecomment-1883614535)
[clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt](https://github.com/bitcoin/bitcoin/files/13877884/clusterfuzz-testcase-mocked_descriptor_parse-5224675118481408.bin.not.txt)
💬 jamesob commented on pull request "logging: Simplify API for level based logging":
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1446491162)
> Given that LogInfo is unconditional, what would be your use case for adding a log category?
Simply that "info = unconditional" might not always be the case (and is an unusual convention to have hardcoded), and a user could rightly want to see only WARNING+ logs as well as, say, `net` or validation-related INFOs.
(https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1446491162)
> Given that LogInfo is unconditional, what would be your use case for adding a log category?
Simply that "info = unconditional" might not always be the case (and is an unusual convention to have hardcoded), and a user could rightly want to see only WARNING+ logs as well as, say, `net` or validation-related INFOs.
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1445260009)
In commit "desc spkm: Add functions to retrieve specific private keys" (ebd02d73241c0922f486cdf14719281c079b45c7)
Maybe add a comment saying it returns nullopt if the key doesn't exist, or can't be decrypted because the wallet is locked, or because there's decryption error. Otherwise it's not clear what the function is assuming or when it returns nullopt.
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1445260009)
In commit "desc spkm: Add functions to retrieve specific private keys" (ebd02d73241c0922f486cdf14719281c079b45c7)
Maybe add a comment saying it returns nullopt if the key doesn't exist, or can't be decrypted because the wallet is locked, or because there's decryption error. Otherwise it's not clear what the function is assuming or when it returns nullopt.
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1445262239)
In commit "wallet, rpc: Add gethdkeys RPC" (cbe990b1a95e6331106c3ce6d1f2abd860f88ea8)
I think this example is not right anymore since the parameter needs to be named. There seems to be a HelpExampleCliNamed function for this.
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1445262239)
In commit "wallet, rpc: Add gethdkeys RPC" (cbe990b1a95e6331106c3ce6d1f2abd860f88ea8)
I think this example is not right anymore since the parameter needs to be named. There seems to be a HelpExampleCliNamed function for this.
👍 ryanofsky approved a pull request: "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors"
(https://github.com/bitcoin/bitcoin/pull/29130#pullrequestreview-1809920961)
Code review ACK c10150b6150083440af4f0aa1110c8aa99ba2dc8. Left a lot of suggestions, but none are important. Overall this looks great and is very cleanly implemented.
(https://github.com/bitcoin/bitcoin/pull/29130#pullrequestreview-1809920961)
Code review ACK c10150b6150083440af4f0aa1110c8aa99ba2dc8. Left a lot of suggestions, but none are important. Overall this looks great and is very cleanly implemented.
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446462443)
In commit "wallet: Add GetActiveHDPubKeys to retrieve xpubs from active descriptors" (2621ca0e6418b406161b21b6afac159b0be775b7)
Would probably be good to change this to an assert. Otherwise it could be called in legacy code and seem to succeed but never return data.
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446462443)
In commit "wallet: Add GetActiveHDPubKeys to retrieve xpubs from active descriptors" (2621ca0e6418b406161b21b6afac159b0be775b7)
Would probably be good to change this to an assert. Otherwise it could be called in legacy code and seem to succeed but never return data.
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446457022)
In commit "wallet: Refactor function for single DescSPKM setup" (8d107d5f48c2fae0d09bf9be79f9cf47daad6241)
Would be good for this to return a reference instead of a pointer. It seems to never return null, always return non-null or throw an exception.
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446457022)
In commit "wallet: Refactor function for single DescSPKM setup" (8d107d5f48c2fae0d09bf9be79f9cf47daad6241)
Would be good for this to return a reference instead of a pointer. It seems to never return null, always return non-null or throw an exception.
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446468708)
In commit "wallet: Be able to retrieve single key from descriptors" (8bcb89d3730d30a3202fcee172fd709a9f588a4e)
Again I think this would be a little better as an assert so if this is called in legacy wallet code it doesn't appear to succeed but never return anything.
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446468708)
In commit "wallet: Be able to retrieve single key from descriptors" (8bcb89d3730d30a3202fcee172fd709a9f588a4e)
Again I think this would be a little better as an assert so if this is called in legacy wallet code it doesn't appear to succeed but never return anything.
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1445380623)
In commit "wallet: Add GetActiveHDPubKeys to retrieve xpubs from active descriptors" (2621ca0e6418b406161b21b6afac159b0be775b7)
Should delete this comment if it just applies to calling code (it doesn't seem to be enforced here).
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1445380623)
In commit "wallet: Add GetActiveHDPubKeys to retrieve xpubs from active descriptors" (2621ca0e6418b406161b21b6afac159b0be775b7)
Should delete this comment if it just applies to calling code (it doesn't seem to be enforced here).
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446464920)
In commit "wallet: Add GetActiveHDPubKeys to retrieve xpubs from active descriptors" (2621ca0e6418b406161b21b6afac159b0be775b7)
std::move would be appropriate here
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446464920)
In commit "wallet: Add GetActiveHDPubKeys to retrieve xpubs from active descriptors" (2621ca0e6418b406161b21b6afac159b0be775b7)
std::move would be appropriate here
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446473752)
In commit "wallet, rpc: Add createwalletdescriptor RPC" (ed177c90c8e5d3b5f22b6744b725ff760b5897c1)
Maybe consider calling the local variable something like like `opt_internal` or `internal_only` since there is another variable below called `internal` which shadows this.
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446473752)
In commit "wallet, rpc: Add createwalletdescriptor RPC" (ed177c90c8e5d3b5f22b6744b725ff760b5897c1)
Maybe consider calling the local variable something like like `opt_internal` or `internal_only` since there is another variable below called `internal` which shadows this.
💬 ryanofsky commented on pull request "wallet: Add `createwalletdescriptor` and `gethdkeys` RPCs for adding new automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446440048)
In commit "wallet: Add IsActiveScriptPubKeyMan" (2c98b2f395eafa125054e2f6e7c570e18795e318)
Would be a little better to take a reference than a pointer, since it's not useful to be able to pass null.
(https://github.com/bitcoin/bitcoin/pull/29130#discussion_r1446440048)
In commit "wallet: Add IsActiveScriptPubKeyMan" (2c98b2f395eafa125054e2f6e7c570e18795e318)
Would be a little better to take a reference than a pointer, since it's not useful to be able to pass null.
🤔 murchandamus reviewed a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1811161257)
Addressed @sipa’s comments
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1811161257)
Addressed @sipa’s comments
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1446141597)
Good idea.
I’ve added ```auto curr_tail = curr_selection.back();``` to reference that instead throughout the evaluation block.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1446141597)
Good idea.
I’ve added ```auto curr_tail = curr_selection.back();``` to reference that instead throughout the evaluation block.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1446479218)
Thanks, I’ve adopted your proposed change.
For context on the `!best_selection.empty())`: we need to check whether we have found any solution so far, as we would otherwise no longer trigger the "max weight exceeded" failure message.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1446479218)
Thanks, I’ve adopted your proposed change.
For context on the `!best_selection.empty())`: we need to check whether we have found any solution so far, as we would otherwise no longer trigger the "max weight exceeded" failure message.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1446151655)
Ah, oops. Yes of course.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1446151655)
Ah, oops. Yes of course.