💬 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.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1484076997)
> Also, can you drop the `!best_selection.empty() &&` here?
No, dropping the `!best_selection_empty()` will prevent CoinGrinder from returning the `max_tx_weight_exceeded` error where expected. I fixed the rounding up as you described.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1484076997)
> Also, can you drop the `!best_selection.empty() &&` here?
No, dropping the `!best_selection_empty()` will prevent CoinGrinder from returning the `max_tx_weight_exceeded` error where expected. I fixed the rounding up as you described.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1483986603)
I added a comment with the calculation
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1483986603)
I added a comment with the calculation
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1484028065)
Yes, that would be possible. It looks like it would be the "4, 13" from the light coins and the "14" from the medium coins that should get found. I added a check.
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1484028065)
Yes, that would be possible. It looks like it would be the "4, 13" from the light coins and the "14" from the medium coins that should get found. I added a check.
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1484030906)
Fixed
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1484030906)
Fixed
💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1483995322)
Fixed
(https://github.com/bitcoin/bitcoin/pull/27877#discussion_r1483995322)
Fixed
💬 dergoegge commented on pull request "p2p: Don't process mutated blocks":
(https://github.com/bitcoin/bitcoin/pull/29412#issuecomment-1935643125)
> How do you define a mutated block? What are the known forms of mutated blocks?
Looking at `IsBlockMutated` in this PR should provide the answers for these questions, but to recap:
* When we receive a block it contains a header and a list of transactions. The header contains a merkle root hash (`CBlock::hashMerkleRoot`) which should commit to the list of transactions but [if it doesn't the block is considered mutated](https://github.com/bitcoin/bitcoin/blob/b2b2b1e9e415c3b5f74d517eaebfc20
...
(https://github.com/bitcoin/bitcoin/pull/29412#issuecomment-1935643125)
> How do you define a mutated block? What are the known forms of mutated blocks?
Looking at `IsBlockMutated` in this PR should provide the answers for these questions, but to recap:
* When we receive a block it contains a header and a list of transactions. The header contains a merkle root hash (`CBlock::hashMerkleRoot`) which should commit to the list of transactions but [if it doesn't the block is considered mutated](https://github.com/bitcoin/bitcoin/blob/b2b2b1e9e415c3b5f74d517eaebfc20
...
💬 maflcko commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1484122214)
I checked this and the test would fail if the early return errors would be shuffled. node0 returns:
```
- 2024-02-09T10:15:58.291593Z (mocktime: 2011-02-02T23:17:17Z) [httpworker.3] [validation.cpp:5392] [PopulateAndValidateSnapshot] [snapshot] activation failed - work does not exceed active chainstate
(https://github.com/bitcoin/bitcoin/pull/29394#discussion_r1484122214)
I checked this and the test would fail if the early return errors would be shuffled. node0 returns:
```
- 2024-02-09T10:15:58.291593Z (mocktime: 2011-02-02T23:17:17Z) [httpworker.3] [validation.cpp:5392] [PopulateAndValidateSnapshot] [snapshot] activation failed - work does not exceed active chainstate