💬 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.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1446094975)
Thanks, I went with
```// EVALUATE current selection: check for solutions and determine whether we can CUT or SHIFT before EXPLORING further```
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1446094975)
Thanks, I went with
```// EVALUATE current selection: check for solutions and determine whether we can CUT or SHIFT before EXPLORING further```
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1446186725)
Thanks, I did some hard staring at this while-loop. I restructured it to use a `break` as you suggested, and was able to reduce the loop’s condition to:
```diff
- while (!should_shift && next_utxo > 0
- && (curr_selection.empty() || curr_selection.back() != next_utxo - 1)
- && utxo_pool[next_utxo - 1].GetSelectionAmount() == utxo_pool[next_utxo].GetSelectionAmount()
+ while (utxo_pool[next_utxo - 1].GetSelectionAmount() == utxo_p
...
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1446186725)
Thanks, I did some hard staring at this while-loop. I restructured it to use a `break` as you suggested, and was able to reduce the loop’s condition to:
```diff
- while (!should_shift && next_utxo > 0
- && (curr_selection.empty() || curr_selection.back() != next_utxo - 1)
- && utxo_pool[next_utxo - 1].GetSelectionAmount() == utxo_pool[next_utxo].GetSelectionAmount()
+ while (utxo_pool[next_utxo - 1].GetSelectionAmount() == utxo_p
...
💬 jamesob commented on pull request "fuzz: Test headers pre-sync through p2p interface":
(https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1446512814)
Should probably have some belt-and-suspenders check that we're 100% not running on mainnet, maybe based on the contents of `params` and whatever applicable global.
(https://github.com/bitcoin/bitcoin/pull/28043#discussion_r1446512814)
Should probably have some belt-and-suspenders check that we're 100% not running on mainnet, maybe based on the contents of `params` and whatever applicable global.
🤔 sr-gi reviewed a pull request: "p2p: Fill reconciliation sets (Erlay)"
(https://github.com/bitcoin/bitcoin/pull/28765#pullrequestreview-1811832956)
Some additional comments after the recent optimizations + caching
(https://github.com/bitcoin/bitcoin/pull/28765#pullrequestreview-1811832956)
Some additional comments after the recent optimizations + caching
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1446503850)
nit: `targes` -> `targets`
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1446503850)
nit: `targes` -> `targets`
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1446517082)
nit: I'd be nice to define this as a constant
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1446517082)
nit: I'd be nice to define this as a constant
💬 sr-gi commented on pull request "p2p: Fill reconciliation sets (Erlay)":
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1446518794)
I don't think this is right(?), this logic is never triggered.
You are checking whether the size of `tx_fanout_targes_cache_order` is over the limit, but you are only adding elements to the queue within that same `if` context, meaning that the queue is always empty.
(https://github.com/bitcoin/bitcoin/pull/28765#discussion_r1446518794)
I don't think this is right(?), this logic is never triggered.
You are checking whether the size of `tx_fanout_targes_cache_order` is over the limit, but you are only adding elements to the queue within that same `if` context, meaning that the queue is always empty.
👍 alfonsoromanz approved a pull request: "doc: Add example of mixing private and public keys in descriptors"
(https://github.com/bitcoin/bitcoin/pull/28373#pullrequestreview-1811870092)
ACK
(https://github.com/bitcoin/bitcoin/pull/28373#pullrequestreview-1811870092)
ACK
💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#issuecomment-1883691930)
Updated https://github.com/bitcoin/bitcoin/commit/909c6bb1dbc6f60c5ad3200d83f3fce98f4c3706 -> https://github.com/bitcoin/bitcoin/commit/c856a1093009c6098c7c9fed5cc3f03fd0c0cf5b ([compare](https://github.com/josibake/bitcoin/compare/909c6bb1dbc6f60c5ad3200d83f3fce98f4c3706..c856a1093009c6098c7c9fed5cc3f03fd0c0cf5b))
* Moved `CTxDestination`, `Amount` into loop
* Refactored `InterpretSubtractFeeFromAmountInstructions` to infer parsing logic based on the instruction type (int, bool, str)
Tha
...
(https://github.com/bitcoin/bitcoin/pull/28560#issuecomment-1883691930)
Updated https://github.com/bitcoin/bitcoin/commit/909c6bb1dbc6f60c5ad3200d83f3fce98f4c3706 -> https://github.com/bitcoin/bitcoin/commit/c856a1093009c6098c7c9fed5cc3f03fd0c0cf5b ([compare](https://github.com/josibake/bitcoin/compare/909c6bb1dbc6f60c5ad3200d83f3fce98f4c3706..c856a1093009c6098c7c9fed5cc3f03fd0c0cf5b))
* Moved `CTxDestination`, `Amount` into loop
* Refactored `InterpretSubtractFeeFromAmountInstructions` to infer parsing logic based on the instruction type (int, bool, str)
Tha
...