💬 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.
💬 jnewbery commented on pull request "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg":
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1146327740)
Style nit: feel free to use a single line for single-line if blocks:
```c++
if (complete) node.MarkReceivedMsgsForProcessing(nReceiveFloodSize);
```
(https://github.com/bitcoin/bitcoin/pull/27257#discussion_r1146327740)
Style nit: feel free to use a single line for single-line if blocks:
```c++
if (complete) node.MarkReceivedMsgsForProcessing(nReceiveFloodSize);
```
💬 LarryRuane commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1146502076)
That's clever! They generate similar code, but this first way is slightly less code https://godbolt.org/z/Pz77TTEd8
```
int f(int bytes)
{
return bytes > 0 ? (bytes+8-1)/8 : 1;
}
```
Or https://godbolt.org/z/adzasoPad
```
int f(int bytes)
{
return (bytes+8-1)/8 + (bytes == 0);
}
```
That's using x86-64 gcc 11.3; results are similar for x86-64 clang 16.0.0.
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1146502076)
That's clever! They generate similar code, but this first way is slightly less code https://godbolt.org/z/Pz77TTEd8
```
int f(int bytes)
{
return bytes > 0 ? (bytes+8-1)/8 : 1;
}
```
Or https://godbolt.org/z/adzasoPad
```
int f(int bytes)
{
return (bytes+8-1)/8 + (bytes == 0);
}
```
That's using x86-64 gcc 11.3; results are similar for x86-64 clang 16.0.0.
✅ rebroad closed a pull request: "Correct poor grammar in wallet synchronization warning."
(https://github.com/bitcoin-core/gui/pull/720)
(https://github.com/bitcoin-core/gui/pull/720)
👋 MarcoFalke's pull request is ready for review: "test: getblock on header throws"
(https://github.com/bitcoin/bitcoin/pull/27237)
(https://github.com/bitcoin/bitcoin/pull/27237)
🚀 fanquake merged a pull request: "refactor, net: End friendship of CNode, CConnman and ConnmanTestMsg"
(https://github.com/bitcoin/bitcoin/pull/27257)
(https://github.com/bitcoin/bitcoin/pull/27257)
👍 ryanofsky approved a pull request: "refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it"
(https://github.com/bitcoin/bitcoin/pull/27256)
Code review ACK 79ee14bf1ea9530922e5d15edd35f9debf661998. Just added `static` and updated commit description since last review
(https://github.com/bitcoin/bitcoin/pull/27256)
Code review ACK 79ee14bf1ea9530922e5d15edd35f9debf661998. Just added `static` and updated commit description since last review
💬 ryanofsky commented on pull request "refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146540762)
re: https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146452144
> ```c++
> BOOST_CHECK_THROW(AmountFromValue(ParseNonRFCJSONValue(".19e-6")), std::runtime_error);
> ```
Wow, it didn't occur to me that AmountFromValue call on this line was useless because it was bypassed by the exception. I'm not sure if the person who wrote this test originally knew the AmountFromValue was being skipped, but it definitely looks to any reasonable observer like this line is intending to check whet
...
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146540762)
re: https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146452144
> ```c++
> BOOST_CHECK_THROW(AmountFromValue(ParseNonRFCJSONValue(".19e-6")), std::runtime_error);
> ```
Wow, it didn't occur to me that AmountFromValue call on this line was useless because it was bypassed by the exception. I'm not sure if the person who wrote this test originally knew the AmountFromValue was being skipped, but it definitely looks to any reasonable observer like this line is intending to check whet
...
💬 ryanofsky commented on pull request "refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it":
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146548353)
Makes sense, and fine if this is intentional, I just thought it was an accident.
(https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146548353)
Makes sense, and fine if this is intentional, I just thought it was an accident.
💬 martinus commented on pull request "Add pool based memory resource":
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1146560809)
The code generation is a bit different when you use unsigned types, then the `+ (bytes == 0)` version is the shortedst for me. In my microbenchmark the `+ (bytes == 0)` is also fastest, but in practice its most likely irrelevant
(https://github.com/bitcoin/bitcoin/pull/25325#discussion_r1146560809)
The code generation is a bit different when you use unsigned types, then the `+ (bytes == 0)` version is the shortedst for me. In my microbenchmark the `+ (bytes == 0)` is also fastest, but in practice its most likely irrelevant
💬 ryanofsky commented on pull request "init: Error if ignored bitcoin.conf file is found":
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1146611376)
re: 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?
There may be other use cases, but the backwards compatibility use-case I had in mind is where someone has a bitcoi
...
(https://github.com/bitcoin/bitcoin/pull/27302#discussion_r1146611376)
re: 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?
There may be other use cases, but the backwards compatibility use-case I had in mind is where someone has a bitcoi
...
💬 achow101 commented on pull request "refactor: Remove CAddressBookData::destdata":
(https://github.com/bitcoin/bitcoin/pull/27224#issuecomment-1481673419)
ACK fe367f2e4e5fb95546fa26491d43346e0bc11adb
(https://github.com/bitcoin/bitcoin/pull/27224#issuecomment-1481673419)
ACK fe367f2e4e5fb95546fa26491d43346e0bc11adb
💬 stickies-v commented on pull request "refactor: rpc: Remove unnecessary uses of ParseNonRFCJSONValue() and rename it":
(https://github.com/bitcoin/bitcoin/pull/27256#issuecomment-1481691410)
Force pushed to [add test a test case on `AmountFromValue()`](https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146540762) that was previously not reached but probably intended.
Addressed all of @ryanofsky's comments - thank you for the review!
(https://github.com/bitcoin/bitcoin/pull/27256#issuecomment-1481691410)
Force pushed to [add test a test case on `AmountFromValue()`](https://github.com/bitcoin/bitcoin/pull/27256#discussion_r1146540762) that was previously not reached but probably intended.
Addressed all of @ryanofsky's comments - thank you for the review!
⚠️ ryanofsky opened an issue: "wallet_create_tx.py "Not solvable pre-selected input" exception"
(https://github.com/bitcoin/bitcoin/issues/27316)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
CI test failure https://cirrus-ci.com/task/5315879898972160?logs=functional_tests#L2675:
```
test 2023-03-23T17:23:41.645000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
self.run_test()
File "C:\Users\Con
...
(https://github.com/bitcoin/bitcoin/issues/27316)
### Is there an existing issue for this?
- [X] I have searched the existing issues
### Current behaviour
CI test failure https://cirrus-ci.com/task/5315879898972160?logs=functional_tests#L2675:
```
test 2023-03-23T17:23:41.645000Z TestFramework (ERROR): JSONRPC error
Traceback (most recent call last):
File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
self.run_test()
File "C:\Users\Con
...