💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468539208)
In commit "coinselection: Add CoinGrinder algorithm"
"state transactions" -> "state transitions"?
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468539208)
In commit "coinselection: Add CoinGrinder algorithm"
"state transactions" -> "state transitions"?
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468584127)
In commit "wallet: Add CoinGrinder coin selection algorithm":
Would it make sense to construct the `group_pos` variable directly from the fuzzer? I think you only need to populate its `m_weight` and `m_effective_value`. As you don't use anything but `group_pos`, `target`, and `change_target` in the actual test, all fuzz information you use to construct other things is effectively wasted.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468584127)
In commit "wallet: Add CoinGrinder coin selection algorithm":
Would it make sense to construct the `group_pos` variable directly from the fuzzer? I think you only need to populate its `m_weight` and `m_effective_value`. As you don't use anything but `group_pos`, `target`, and `change_target` in the actual test, all fuzz information you use to construct other things is effectively wasted.
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468544154)
In commit "fuzz: Add CoinGrinder fuzz target"
`coin_params.m_min_change_target` is left at 0 here. Perhaps leave a comment about why that is ok?
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468544154)
In commit "fuzz: Add CoinGrinder fuzz target"
`coin_params.m_min_change_target` is left at 0 here. Perhaps leave a comment about why that is ok?
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468584560)
In commit "wallet: Add CoinGrinder coin selection algorithm":
SRD is not used in this test, so I think you can construct `change_target` from fuzz data directly.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468584560)
In commit "wallet: Add CoinGrinder coin selection algorithm":
SRD is not used in this test, so I think you can construct `change_target` from fuzz data directly.
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468513733)
In commit "coinselection: Add CoinGrinder algorithm"
Not as much in this commit yet, but in the PR overall, almost everywhere the sum `selection_target + change_target` is used. Maybe it makes sense to have a `const auto total_target = selection_target + change_target`, and use `total_target` everywhere?
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468513733)
In commit "coinselection: Add CoinGrinder algorithm"
Not as much in this commit yet, but in the PR overall, almost everywhere the sum `selection_target + change_target` is used. Maybe it makes sense to have a `const auto total_target = selection_target + change_target`, and use `total_target` everywhere?
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468584961)
In commit "wallet: Add CoinGrinder coin selection algorithm":
I think it would be better if you'd also construct `max_weight` from the fuzz data (and then take it into account in the brute force loop below).
As an alternative, run the brute force loop first (without any weight maximum restriction), and then run CG twice, once with a (fuzz-constructed) `max_weight` >= the brute force best solution (and check that CG finds it) and once with a (fuzz-constructed) `max_weight` < the brute force
...
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468584961)
In commit "wallet: Add CoinGrinder coin selection algorithm":
I think it would be better if you'd also construct `max_weight` from the fuzz data (and then take it into account in the brute force loop below).
As an alternative, run the brute force loop first (without any weight maximum restriction), and then run CG twice, once with a (fuzz-constructed) `max_weight` >= the brute force best solution (and check that CG finds it) and once with a (fuzz-constructed) `max_weight` < the brute force
...
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468517854)
In commit "coinselection: Add CoinGrinder algorithm"
I don't believe it's actually possible to not have a solution at this point, unless the maximum weight is exceeded because the two ways of achieving that are:
* Not enough funds (but that is checked early in the function)
* A solution exists, but wasn't found due to computation limits. However, with a UTXO set of $n$ elements/groups, *a* solution should always be found within $n$ iterations (and I think the number of iterations should be
...
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468517854)
In commit "coinselection: Add CoinGrinder algorithm"
I don't believe it's actually possible to not have a solution at this point, unless the maximum weight is exceeded because the two ways of achieving that are:
* Not enough funds (but that is checked early in the function)
* A solution exists, but wasn't found due to computation limits. However, with a UTXO set of $n$ elements/groups, *a* solution should always be found within $n$ iterations (and I think the number of iterations should be
...
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468584354)
In commit "wallet: Add CoinGrinder coin selection algorithm":
Only one coin selection algorithm is run here (unless you count the brute forcing).
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468584354)
In commit "wallet: Add CoinGrinder coin selection algorithm":
Only one coin selection algorithm is run here (unless you count the brute forcing).
💬 furszy commented on issue "New crash in v26.0":
(https://github.com/bitcoin-core/gui/issues/785#issuecomment-1913324146)
It seems you are running a prune node. Please with the logs, provide your bitcoin.conf values (hiding the sensitive information).
(https://github.com/bitcoin-core/gui/issues/785#issuecomment-1913324146)
It seems you are running a prune node. Please with the logs, provide your bitcoin.conf values (hiding the sensitive information).
💬 furszy commented on pull request "Add max_tx_weight to transaction funding options":
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1468635643)
This does not seem to be necessary. As `sendall()` doesn't make use of the internal coin selection procedures, wouldn't make more sense to return the weight in the command result and let the user decide whether to broadcast the tx based on it?
(https://github.com/bitcoin/bitcoin/pull/29264#discussion_r1468635643)
This does not seem to be necessary. As `sendall()` doesn't make use of the internal coin selection procedures, wouldn't make more sense to return the weight in the command result and let the user decide whether to broadcast the tx based on it?
🤔 furszy reviewed a pull request: "net: don't lock cs_main while reading blocks in net processing"
(https://github.com/bitcoin/bitcoin/pull/26326#pullrequestreview-1847265131)
Re-ACK f9d4152.
Since my last review, this PR was only rebased and a comment was added.
(https://github.com/bitcoin/bitcoin/pull/26326#pullrequestreview-1847265131)
Re-ACK f9d4152.
Since my last review, this PR was only rebased and a comment was added.
⚠️ hebasto opened an issue: "Clang 14 emits `-Wunreachable-code` warnings"
(https://github.com/bitcoin/bitcoin/issues/29334)
```
% clang --version
Apple clang version 14.0.0 (clang-1400.0.29.202)
Target: x86_64-apple-darwin21.6.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin
```
```
...
CXX wallet/libbitcoin_wallet_a-walletdb.o
wallet/walletdb.cpp:1518:15: warning: code will never be executed [-Wunreachable-code]
error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path)));
...
(https://github.com/bitcoin/bitcoin/issues/29334)
```
% clang --version
Apple clang version 14.0.0 (clang-1400.0.29.202)
Target: x86_64-apple-darwin21.6.0
Thread model: posix
InstalledDir: /Library/Developer/CommandLineTools/usr/bin
```
```
...
CXX wallet/libbitcoin_wallet_a-walletdb.o
wallet/walletdb.cpp:1518:15: warning: code will never be executed [-Wunreachable-code]
error = Untranslated(strprintf("Failed to open database path '%s'. Build does not support Berkeley DB database format.", fs::PathToString(path)));
...
💬 TheCharlatan commented on pull request "Add missed `config/bitcoin-config.h` header":
(https://github.com/bitcoin/bitcoin/pull/29333#issuecomment-1913340159)
What motivates changing only this single file and not the various other modules that iwyu reports to have unused or missing `bitcoin-config.h` includes?
(https://github.com/bitcoin/bitcoin/pull/29333#issuecomment-1913340159)
What motivates changing only this single file and not the various other modules that iwyu reports to have unused or missing `bitcoin-config.h` includes?
✅ hebasto closed a pull request: "Add missed `config/bitcoin-config.h` header"
(https://github.com/bitcoin/bitcoin/pull/29333)
(https://github.com/bitcoin/bitcoin/pull/29333)
💬 hebasto commented on pull request "Add missed `config/bitcoin-config.h` header":
(https://github.com/bitcoin/bitcoin/pull/29333#issuecomment-1913342542)
> What motivates changing only this single file and not the various other modules that iwyu reports to have unused or missing `bitcoin-config.h` includes?
It was spotted during debugging another issue. Agree that it looks like a random diff.
(https://github.com/bitcoin/bitcoin/pull/29333#issuecomment-1913342542)
> What motivates changing only this single file and not the various other modules that iwyu reports to have unused or missing `bitcoin-config.h` includes?
It was spotted during debugging another issue. Agree that it looks like a random diff.
✅ petertodd closed a pull request: "RBF: Require unconfirmed inputs to come from a single conflicting transaction"
(https://github.com/bitcoin/bitcoin/pull/29297)
(https://github.com/bitcoin/bitcoin/pull/29297)
💬 petertodd commented on pull request "RBF: Require unconfirmed inputs to come from a single conflicting transaction":
(https://github.com/bitcoin/bitcoin/pull/29297#issuecomment-1913382743)
Closing. Turns out the exploit this was meant to solve doesn't actually exist: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2024-January/022314.html
(https://github.com/bitcoin/bitcoin/pull/29297#issuecomment-1913382743)
Closing. Turns out the exploit this was meant to solve doesn't actually exist: https://lists.linuxfoundation.org/pipermail/bitcoin-dev/2024-January/022314.html
💬 petertodd commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1913477451)
@chrisguida Let me expand on my NACK: given how easy it is for miners to adopt profit-maximizing transaction policies, and how easy it is for people to relay profit-maximizing transactions to them, we should not try to filter out profitable transactions. That'll just make life more difficult for smaller miners and encourage the growth of out-of-band transaction submission mechanisms that only large miners can realistically profit from.
We'd all be better off focusing our energy on technical i
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1913477451)
@chrisguida Let me expand on my NACK: given how easy it is for miners to adopt profit-maximizing transaction policies, and how easy it is for people to relay profit-maximizing transactions to them, we should not try to filter out profitable transactions. That'll just make life more difficult for smaller miners and encourage the growth of out-of-band transaction submission mechanisms that only large miners can realistically profit from.
We'd all be better off focusing our energy on technical i
...
💬 BitcoinMechanic commented on pull request "set `DEFAULT_PERMIT_BAREMULTISIG` to false":
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1913486716)
@petertodd
>we should not try to filter out profitable transactions
That is not what I or I believe others wish to do. The concern is the circumnavigating around sensible restrictions on arbitrary data storage, not presence of a relatively high fee.
>That'll just make life more difficult for smaller miners and encourage the growth of out-of-band transaction submission mechanisms
This is something that can only be offered by the larger pools which, as you point out can create centra
...
(https://github.com/bitcoin/bitcoin/pull/28217#issuecomment-1913486716)
@petertodd
>we should not try to filter out profitable transactions
That is not what I or I believe others wish to do. The concern is the circumnavigating around sensible restrictions on arbitrary data storage, not presence of a relatively high fee.
>That'll just make life more difficult for smaller miners and encourage the growth of out-of-band transaction submission mechanisms
This is something that can only be offered by the larger pools which, as you point out can create centra
...
📝 BrandonOdiwuor opened a pull request: "test: Handle functional test disk-full error"
(https://github.com/bitcoin/bitcoin/pull/29335)
Fixes: https://github.com/bitcoin/bitcoin/issues/23099
Handle disk-full more gracefully in functional tests
(https://github.com/bitcoin/bitcoin/pull/29335)
Fixes: https://github.com/bitcoin/bitcoin/issues/23099
Handle disk-full more gracefully in functional tests