📝 hebasto opened a pull request: "Add missed `config/bitcoin-config.h` header"
(https://github.com/bitcoin/bitcoin/pull/29333)
The `config/bitcoin-config.h` header is required to provide definitions for the `USE_BDB` and `USE_SQLITE` macros.
While this header might be included indirectly elsewhere, including it explicitly makes the build process more robust.
(https://github.com/bitcoin/bitcoin/pull/29333)
The `config/bitcoin-config.h` header is required to provide definitions for the `USE_BDB` and `USE_SQLITE` macros.
While this header might be included indirectly elsewhere, including it explicitly makes the build process more robust.
💬 furszy commented on pull request "Add missed `config/bitcoin-config.h` header":
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468552774)
Isn't `bitcoin-config.h` been indirectly included by this `system.h` include? (which btw, doesn't seems to be needed).
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468552774)
Isn't `bitcoin-config.h` been indirectly included by this `system.h` include? (which btw, doesn't seems to be needed).
💬 hebasto commented on pull request "Add missed `config/bitcoin-config.h` header":
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468556096)
> Isn't `bitcoin-config.h` indirectly included by this `system.h` include?
It is.
> While this header might be included indirectly elsewhere, including it explicitly makes the build process more robust.
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468556096)
> Isn't `bitcoin-config.h` indirectly included by this `system.h` include?
It is.
> While this header might be included indirectly elsewhere, including it explicitly makes the build process more robust.
💬 furszy commented on pull request "Add missed `config/bitcoin-config.h` header":
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468568723)
it would be good to remove the unused include within this change. walletdb.cpp does not call to any system.h` function.
(https://github.com/bitcoin/bitcoin/pull/29333#discussion_r1468568723)
it would be good to remove the unused include within this change. walletdb.cpp does not call to any system.h` function.
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468539058)
In commit "coinselection: Add CoinGrinder algorithm"
Perhaps add a one-line comment for each of the 8 variables here.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468539058)
In commit "coinselection: Add CoinGrinder algorithm"
Perhaps add a one-line comment for each of the 8 variables here.
💬 sipa commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468513141)
In commit "coinselection: Add CoinGrinder algorithm"
Nit: non-positive effective value (0 is not allowed either, apparently)
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1468513141)
In commit "coinselection: Add CoinGrinder algorithm"
Nit: non-positive effective value (0 is not allowed either, apparently)
💬 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)