💬 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
💬 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
(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
...
(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
(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
(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
...
(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?
(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
(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.
(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!
(https://github.com/bitcoin/bitcoin/pull/29403#pullrequestreview-1871240281)
Updated per feedback. Thanks both!
💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483587941)
> In [5235257](https://github.com/bitcoin/bitcoin/commit/523525707742fc620039cd67e8f1437f7f613a01) "wallet: bdb batch 'ErasePrefix', do not create txn internally"
>
> nit: Could use `RunWithinTxn()`.
I did not used it because it needs `walletdb.h` dependency (`RunWithinTxn` provides access to `WalletBatch`). And this tests (`db_tests.cpp`) run on a lower level, using `DatabaseBatch`.
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483587941)
> In [5235257](https://github.com/bitcoin/bitcoin/commit/523525707742fc620039cd67e8f1437f7f613a01) "wallet: bdb batch 'ErasePrefix', do not create txn internally"
>
> nit: Could use `RunWithinTxn()`.
I did not used it because it needs `walletdb.h` dependency (`RunWithinTxn` provides access to `WalletBatch`). And this tests (`db_tests.cpp`) run on a lower level, using `DatabaseBatch`.
💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483579553)
> I think it would be relevant to mention that BDB returns errors on del() if it is not in a transaction.
Sure, we could mention that the read-write operation is currently disallowed by our current bdb configurations.
Still, JFYI, I fixed it locally by subdividing the read-write operations. However, since we currently ensure that all callers start a transaction, and this division adds an extra loop over the matching entries (see https://github.com/furszy/bitcoin-core/commit/4e7b5c02837b6b3
...
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483579553)
> I think it would be relevant to mention that BDB returns errors on del() if it is not in a transaction.
Sure, we could mention that the read-write operation is currently disallowed by our current bdb configurations.
Still, JFYI, I fixed it locally by subdividing the read-write operations. However, since we currently ensure that all callers start a transaction, and this division adds an extra loop over the matching entries (see https://github.com/furszy/bitcoin-core/commit/4e7b5c02837b6b3
...
💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483617478)
> In [757d651](https://github.com/bitcoin/bitcoin/commit/757d6516db56e2732ea77624e66025446cb816a4) "wallet: simplify EraseRecords by using 'ErasePrefix'"
>
> This overload feels unnecessary given that there are only 2 places this function is really used.
The idea is to use this function as a top-level wrapper for wallet functions and scope the other function for internal `walletdb.cpp` usage mostly/only. This approach avoids creating `WalletBatch` objects across the sources and makes use o
...
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483617478)
> In [757d651](https://github.com/bitcoin/bitcoin/commit/757d6516db56e2732ea77624e66025446cb816a4) "wallet: simplify EraseRecords by using 'ErasePrefix'"
>
> This overload feels unnecessary given that there are only 2 places this function is really used.
The idea is to use this function as a top-level wrapper for wallet functions and scope the other function for internal `walletdb.cpp` usage mostly/only. This approach avoids creating `WalletBatch` objects across the sources and makes use o
...
💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483619574)
> This could be done in the commit introducing `RunWithinTxn()` rather than changing it in a later commit.
This is merely an organizational discussion, but.. I'm not a big fan of introducing a function overload in a commit that does not utilizes it. Yet, if you have a strong opinion about it, I could move this to the first commit. np.
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1483619574)
> This could be done in the commit introducing `RunWithinTxn()` rather than changing it in a later commit.
This is merely an organizational discussion, but.. I'm not a big fan of introducing a function overload in a commit that does not utilizes it. Yet, if you have a strong opinion about it, I could move this to the first commit. np.