📝 hernanmarino opened a pull request: "gui: Correct tooltip wording for watch-only wallets"
(https://github.com/bitcoin-core/gui/pull/792)
This is a continuation of https://github.com/bitcoin-core/gui/pull/37
This is a simple change, but imho necessary because it leads to confusion.
The objective of this change is to provide a better description for the tooltip for the "Available Balance" balance because the current version on master mentions the word "You current spendable balance" which is a bit misleading, because that balance is not always spendable , or not always yours on watch only wallets.
(https://github.com/bitcoin-core/gui/pull/792)
This is a continuation of https://github.com/bitcoin-core/gui/pull/37
This is a simple change, but imho necessary because it leads to confusion.
The objective of this change is to provide a better description for the tooltip for the "Available Balance" balance because the current version on master mentions the word "You current spendable balance" which is a bit misleading, because that balance is not always spendable , or not always yours on watch only wallets.
💬 hernanmarino commented on pull request "gui: Correct tooltip wording for watch-only wallets":
(https://github.com/bitcoin-core/gui/pull/792#issuecomment-1935323075)
As a first commit, I only took @ saibato's latest commit and corrected a couple of bugs (mentioned by @pablomartin4btc in the original PR.)
Marked as Draft, in the following days I'll try to address all comments and sugestions in that PR, and mark it ready for review.
(https://github.com/bitcoin-core/gui/pull/792#issuecomment-1935323075)
As a first commit, I only took @ saibato's latest commit and corrected a couple of bugs (mentioned by @pablomartin4btc in the original PR.)
Marked as Draft, in the following days I'll try to address all comments and sugestions in that PR, and mark it ready for review.
💬 hernanmarino commented on pull request "indicate explicit to the user that the wallet balances shown is watch only.":
(https://github.com/bitcoin-core/gui/pull/37#issuecomment-1935326761)
Hi, if no one disagrees, I am taking over the development of this functionality. Discussion can continue here: https://github.com/bitcoin-core/gui/pull/792 Thanks @Saibato for the work
(https://github.com/bitcoin-core/gui/pull/37#issuecomment-1935326761)
Hi, if no one disagrees, I am taking over the development of this functionality. Discussion can continue here: https://github.com/bitcoin-core/gui/pull/792 Thanks @Saibato for the work
🤔 stratospher reviewed a pull request: "rpc: parse legacy pubkeys consistently with specific error messages"
(https://github.com/bitcoin/bitcoin/pull/28336#pullrequestreview-1871661382)
Concept ACK! liked how the error handling is done in a consitent manner now.
(https://github.com/bitcoin/bitcoin/pull/28336#pullrequestreview-1871661382)
Concept ACK! liked how the error handling is done in a consitent manner now.
💬 stratospher commented on pull request "rpc: parse legacy pubkeys consistently with specific error messages":
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1483864342)
b8ee41b: is it impossible to have valid bitcoin addresses which are just hex characters?
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1483864342)
b8ee41b: is it impossible to have valid bitcoin addresses which are just hex characters?
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1483975288)
I see thanks for the explanation. It seems to me like most of these cases should be caught by the optimization to cut instead of shift when we exceed the weight:
> opt: Cut if last addition was minimal weight
>
> In situations where we have UTXO groups of various weight, we can CUT
> rather than SHIFT when we exceeded the max_weight or the best
> selection’s weight while the last step was equal to the minimum weight
> in the lookahead.
It would take more time to determine whether th
...
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1483975288)
I see thanks for the explanation. It seems to me like most of these cases should be caught by the optimization to cut instead of shift when we exceed the weight:
> opt: Cut if last addition was minimal weight
>
> In situations where we have UTXO groups of various weight, we can CUT
> rather than SHIFT when we exceeded the max_weight or the best
> selection’s weight while the last step was equal to the minimum weight
> in the lookahead.
It would take more time to determine whether th
...
💬 maflcko commented on pull request "fuzz: increase length of string used for `NetWhitelist{bind}Permissions::TryParse`":
(https://github.com/bitcoin/bitcoin/pull/29413#issuecomment-1935504931)
lgtm ACK 864e2e9097de8f1fda63137f803687dd5cc96c03
(https://github.com/bitcoin/bitcoin/pull/29413#issuecomment-1935504931)
lgtm ACK 864e2e9097de8f1fda63137f803687dd5cc96c03
💬 maflcko commented on pull request "doc: Update translation process guide":
(https://github.com/bitcoin/bitcoin/pull/29414#discussion_r1483999946)
Instead of copy-pasting third-party docs, it may be better to just update the link to the third-party docs and remove the outdated parts, to reduce the maintenance overhead here.
(https://github.com/bitcoin/bitcoin/pull/29414#discussion_r1483999946)
Instead of copy-pasting third-party docs, it may be better to just update the link to the third-party docs and remove the outdated parts, to reduce the maintenance overhead here.
💬 maflcko commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1484005817)
If node 0 can load the snapshot, that sounds like a bug, because node 0 created the snapshot, no?
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1484005817)
If node 0 can load the snapshot, that sounds like a bug, because node 0 created the snapshot, no?
💬 TheCharlatan commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1935522411)
Made this into a scripted-diff here: https://github.com/TheCharlatan/bitcoin/commit/e4186f771294a8b112bac759f92ea5e4bef8f168
There are no differences with the patch in this PR, besides a single whitespace issue. Feel free to pick the commit and make this into a scripted-diff as well, though I'm not sure if it is worth it, since the script is a bit dense.
(https://github.com/bitcoin/bitcoin/pull/29404#issuecomment-1935522411)
Made this into a scripted-diff here: https://github.com/TheCharlatan/bitcoin/commit/e4186f771294a8b112bac759f92ea5e4bef8f168
There are no differences with the patch in this PR, besides a single whitespace issue. Feel free to pick the commit and make this into a scripted-diff as well, though I'm not sure if it is worth it, since the script is a bit dense.
💬 maflcko commented on pull request "Nuke adjusted time from validation (attempt 2)":
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1935525053)
> My belief we should conserve a a warning system based on other peers nversion announcements.
Yes, this is exactly what this pull request did. See also the description, which said: "while the warning to users if their clock is out of sync with the rest of the network remains."
See also: https://bitcoinops.org/en/newsletters/2024/02/07/#bitcoin-core-28956
(https://github.com/bitcoin/bitcoin/pull/28956#issuecomment-1935525053)
> My belief we should conserve a a warning system based on other peers nversion announcements.
Yes, this is exactly what this pull request did. See also the description, which said: "while the warning to users if their clock is out of sync with the rest of the network remains."
See also: https://bitcoinops.org/en/newsletters/2024/02/07/#bitcoin-core-28956
💬 maflcko commented on issue "getJsonToken assumes underlying string is null-terminated but requires end pointer":
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1935535162)
I like bugfixes the most, when they are touching only a single line of code. But if more changes are required, then that is fine, too.
I checked that this is only a theoretical issue in Bitcoin Core, and can not be hit in production code, but it may still be good to fix it.
It may be good to update the fuzz target to catch the issue here: `src/test/fuzz/parse_univalue.cpp`
(https://github.com/bitcoin/bitcoin/issues/28260#issuecomment-1935535162)
I like bugfixes the most, when they are touching only a single line of code. But if more changes are required, then that is fine, too.
I checked that this is only a theoretical issue in Bitcoin Core, and can not be hit in production code, but it may still be good to fix it.
It may be good to update the fuzz target to catch the issue here: `src/test/fuzz/parse_univalue.cpp`
💬 0xB10C commented on pull request "rpc: addpeeraddress tried return error on failure":
(https://github.com/bitcoin/bitcoin/pull/28998#issuecomment-1935600643)
Thanks for the review @sr-gi! You might want to take a look at #29007 first - this allows us to also test `failed-adding-to-tried`. Once this is merged, I'll continue to work on this PR.
(https://github.com/bitcoin/bitcoin/pull/28998#issuecomment-1935600643)
Thanks for the review @sr-gi! You might want to take a look at #29007 first - this allows us to also test `failed-adding-to-tried`. Once this is merged, I'll continue to work on this PR.
💬 maflcko commented on pull request "bitcoin-config.h includes cleanup":
(https://github.com/bitcoin/bitcoin/pull/29404#discussion_r1484076469)
My thinking was that removing the guard doesn't preclude to copy the header and use it elsewhere, instead it makes it obvious that some features are disabled, or may be misdetected (which could lead to bugs). The overhead of removing a single line of code after creating the copy seems worth it to notify the user of this fact? Also, the overall overhead of all users copying a few headers and having to remove a single line in some of them should be much less than the overhead on Bitcoin Core havin
...
(https://github.com/bitcoin/bitcoin/pull/29404#discussion_r1484076469)
My thinking was that removing the guard doesn't preclude to copy the header and use it elsewhere, instead it makes it obvious that some features are disabled, or may be misdetected (which could lead to bugs). The overhead of removing a single line of code after creating the copy seems worth it to notify the user of this fact? Also, the overall overhead of all users copying a few headers and having to remove a single line in some of them should be much less than the overhead on Bitcoin Core havin
...
💬 Sjors commented on pull request "Stratum v2 Noise Protocol":
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-1935603779)
This latest push improves support for mock time (which the parent PR needs). It's still using the deprecated `GetTime`, but it's not clear to me what to replace it with.
It also changes the protocol name to `Noise_NX_Secp256k1+EllSwift_ChaChaPoly_SHA256` (https://github.com/stratum-mining/sv2-spec/pull/66#discussion_r1483090409).
(https://github.com/bitcoin/bitcoin/pull/29346#issuecomment-1935603779)
This latest push improves support for mock time (which the parent PR needs). It's still using the deprecated `GetTime`, but it's not clear to me what to replace it with.
It also changes the protocol name to `Noise_NX_Secp256k1+EllSwift_ChaChaPoly_SHA256` (https://github.com/stratum-mining/sv2-spec/pull/66#discussion_r1483090409).
💬 maflcko commented on pull request "lint: Check for missing bitcoin-config.h includes":
(https://github.com/bitcoin/bitcoin/pull/29408#discussion_r1484090380)
Nice, done. TIL about regex lookahead assertions.
(https://github.com/bitcoin/bitcoin/pull/29408#discussion_r1484090380)
Nice, done. TIL about regex lookahead assertions.
🤔 murchandamus reviewed a pull request: "wallet: Add CoinGrinder coin selection algorithm"
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1871848002)
Addressed all comments by @sipa and @achow101. Thanks for the review!
(https://github.com/bitcoin/bitcoin/pull/27877#pullrequestreview-1871848002)
Addressed all comments by @sipa and @achow101. Thanks for the review!
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1483991802)
This change just modifies the expected number of iterations and the `expected_result`. Before the last optimization CoinGrinder finds a suboptimal solution (1.8 BTC + 1 BTC), because it does not manage to exhaustively search the entire search space. With the "sand skipping" optimization, it skips over the tail of tiny UTXOs. This allows it to search all relevant combinations and find the preferred solution (1 BTC + 1 BTC).
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1483991802)
This change just modifies the expected number of iterations and the `expected_result`. Before the last optimization CoinGrinder finds a suboptimal solution (1.8 BTC + 1 BTC), because it does not manage to exhaustively search the entire search space. With the "sand skipping" optimization, it skips over the tail of tiny UTXOs. This allows it to search all relevant combinations and find the preferred solution (1 BTC + 1 BTC).
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1484040731)
I added all three checks you suggest as I understand them (some inequalities in different direction):
```diff
+ assert(result_cg->GetWeight() <= high_max_weight);
+ assert(result_cg->GetSelectedEffectiveValue() >= target + coin_params.m_min_change_target);
+ assert(best_weight < result_cg->GetWeight() || (best_weight == result_cg->GetWeight() && best_amount <= result_cg->GetSelectedEffectiveValue()));
```
Note that it is possible that CoinGrinder had found the b
...
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1484040731)
I added all three checks you suggest as I understand them (some inequalities in different direction):
```diff
+ assert(result_cg->GetWeight() <= high_max_weight);
+ assert(result_cg->GetSelectedEffectiveValue() >= target + coin_params.m_min_change_target);
+ assert(best_weight < result_cg->GetWeight() || (best_weight == result_cg->GetWeight() && best_amount <= result_cg->GetSelectedEffectiveValue()));
```
Note that it is possible that CoinGrinder had found the b
...
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1483999681)
I’ve improved the comment to clarify that this check is both used to highlight the improvements brought about by following optimizations and to catch future regressions.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1483999681)
I’ve improved the comment to clarify that this check is both used to highlight the improvements brought about by following optimizations and to catch future regressions.