💬 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.
💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1484235425)
> Maybe use string_view instead of string, since in most cases this is just passed a string literal, so no need to create a full string object
Sure!
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1484235425)
> Maybe use string_view instead of string, since in most cases this is just passed a string literal, so no need to create a full string object
Sure!
💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1484242585)
Sure!, done.
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1484242585)
Sure!, done.
💬 furszy commented on pull request "wallet: batch erase procedures and improve 'EraseRecords' performance":
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1484242772)
sure
(https://github.com/bitcoin/bitcoin/pull/29403#discussion_r1484242772)
sure
💬 theStack commented on pull request "rpc: parse legacy pubkeys consistently with specific error messages":
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1484271021)
Good point. For legacy addresses on mainnet, it's very unlikely, but indeed theoretically possible to have a valid address that `IsHex`, i.e. consists of an even number of hex characters. One example for this is 111111111111111111126C31DfD9, found with the following Python script using our test framework:
```python
#!/usr/bin/env python3
from test_framework.address import keyhash_to_p2pkh
for i in range(100000):
addr = keyhash_to_p2pkh(i.to_bytes(20, 'big'), main=True)
try:
...
(https://github.com/bitcoin/bitcoin/pull/28336#discussion_r1484271021)
Good point. For legacy addresses on mainnet, it's very unlikely, but indeed theoretically possible to have a valid address that `IsHex`, i.e. consists of an even number of hex characters. One example for this is 111111111111111111126C31DfD9, found with the following Python script using our test framework:
```python
#!/usr/bin/env python3
from test_framework.address import keyhash_to_p2pkh
for i in range(100000):
addr = keyhash_to_p2pkh(i.to_bytes(20, 'big'), main=True)
try:
...
💬 cornwarecjp commented on issue "Incorrect amount in transaction page":
(https://github.com/bitcoin/bitcoin/issues/29378#issuecomment-1935867840)
ACK, it's fixed: in version 26, the transaction shows as +0.0009 BTC, and the total of the transaction amounts equals the wallet balance.
Note to future readers of this issue: when you add up the amounts in the CSV file, don't forget to skip the amounts of the non-confirmed transactions (e.g. ones that have been RBFed).
(https://github.com/bitcoin/bitcoin/issues/29378#issuecomment-1935867840)
ACK, it's fixed: in version 26, the transaction shows as +0.0009 BTC, and the total of the transaction amounts equals the wallet balance.
Note to future readers of this issue: when you add up the amounts in the CSV file, don't forget to skip the amounts of the non-confirmed transactions (e.g. ones that have been RBFed).
👍 vasild approved a pull request: "fuzz: increase length of string used for `NetWhitelist{bind}Permissions::TryParse`"
(https://github.com/bitcoin/bitcoin/pull/29413#pullrequestreview-1872299532)
ACK 864e2e9097de8f1fda63137f803687dd5cc96c03
(https://github.com/bitcoin/bitcoin/pull/29413#pullrequestreview-1872299532)
ACK 864e2e9097de8f1fda63137f803687dd5cc96c03
💬 brunoerg commented on pull request "p2p: Allow whitelisting manual connections":
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1935886953)
Force-pushed addressing:
- https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1480525652
- https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1480624819
- https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1481727947
- https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1483435534
Thanks @sr-gi for reviewing!
(https://github.com/bitcoin/bitcoin/pull/27114#issuecomment-1935886953)
Force-pushed addressing:
- https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1480525652
- https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1480624819
- https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1481727947
- https://github.com/bitcoin/bitcoin/pull/27114#discussion_r1483435534
Thanks @sr-gi for reviewing!
💬 ryanofsky commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1484292817)
re: https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1484199895
> [3c31173](https://github.com/bitcoin/bitcoin/commit/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?
I will add a comment here, but this does not happen due to the "Template deduction guide for a single params argument that's slightly different from the default
...
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1484292817)
re: https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1484199895
> [3c31173](https://github.com/bitcoin/bitcoin/commit/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?
I will add a comment here, but this does not happen due to the "Template deduction guide for a single params argument that's slightly different from the default
...
💬 jadijadi commented on issue "Weird focus rect displayed on inital sync":
(https://github.com/bitcoin-core/gui/issues/783#issuecomment-1935930424)
Tried to reproduce this on 26.9.9 with no successful.
May I ask how you produced the "expected result" image? what do you do to "fix" the issue? And is there a way to "show/hide" the strange rectangular? I can see that in the problematic photo you have the Hide button focused too. Does losing the focus hides the strange highlight square?
(https://github.com/bitcoin-core/gui/issues/783#issuecomment-1935930424)
Tried to reproduce this on 26.9.9 with no successful.
May I ask how you produced the "expected result" image? what do you do to "fix" the issue? And is there a way to "show/hide" the strange rectangular? I can see that in the problematic photo you have the Hide button focused too. Does losing the focus hides the strange highlight square?
💬 maflcko commented on pull request "serialization: Support for multiple parameters":
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1484322490)
In any case `m_substream{substream}` still creates a copy when it shouldn't, due to a missing `std::forward`, no?
(https://github.com/bitcoin/bitcoin/pull/28929#discussion_r1484322490)
In any case `m_substream{substream}` still creates a copy when it shouldn't, due to a missing `std::forward`, no?