💬 sr-gi commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475116651)
In commit: test: Add coin_grinder_tests
Same as per point 5)
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475116651)
In commit: test: Add coin_grinder_tests
Same as per point 5)
💬 sr-gi commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475157115)
In opt: Track remaining effective_value in lookahead:
I wonder why this gets turned into a descending while loop instead of a descending for loop. Given the total number of iterations is known beforehand a for seems best suited (plus the scope of the counter is reduced to to scope of the loop)
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475157115)
In opt: Track remaining effective_value in lookahead:
I wonder why this gets turned into a descending while loop instead of a descending for loop. Given the total number of iterations is known beforehand a for seems best suited (plus the scope of the counter is reduced to to scope of the loop)
💬 sr-gi commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475162152)
in test: Exhaust search attempts with tiny UTXOs:
Same as for cases 5) and 6), clearing is not needed
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475162152)
in test: Exhaust search attempts with tiny UTXOs:
Same as for cases 5) and 6), clearing is not needed
💬 ariard commented on issue "Cluster mempool, CPFP carveout, and V3 transaction policy":
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1922277580)
@petertodd
> Can you give a bit more detail on what challenges you think that'll pose?
from my memory: "How this new replacement rule would behave if you have a parent in the
"replace-by-feerate" half but the child is in the "replace-by-fee" one ?” see the conversation here: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019839.html
in past conversations we have assumed a “static” N blocks worth of mempool, unclear to me with your proposal if dynamic.
i wonder abo
...
(https://github.com/bitcoin/bitcoin/issues/29319#issuecomment-1922277580)
@petertodd
> Can you give a bit more detail on what challenges you think that'll pose?
from my memory: "How this new replacement rule would behave if you have a parent in the
"replace-by-feerate" half but the child is in the "replace-by-fee" one ?” see the conversation here: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2022-January/019839.html
in past conversations we have assumed a “static” N blocks worth of mempool, unclear to me with your proposal if dynamic.
i wonder abo
...
⚠️ ariard opened an issue: "Update security.md with all PGP fingerprints"
(https://github.com/bitcoin/bitcoin/issues/29366)
As a bitcoin sec researcher, appreciated if PGP fingerprints of all receiving endpoints of security@bitcoincore.org can be public.
Thanks.
(https://github.com/bitcoin/bitcoin/issues/29366)
As a bitcoin sec researcher, appreciated if PGP fingerprints of all receiving endpoints of security@bitcoincore.org can be public.
Thanks.
💬 tdb3 commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1475182625)
>Although from discussion thread above it sounds like maybe we are willing to not fully comply with 2.0 spec if we are prioritizing backwards compatibility over spec compatibility in cases where 2.0 spec is mandating a certain response even when the request does not contain a "jsonrpc":"2.0" field.
Yeah, exactly this. If/when support for jsonrpc 1.0 and 1.1 is removed, then the implementation could be updated to achieve full/closer compliance to the 2.0 spec. If it's decided that jsonrpc 1.
...
(https://github.com/bitcoin/bitcoin/pull/27101#discussion_r1475182625)
>Although from discussion thread above it sounds like maybe we are willing to not fully comply with 2.0 spec if we are prioritizing backwards compatibility over spec compatibility in cases where 2.0 spec is mandating a certain response even when the request does not contain a "jsonrpc":"2.0" field.
Yeah, exactly this. If/when support for jsonrpc 1.0 and 1.1 is removed, then the implementation could be updated to achieve full/closer compliance to the 2.0 spec. If it's decided that jsonrpc 1.
...
💬 tdb3 commented on pull request "Support JSON-RPC 2.0 when requested by client":
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1922293722)
ACK for 58cb22c83c1b72af7f3c580dc6261cf50e7ed2cb. Thanks for the great feature update. Verification steps taken below.
Pulled 58cb22c83c1b72af7f3c580dc6261cf50e7ed2cb. Each of the seven cases (from above) exhibited expected behavior (i.e. non-2.0 requests match previous response behavior, 2.0-marked requests appeared spec-compliant).
The content below is as-run request/response for posterity.
### Action (1):
`curl --user test --data-binary '[]' -H 'content-type: text/plain;' h
...
(https://github.com/bitcoin/bitcoin/pull/27101#issuecomment-1922293722)
ACK for 58cb22c83c1b72af7f3c580dc6261cf50e7ed2cb. Thanks for the great feature update. Verification steps taken below.
Pulled 58cb22c83c1b72af7f3c580dc6261cf50e7ed2cb. Each of the seven cases (from above) exhibited expected behavior (i.e. non-2.0 requests match previous response behavior, 2.0-marked requests appeared spec-compliant).
The content below is as-run request/response for posterity.
### Action (1):
`curl --user test --data-binary '[]' -H 'content-type: text/plain;' h
...
💬 achow101 commented on issue "Update security.md with all PGP fingerprints":
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1922296387)
The full recipients list of security@bitcoincore.org is intentionally kept private in order to reduce the risk of targeted attacks against those who receive those emails. Only the people who are willing to take that risk have their fingerprints published.
(https://github.com/bitcoin/bitcoin/issues/29366#issuecomment-1922296387)
The full recipients list of security@bitcoincore.org is intentionally kept private in order to reduce the risk of targeted attacks against those who receive those emails. Only the people who are willing to take that risk have their fingerprints published.
💬 ariard commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1922316467)
> I definitely appreciate the discussions about LN usage, but it would be best to take it to the Delving thread. Here's the link > for {switching, imbuing} {commitment, HTLC-X} transactions to use {v3, ephemeral anchors}: https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418/24
no time for delving - apologies.
concern is on the lack of anti-pinning robustness of v3 policy, see my previous comments.
if correct and leave a substantial pinning loophole, i’m wo
...
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1922316467)
> I definitely appreciate the discussions about LN usage, but it would be best to take it to the Delving thread. Here's the link > for {switching, imbuing} {commitment, HTLC-X} transactions to use {v3, ephemeral anchors}: https://delvingbitcoin.org/t/lightning-transactions-with-v3-and-ephemeral-anchors/418/24
no time for delving - apologies.
concern is on the lack of anti-pinning robustness of v3 policy, see my previous comments.
if correct and leave a substantial pinning loophole, i’m wo
...
💬 theuni commented on pull request "serialization: c++20 endian/byteswap/clz modernization":
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1922334399)
Aha, I finally tracked down the culprit! No wonder the benchmarks weren't making sense!
The problem was the removal of this from `crypto/common.h`:
```c++
#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif
```
Turns out, `sha256.cpp` was relying on that. Adding that include to `sha256.cpp` (where it belongs) fixes my local benchmarks.
I'm going to put the clz changes back to match the PR title/description, but this time with the include fixed up. Hopefully after t
...
(https://github.com/bitcoin/bitcoin/pull/29263#issuecomment-1922334399)
Aha, I finally tracked down the culprit! No wonder the benchmarks weren't making sense!
The problem was the removal of this from `crypto/common.h`:
```c++
#if defined(HAVE_CONFIG_H)
#include <config/bitcoin-config.h>
#endif
```
Turns out, `sha256.cpp` was relying on that. Adding that include to `sha256.cpp` (where it belongs) fixes my local benchmarks.
I'm going to put the clz changes back to match the PR title/description, but this time with the include fixed up. Hopefully after t
...
🤔 murchandamus reviewed a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1857527717)
Addressed feedback by @sipa and @sr-gi.
I also improved the clone skipping by skipping any UTXOs with same effective value and heavier weight that follow an unselected UTXO.
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1857527717)
Addressed feedback by @sipa and @sr-gi.
I also improved the clone skipping by skipping any UTXOs with same effective value and heavier weight that follow an unselected UTXO.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475100291)
Changed as follows:
```diff
- // find any solutions. Redirect to exploring Omission branch of the penultimate selected UTXO (i.e. deselect
- // last selected UTXO, then SHIFT)
+ // find any solutions. Redirect to exploring the Omission branch of the penultimate selected UTXO (i.e.
+ // set `next_utxo` to one after the penultimate selected, then deselect the last two selected UTXOs)
```
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475100291)
Changed as follows:
```diff
- // find any solutions. Redirect to exploring Omission branch of the penultimate selected UTXO (i.e. deselect
- // last selected UTXO, then SHIFT)
+ // find any solutions. Redirect to exploring the Omission branch of the penultimate selected UTXO (i.e.
+ // set `next_utxo` to one after the penultimate selected, then deselect the last two selected UTXOs)
```
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475094275)
Moved `result.SetSelectionsEvaluated(curr_try)` after the loop
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475094275)
Moved `result.SetSelectionsEvaluated(curr_try)` after the loop
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475155882)
Instead of comparing fees, I’m comparing weights here now. I’ll also add a commit to skip larger weights after tie-breaking the sort order with the weight.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475155882)
Instead of comparing fees, I’m comparing weights here now. I’ll also add a commit to skip larger weights after tie-breaking the sort order with the weight.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475095394)
Fixed
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475095394)
Fixed
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475183856)
Done
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475183856)
Done
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475146550)
I had to make the following change to ensure that there is a positive effective value:
```diff
- const CAmount eff_value{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(0, MAX_MONEY - max_spendable)};
+ // Ensure that each UTXO has at least an effective value of 1 sat
+ const CAmount eff_value{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(1, MAX_MONEY - max_spendable - max_output_groups + group_pos.size())};
```
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475146550)
I had to make the following change to ensure that there is a positive effective value:
```diff
- const CAmount eff_value{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(0, MAX_MONEY - max_spendable)};
+ // Ensure that each UTXO has at least an effective value of 1 sat
+ const CAmount eff_value{fuzzed_data_provider.ConsumeIntegralInRange<CAmount>(1, MAX_MONEY - max_spendable - max_output_groups + group_pos.size())};
```
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475183696)
Yep, moved declaration down there.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475183696)
Yep, moved declaration down there.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475198825)
Sure, replaced with:
```diff
- size_t i = utxo_pool.size();
- while (i > 0) {
- --i;
+ for (int i = utxo_pool.size() - 1 ; i >= 0; --i) {
```
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475198825)
Sure, replaced with:
```diff
- size_t i = utxo_pool.size();
- while (i > 0) {
- --i;
+ for (int i = utxo_pool.size() - 1 ; i >= 0; --i) {
```
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475202681)
Absolutely. Fixed.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1475202681)
Absolutely. Fixed.