💬 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
...
💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1446538974)
Fixed
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1446538974)
Fixed
💬 josibake commented on pull request "wallet, rpc: `FundTransaction` refactor":
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1446539102)
Done
(https://github.com/bitcoin/bitcoin/pull/28560#discussion_r1446539102)
Done