👍 ryanofsky approved a pull request: "refactor: rpc: remove ParseNonRFCJSONValue()"
(https://github.com/bitcoin/bitcoin/pull/27256)
Code review ACK 860e70713c2c4a13f9cb108046669ad9fdb02331. This looks good, but we will have to find another function to give the most confusing function name award to. I left a few minor review suggestions, but they can be ignored.
(https://github.com/bitcoin/bitcoin/pull/27256)
Code review ACK 860e70713c2c4a13f9cb108046669ad9fdb02331. This looks good, but we will have to find another function to give the most confusing function name award to. I left a few minor review suggestions, but they can be ignored.
💬 ryanofsky commented on pull request "refactor: rpc: remove ParseNonRFCJSONValue()":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146386553)
In commit "test: move coverage on ParseNonRFCJSONValue() to UniValue::read()" (6e28dedcd6fe44b84ddfac71f3325305e5219885)
I don't think you should delete these two lines entirely, because doing that really seems to lose test coverage for the `AmountFromValue` function despite the claim in the commit description. I would keep this two lines, but move them to the previous test and replace `ParseNonRFCJSONValue` with `ValueFromString`.
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146386553)
In commit "test: move coverage on ParseNonRFCJSONValue() to UniValue::read()" (6e28dedcd6fe44b84ddfac71f3325305e5219885)
I don't think you should delete these two lines entirely, because doing that really seems to lose test coverage for the `AmountFromValue` function despite the claim in the commit description. I would keep this two lines, but move them to the previous test and replace `ParseNonRFCJSONValue` with `ValueFromString`.
💬 ryanofsky commented on pull request "refactor: rpc: remove ParseNonRFCJSONValue()":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146414393)
In commit "refactor: rpc: remove ParseNonRFCJSONValue() function" (860e70713c2c4a13f9cb108046669ad9fdb02331)
This commit description seems misleading (or maybe it is out of date?). The commit is not removing the `ParseNonRFCJSONValue()` function, just renaming it and making it private.
I think a better name for the commit would be something like "Hide and rename ParseNonRFCJSONValue()", and a better name for the PR would be "Remove unnecessary uses of ParseNonRFCJSONValue() and rename it".
...
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146414393)
In commit "refactor: rpc: remove ParseNonRFCJSONValue() function" (860e70713c2c4a13f9cb108046669ad9fdb02331)
This commit description seems misleading (or maybe it is out of date?). The commit is not removing the `ParseNonRFCJSONValue()` function, just renaming it and making it private.
I think a better name for the commit would be something like "Hide and rename ParseNonRFCJSONValue()", and a better name for the PR would be "Remove unnecessary uses of ParseNonRFCJSONValue() and rename it".
...
💬 ryanofsky commented on pull request "refactor: rpc: remove ParseNonRFCJSONValue()":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146389196)
In commit "test: move coverage on ParseNonRFCJSONValue() to UniValue::read()" (6e28dedcd6fe44b84ddfac71f3325305e5219885)
Test order seems to have changed unintentionally. Would be good to keep the 19e-6 test before the 1e+30 for easier comparison.
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146389196)
In commit "test: move coverage on ParseNonRFCJSONValue() to UniValue::read()" (6e28dedcd6fe44b84ddfac71f3325305e5219885)
Test order seems to have changed unintentionally. Would be good to keep the 19e-6 test before the 1e+30 for easier comparison.
💬 ryanofsky commented on pull request "refactor: rpc: remove ParseNonRFCJSONValue()":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146418012)
In commit "refactor: rpc: remove ParseNonRFCJSONValue() function" (860e70713c2c4a13f9cb108046669ad9fdb02331)
Probably should add `static` to avoid exporting a linker symbol that could clash with a `Parse(string_view)` function in another translation unit.
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146418012)
In commit "refactor: rpc: remove ParseNonRFCJSONValue() function" (860e70713c2c4a13f9cb108046669ad9fdb02331)
Probably should add `static` to avoid exporting a linker symbol that could clash with a `Parse(string_view)` function in another translation unit.
💬 willcl-ark commented on issue "Running RPC call importmulti with a xpub key with very large range results in a timeout error":
(https://github.com/bitcoin/bitcoin/issues/17797#issuecomment-1481495736)
@chris-belcher have you tried this with the new native descriptor wallet from [April 2020](https://github.com/bitcoin/bitcoin/pull/16528)? I have just tried to reproduce* and importing the range of 999,999 took 1m25s (and therefore didn't trigger the http timeout you experienced):
```fish
will@ubuntu in ~/.bitcoin
₿ /home/will/src/bitcoin/src/bitcoin-cli createwallet "test-wallet-disable-privkeys" true
{
"name": "test-wallet-disable-privkeys",
"warning": ""
}
will@ubuntu in ~/.bi
...
(https://github.com/bitcoin/bitcoin/issues/17797#issuecomment-1481495736)
@chris-belcher have you tried this with the new native descriptor wallet from [April 2020](https://github.com/bitcoin/bitcoin/pull/16528)? I have just tried to reproduce* and importing the range of 999,999 took 1m25s (and therefore didn't trigger the http timeout you experienced):
```fish
will@ubuntu in ~/.bitcoin
₿ /home/will/src/bitcoin/src/bitcoin-cli createwallet "test-wallet-disable-privkeys" true
{
"name": "test-wallet-disable-privkeys",
"warning": ""
}
will@ubuntu in ~/.bi
...
💬 jnewbery commented on pull request "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg":
(https://github.com/bitcoin/bitcoin/pull/27257#issuecomment-1481496992)
utACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e
I didn't verify myself that the new code doesn't result in a copy of `CNetMessage`, but I trust that you've tested that. A follow up PR could potentially delete the default copy constructor for `CNetMessage` to ensure that we never copy.
All of my review comments are nits/style comments and shouldn't stop this from being merged.
(https://github.com/bitcoin/bitcoin/pull/27257#issuecomment-1481496992)
utACK 3566aa7d495bb783bbd135726238d9f2a9e9f80e
I didn't verify myself that the new code doesn't result in a copy of `CNetMessage`, but I trust that you've tested that. A follow up PR could potentially delete the default copy constructor for `CNetMessage` to ensure that we never copy.
All of my review comments are nits/style comments and shouldn't stop this from being merged.
💬 stickies-v commented on pull request "refactor: rpc: remove ParseNonRFCJSONValue()":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146452144)
> BOOST_CHECK_THROW(AmountFromValue(ParseNonRFCJSONValue(".19e-6")), std::runtime_error);
Unless I misunderstand, `AmountFromValue` is not even called because `ParseNonRFCJSONValue` already throws first, so I don't think we lose any coverage here? And the happy path is already tested at the moment:
```
BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.19e-6")), 19);
```
> BOOST_CHECK_EQUAL(AmountFromValue(ParseNonRFCJSONValue("0.00000000000000000000000000000000000001e+30 ")), 1);
...
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146452144)
> BOOST_CHECK_THROW(AmountFromValue(ParseNonRFCJSONValue(".19e-6")), std::runtime_error);
Unless I misunderstand, `AmountFromValue` is not even called because `ParseNonRFCJSONValue` already throws first, so I don't think we lose any coverage here? And the happy path is already tested at the moment:
```
BOOST_CHECK_EQUAL(AmountFromValue(ValueFromString("0.19e-6")), 19);
```
> BOOST_CHECK_EQUAL(AmountFromValue(ParseNonRFCJSONValue("0.00000000000000000000000000000000000001e+30 ")), 1);
...
💬 vincenzopalazzo commented on issue "core stops to run with `Failed to read block` error":
(https://github.com/bitcoin/bitcoin/issues/27142#issuecomment-1481499943)
@willcl-ark after the downgrade to the tagged version I have not try it again, sorry!
(https://github.com/bitcoin/bitcoin/issues/27142#issuecomment-1481499943)
@willcl-ark after the downgrade to the tagged version I have not try it again, sorry!
💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1481501701)
Updated 780c696fc310a6df8464b40983b23f8b0a3074f0 -> 53d99551e913bfb85769a2b34fed73898779ff0f ([`pr/ignoredconf.4`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.4) -> [`pr/ignoredconf.5`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.5), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ignoredconf.4..pr/ignoredconf.5)) to try to fix another CI error on win64:
```
Traceback (most recent call last):
File "C:\Users\ContainerAdministrator\AppData\Local\Tem
...
(https://github.com/bitcoin/bitcoin/pull/27302#issuecomment-1481501701)
Updated 780c696fc310a6df8464b40983b23f8b0a3074f0 -> 53d99551e913bfb85769a2b34fed73898779ff0f ([`pr/ignoredconf.4`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.4) -> [`pr/ignoredconf.5`](https://github.com/ryanofsky/bitcoin/commits/pr/ignoredconf.5), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/ignoredconf.4..pr/ignoredconf.5)) to try to fix another CI error on win64:
```
Traceback (most recent call last):
File "C:\Users\ContainerAdministrator\AppData\Local\Tem
...
💬 stickies-v commented on pull request "refactor: rpc: remove ParseNonRFCJSONValue()":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146457650)
The change is intentional, I'm now first doing the valid tests followed by the one invalid test, making it consistent with the docs too. I think this is easier to understand? But I don't care too much either way, happy to change the order (and add some more docs) if you think that makes it easier.
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146457650)
The change is intentional, I'm now first doing the valid tests followed by the one invalid test, making it consistent with the docs too. I think this is easier to understand? But I don't care too much either way, happy to change the order (and add some more docs) if you think that makes it easier.
💬 stickies-v commented on pull request "refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146469854)
Thanks, I've adopted your phrasing for both commit and PR description.
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146469854)
Thanks, I've adopted your phrasing for both commit and PR description.
💬 stickies-v commented on pull request "refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146470447)
Good point especially for such a generic name, made it `static`, thanks.
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146470447)
Good point especially for such a generic name, made it `static`, thanks.
💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1481521004)
Thank you for the discussion and suggestions.
Updated 9f5600742c65a2421c9fe5a2a2670e86a25ef696 -> 97bf70119e5b8567bcdc553d59b18a09023fd05a ([removeBlockstorageArgs_10](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_10) -> [removeBlockstorageArgs_11](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_11), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_10..removeBlockstorageArgs_11)) to add @dergoegge's patch as suggested i
...
(https://github.com/bitcoin/bitcoin/pull/27125#issuecomment-1481521004)
Thank you for the discussion and suggestions.
Updated 9f5600742c65a2421c9fe5a2a2670e86a25ef696 -> 97bf70119e5b8567bcdc553d59b18a09023fd05a ([removeBlockstorageArgs_10](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_10) -> [removeBlockstorageArgs_11](https://github.com/TheCharlatan/bitcoin/tree/removeBlockstorageArgs_11), [compare](https://github.com/TheCharlatan/bitcoin/compare/removeBlockstorageArgs_10..removeBlockstorageArgs_11)) to add @dergoegge's patch as suggested i
...
💬 willcl-ark commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1146473010)
Whilst I appreciate that this is a simple and elegant way to remain backwards-compatible, I can't help but wonder if it's something we actually want to support with a new option; what is the use-case here? The operator either has to modify their current config with this new flag, or... fix their config?
If the operator is going to fix something up, then I think it should be the latter!
Perhaps I am missing a use-case though where they would want this old config to be purposefully ignored?
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1146473010)
Whilst I appreciate that this is a simple and elegant way to remain backwards-compatible, I can't help but wonder if it's something we actually want to support with a new option; what is the use-case here? The operator either has to modify their current config with this new flag, or... fix their config?
If the operator is going to fix something up, then I think it should be the latter!
Perhaps I am missing a use-case though where they would want this old config to be purposefully ignored?
💬 TheCharlatan commented on pull request "refactor, kernel: Decouple ArgsManager from blockstorage":
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1146473683)
This patch is much simpler than what I came up with, thanks a lot!
(https://github.com/bitcoin/bitcoin/pull/27125#discussion_r1146473683)
This patch is much simpler than what I came up with, thanks a lot!
💬 furszy commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1146476492)
isn't this assertion redundant?
If `iter` would be equal to `end()`, then `to_process.empty()` would be true. Which breaks the loop.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1146476492)
isn't this assertion redundant?
If `iter` would be equal to `end()`, then `to_process.empty()` would be true. Which breaks the loop.
💬 TheCharlatan commented on pull request "clang-tidy: Add more `performance-*` checks and related fixes":
(https://github.com/bitcoin/bitcoin/pull/26642#issuecomment-1481537091)
re-ACK [09383b6](https://github.com/bitcoin/bitcoin/pull/26642/commits/09383b612216b63f2aec0f1fffc889989c66f212)
Locally, I'm also getting a bunch of (though still on clang-14):
```
error: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy,-warnings-as-errors]
```
after running:
```
bear --config src/.bear-tidy-config -- make -j $(nproc)
cd ./src/ && run-clang-tidy -j $(nproc)
```
(https://github.com/bitcoin/bitcoin/pull/26642#issuecomment-1481537091)
re-ACK [09383b6](https://github.com/bitcoin/bitcoin/pull/26642/commits/09383b612216b63f2aec0f1fffc889989c66f212)
Locally, I'm also getting a bunch of (though still on clang-14):
```
error: loop variable is copied but only used as const reference; consider making it a const reference [performance-for-range-copy,-warnings-as-errors]
```
after running:
```
bear --config src/.bear-tidy-config -- make -j $(nproc)
cd ./src/ && run-clang-tidy -j $(nproc)
```
💬 jnewbery commented on pull request "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg":
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1146305999)
(for follow up) it'd be nice to remove the `recv_flood_size` parameter here:
- Add a `const size_t m_receive_flood_size` member to `CNode` and set it during construction.
- Remove this parameter and use `m_receive_flood_size` for the comparison in this function.
- Remove the `GetReceiveFloodSize()` function from `CConnman`.
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1146305999)
(for follow up) it'd be nice to remove the `recv_flood_size` parameter here:
- Add a `const size_t m_receive_flood_size` member to `CNode` and set it during construction.
- Remove this parameter and use `m_receive_flood_size` for the comparison in this function.
- Remove the `GetReceiveFloodSize()` function from `CConnman`.
💬 jnewbery commented on pull request "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg":
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1146296559)
C++ Core Guidelines suggest avoiding trivial getters: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c131-avoid-trivial-getters-and-setters
Here, `m_conn_type` is const, so it can just be made public and read from outside the class.
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1146296559)
C++ Core Guidelines suggest avoiding trivial getters: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#c131-avoid-trivial-getters-and-setters
Here, `m_conn_type` is const, so it can just be made public and read from outside the class.