💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468939922)
Sure, makes sense.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468939922)
Sure, makes sense.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468941852)
Thanks, fixed up the sentence.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468941852)
Thanks, fixed up the sentence.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468941377)
Added descriptions
```cpp
// The current selection and the best input set found so far, stored as the utxo_pool indices of the UTXOs forming them
std::vector<size_t> curr_selection;
std::vector<size_t> best_selection;
// The currently selected effective amount, and the effective amount of the best selection so far
CAmount curr_amount = 0;
CAmount best_selection_amount = MAX_MONEY;
// The weight of the currently selected input set, and the weight of the best selection
int curr_weig
...
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468941377)
Added descriptions
```cpp
// The current selection and the best input set found so far, stored as the utxo_pool indices of the UTXOs forming them
std::vector<size_t> curr_selection;
std::vector<size_t> best_selection;
// The currently selected effective amount, and the effective amount of the best selection so far
CAmount curr_amount = 0;
CAmount best_selection_amount = MAX_MONEY;
// The weight of the currently selected input set, and the weight of the best selection
int curr_weig
...
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468960861)
That would be good, but that would take more time than I have right now. Will leave this open for the moment.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468960861)
That would be good, but that would take more time than I have right now. Will leave this open for the moment.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468947001)
This seems correct to me, and I’ve fuzzed it for five hours without failure for good measure. This changes a number of commits, in different ways, still pondering whether it’s best to put into the first implementation or to amend the commit that optimizes handling of max_weight. Will revisit.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468947001)
This seems correct to me, and I’ve fuzzed it for five hours without failure for good measure. This changes a number of commits, in different ways, still pondering whether it’s best to put into the first implementation or to amend the commit that optimizes handling of max_weight. Will revisit.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468941448)
Hah, thanks, fixed.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468941448)
Hah, thanks, fixed.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468960224)
Great idea. I’ve amended the fuzz test to exit early if there are insufficient funds, otherwise brute force the solution, and then run CG twice.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468960224)
Great idea. I’ve amended the fuzz test to exit early if there are insufficient funds, otherwise brute force the solution, and then run CG twice.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468966253)
Good catch, thanks.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468966253)
Good catch, thanks.
💬 techy2 commented on issue "loadtxoutset fails to recognize assumeutxo blockhash":
(https://github.com/bitcoin/bitcoin/issues/29340#issuecomment-1913849142)
The release notes indicate this is a new feature, it does not mention that it is not expected to work. Looks like a nice feature. You should clarify the release notes PLEASE.
[bitcoin](https://github.com/bitcoin/bitcoin/tree/master)/[doc](https://github.com/bitcoin/bitcoin/tree/master/doc)/[release-notes](https://github.com/bitcoin/bitcoin/tree/master/doc/release-notes)
/release-notes-26.0.md
New RPCs
loadtxoutset has been added, which allows loading a UTXO snapshot of the format g
...
(https://github.com/bitcoin/bitcoin/issues/29340#issuecomment-1913849142)
The release notes indicate this is a new feature, it does not mention that it is not expected to work. Looks like a nice feature. You should clarify the release notes PLEASE.
[bitcoin](https://github.com/bitcoin/bitcoin/tree/master)/[doc](https://github.com/bitcoin/bitcoin/tree/master/doc)/[release-notes](https://github.com/bitcoin/bitcoin/tree/master/doc/release-notes)
/release-notes-26.0.md
New RPCs
loadtxoutset has been added, which allows loading a UTXO snapshot of the format g
...
💬 theStack commented on pull request "Make provably unsignable standard P2PK and P2MS outpoints unspendable.":
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1913871919)
Fun challenge for everyone: can you link to a specific bare multisig UTXO that has been created within the past year (let's say, since block 769785) where the prunable detection in this PR would hit? Looking at an UTXO snapshot from now (block 827855), from all the 749496 P2MS UTXOs that have been created since the start of the year 2023, I haven't managed to find a _single_ one which is provably unspendable.
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1913871919)
Fun challenge for everyone: can you link to a specific bare multisig UTXO that has been created within the past year (let's say, since block 769785) where the prunable detection in this PR would hit? Looking at an UTXO snapshot from now (block 827855), from all the 749496 P2MS UTXOs that have been created since the start of the year 2023, I haven't managed to find a _single_ one which is provably unspendable.
💬 dzyphr commented on pull request "Make provably unsignable standard P2PK and P2MS outpoints unspendable.":
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1913883771)
> Fun challenge for everyone: can you link to a specific bare multisig UTXO that has been created within the past year (let's say, since block 769785) where the prunable detection in this PR would hit? Looking at an UTXO snapshot from now (block 827855), from all the 749496 P2MS UTXOs that have been created since the start of the year 2023, I haven't managed to find a _single_ one which is provably unspendable.
they have provided a list: https://gist.github.com/russeree/85fb9519e0a1fc166177a6
...
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1913883771)
> Fun challenge for everyone: can you link to a specific bare multisig UTXO that has been created within the past year (let's say, since block 769785) where the prunable detection in this PR would hit? Looking at an UTXO snapshot from now (block 827855), from all the 749496 P2MS UTXOs that have been created since the start of the year 2023, I haven't managed to find a _single_ one which is provably unspendable.
they have provided a list: https://gist.github.com/russeree/85fb9519e0a1fc166177a6
...
💬 ryanofsky commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-1913911460)
re: https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-1910905907
> But if making the log parameter optional is a requirement for you, it should be possible to implement that here and never increase verbosity in any case.
It turns out making the category argument completely optional is not too hard to implement: 69e69b2a82c95f8040be093a29e5f84c37641f3a. So you could write:
```c++
LogWarning("uncategorizable warning")
```
instead of
```
LogWarning({}, "uncategorizable
...
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-1913911460)
re: https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-1910905907
> But if making the log parameter optional is a requirement for you, it should be possible to implement that here and never increase verbosity in any case.
It turns out making the category argument completely optional is not too hard to implement: 69e69b2a82c95f8040be093a29e5f84c37641f3a. So you could write:
```c++
LogWarning("uncategorizable warning")
```
instead of
```
LogWarning({}, "uncategorizable
...
💬 ajtowns commented on pull request "Improve new LogDebug/Trace/Info/Warning/Error Macros":
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-1913991867)
> And if you have ideas on how the harms you see could be avoided, while the problems I am trying to solve (see "Problems this PR attempts to solve" in the PR description) could be addressed,
I think the problem you are actually attempting to solve is ["I, ryanofsky, find this weird and I don't like it"](https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1450411706), and I don't think code changes are the way of solving that, any more than code changes are the right way to solve the p
...
(https://github.com/bitcoin/bitcoin/pull/29256#issuecomment-1913991867)
> And if you have ideas on how the harms you see could be avoided, while the problems I am trying to solve (see "Problems this PR attempts to solve" in the PR description) could be addressed,
I think the problem you are actually attempting to solve is ["I, ryanofsky, find this weird and I don't like it"](https://github.com/bitcoin/bitcoin/pull/28318#discussion_r1450411706), and I don't think code changes are the way of solving that, any more than code changes are the right way to solve the p
...
💬 ajtowns commented on pull request "Make provably unsignable standard P2PK and P2MS outpoints unspendable.":
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1914102082)
> > Fun challenge for everyone: can you link to a specific bare multisig UTXO that has been created within the past year (let's say, since block 769785) where the prunable detection in this PR would hit? Looking at an UTXO snapshot from now (block 827855), from all the 749496 P2MS UTXOs that have been created since the start of the year 2023, I haven't managed to find a _single_ one which is provably unspendable.
>
> they have provided a list: https://gist.github.com/russeree/85fb9519e0a1fc16
...
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1914102082)
> > Fun challenge for everyone: can you link to a specific bare multisig UTXO that has been created within the past year (let's say, since block 769785) where the prunable detection in this PR would hit? Looking at an UTXO snapshot from now (block 827855), from all the 749496 P2MS UTXOs that have been created since the start of the year 2023, I haven't managed to find a _single_ one which is provably unspendable.
>
> they have provided a list: https://gist.github.com/russeree/85fb9519e0a1fc16
...
💬 ajtowns commented on pull request "Make provably unsignable standard P2PK and P2MS outpoints unspendable.":
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1914110506)
> So this would make all current and future STAMPS unspendable? Or just future ones? How would this affect other potential legitimate use cases?
Most STAMPS txs are spendable and are not affected by this PR. As I understand it, that protocol sets up a 1-of-3 multisig, where of the keys is valid (providing a spendable path) and the other two are used for data, and are thus provably unspendable about 50% of the time as the data doesn't match a valid secp point. The only times those txs become u
...
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1914110506)
> So this would make all current and future STAMPS unspendable? Or just future ones? How would this affect other potential legitimate use cases?
Most STAMPS txs are spendable and are not affected by this PR. As I understand it, that protocol sets up a 1-of-3 multisig, where of the keys is valid (providing a spendable path) and the other two are used for data, and are thus provably unspendable about 50% of the time as the data doesn't match a valid secp point. The only times those txs become u
...
🤔 stratospher reviewed a pull request: "test: p2p: check disconnect due to lack of desirable service flags"
(https://github.com/bitcoin/bitcoin/pull/29279#pullrequestreview-1848022516)
tested ACK ff5f994.
(https://github.com/bitcoin/bitcoin/pull/29279#pullrequestreview-1848022516)
tested ACK ff5f994.
💬 stratospher commented on pull request "test: p2p: check disconnect due to lack of desirable service flags":
(https://github.com/bitcoin/bitcoin/pull/29279#discussion_r1469196012)
ff5f994: nit: `NODE_NONE` here too?
(https://github.com/bitcoin/bitcoin/pull/29279#discussion_r1469196012)
ff5f994: nit: `NODE_NONE` here too?
💬 theDavidCoen commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1914214959)
I keep reading ideological points and no one insists on a very basic thing:
consistency.
This PR should be merged because:
1. The code has been tested and it works.
2. Flags that activate a feature should be always set to false. The Bitcoin users (node operators) should be in charge and establish their node's policies actively.
An example:
with version 26.0 we now have the ability to activate v2 transport Protocol, which is a great improvement for Bitcoin.
In order to do that, the users
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1914214959)
I keep reading ideological points and no one insists on a very basic thing:
consistency.
This PR should be merged because:
1. The code has been tested and it works.
2. Flags that activate a feature should be always set to false. The Bitcoin users (node operators) should be in charge and establish their node's policies actively.
An example:
with version 26.0 we now have the ability to activate v2 transport Protocol, which is a great improvement for Bitcoin.
In order to do that, the users
...
💬 dexX7 commented on pull request "Make provably unsignable standard P2PK and P2MS outpoints unspendable.":
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1914218855)
Concept ACK.
Note, however, that pubkeys can be modified to be valid and still be used to store data, for example by using a byte that is shuffled until the pubkey becomes valid.
(https://github.com/bitcoin/bitcoin/pull/28400#issuecomment-1914218855)
Concept ACK.
Note, however, that pubkeys can be modified to be valid and still be used to store data, for example by using a byte that is shuffled until the pubkey becomes valid.
💬 naumenkogs commented on pull request "p2p: adaptive connections services flags":
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1469260564)
9f36e591c551ec2e58a6496334541bfdae8fdfe5
i see that this is a copy-paste from another place, but since it's technically green (and i want to sure it still makes sense after your change), can you explain what is this about? especially `the returned desirable service flags are guaranteed to not change dependent on state`
(https://github.com/bitcoin/bitcoin/pull/28170#discussion_r1469260564)
9f36e591c551ec2e58a6496334541bfdae8fdfe5
i see that this is a copy-paste from another place, but since it's technically green (and i want to sure it still makes sense after your change), can you explain what is this about? especially `the returned desirable service flags are guaranteed to not change dependent on state`