🤔 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!

💬 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).

💬 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
...

💬 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.

💬 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.

💬 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

💬 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.

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(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

💬 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
...

💬 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

💬 maflcko commented on pull request "test, assumeutxo: Add test to ensure failure when mempool not empty":
(https://github.com/bitcoin/bitcoin/pull/29394#issuecomment-1935661253)
lgtm ACK da13cc2c8010cbccf37324cca4403cb346ecdd30

Thanks

💬 hebasto commented on pull request "doc: Update translation process guide":
(https://github.com/bitcoin/bitcoin/pull/29414#discussion_r1484148120)
I agree. The part regarding to the `~/.transifexrc` file is outdated as well and might be dropped. This file is generated automatically after providing a token during the first run:
```
API token not found. Please provide it and it will be saved in '~/.transifexrc'.
If you don't have an API token, you can generate one in https://app.transifex.com/user/settings/api/
>
```

The autogenerated `~/.transifexrc` file looks like that:
```
$ cat ~/.transifexrc
[https://app.transifex.com]
res
...

💬 maflcko commented on pull request "net: Use serialization parameters for CAddress serialization":
(https://github.com/bitcoin/bitcoin/pull/25284#discussion_r1484176865)
Fixed in the first commit of https://github.com/bitcoin/bitcoin/pull/28929

💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#issuecomment-1935771317)
nit: `retreive` -> `retrieve` in the first commit msg

🤔 maflcko reviewed a pull request: "serialization: Support for multiple parameters"
(https://github.com/bitcoin/bitcoin/pull/28929#pullrequestreview-1872178867)
did not review the last commit 3c311734d2fc6a4ca410f254ba3c8e923d58be70 🚔

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: did
...

💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1484199895)
3c311734d2fc6a4ca410f254ba3c8e923d58be70:

I don't understand this. For example, if `SubStream` is a `DataStream` that holds data, this will now create a copy of this data, when previously it didn't?

It seems odd that the stream has to be passed down the whole "type-stack" anyway. Maybe it is simpler to just create a stack of params only to hold the types?

💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1484226391)
One could consider to completely disallow wrapping streams into each other, see also https://www.github.com/bitcoin/bitcoin/pull/25284/commits/faec591d64e40ba7ec7656cbfdda1a05953bde13#r1315927911

💬 brunoerg commented on pull request "fuzz: increase length of string used for `NetWhitelist{bind}Permissions::TryParse`":
(https://github.com/bitcoin/bitcoin/pull/29413#issuecomment-1935808729)
> If a length of 32 is insufficient, why did you choose 1000 vs 100 or 10,000?

Because 1000 is a good value considering current possible combinations of permissions and addresses. 100 is not enough and bigger values like 10,000 might be a waste since these flags are set by users and can't be explored by third parties.

🤔 furszy reviewed a pull request: "wallet: batch erase procedures and improve 'EraseRecords' performance"
(https://github.com/bitcoin/bitcoin/pull/29403#pullrequestreview-1871240281)
Updated per feedback. Thanks both!