💬 MarcoFalke commented on pull request "clang-tidy: Fix `modernize-use-default-member-init` in headers and force to check all headers":
(https://github.com/bitcoin/bitcoin/pull/26705#discussion_r1126213761)
Fixed in clang-tidy-17, see https://github.com/llvm/llvm-project/commit/fa491fefb0f862b653b196eb61f0e690b0b71460
(https://github.com/bitcoin/bitcoin/pull/26705#discussion_r1126213761)
Fixed in clang-tidy-17, see https://github.com/llvm/llvm-project/commit/fa491fefb0f862b653b196eb61f0e690b0b71460
👍 TheCharlatan approved a pull request: "clang-tidy: Add more `performance-*` checks and related fixes"
(https://github.com/bitcoin/bitcoin/pull/26642)
ACK 7db868b49b60de371d01cda1f8e5faaa0e18f822
I reproduced all changes here by running `clang-tidy` locally. They all seem like a clear improvement to me.
(https://github.com/bitcoin/bitcoin/pull/26642)
ACK 7db868b49b60de371d01cda1f8e5faaa0e18f822
I reproduced all changes here by running `clang-tidy` locally. They all seem like a clear improvement to me.
💬 TheCharlatan commented on pull request "refactor: Use move semantics instead of custom swap functions":
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1455960808)
Light code review ACK 142718890e718397e0026c315c8940102b9657ce
> I mostly reviewed the overall PR diff and didn't review individual commits in detail since it was easier to think about class definitions with only move operations or only swap operations, not combinations of both.
I can echo this, reviewing this piecemeal was a bit more difficult than just looking at it all at once.
(https://github.com/bitcoin/bitcoin/pull/26749#issuecomment-1455960808)
Light code review ACK 142718890e718397e0026c315c8940102b9657ce
> I mostly reviewed the overall PR diff and didn't review individual commits in detail since it was easier to think about class definitions with only move operations or only swap operations, not combinations of both.
I can echo this, reviewing this piecemeal was a bit more difficult than just looking at it all at once.
💬 naumenkogs commented on pull request "net, refactor: net_processing, add `ProcessCompactBlockTxns`":
(https://github.com/bitcoin/bitcoin/pull/26969#discussion_r1126306044)
nevermind.
(https://github.com/bitcoin/bitcoin/pull/26969#discussion_r1126306044)
nevermind.
💬 hebasto commented on pull request "Handle invalid hex encoding in ParseHex":
(https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1456009757)
This PR causes cross compiling errors for some hosts using default toolchains on Ubuntu 22.04.
For example, for `riscv64-linux-gnu`
```
$ make -C src bitcoind
...
CXXLD bitcoind
/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: libbitcoin_node.a(libbitcoin_node_a-net_processing.o): in function `.L11743':
net_processing.cpp:(.text+0x1cc82): undefined reference to `std::optional<std::vector<unsigned char, std::allocator<unsigned char> > > TryParseHex<unsig
...
(https://github.com/bitcoin/bitcoin/pull/25227#issuecomment-1456009757)
This PR causes cross compiling errors for some hosts using default toolchains on Ubuntu 22.04.
For example, for `riscv64-linux-gnu`
```
$ make -C src bitcoind
...
CXXLD bitcoind
/usr/lib/gcc-cross/riscv64-linux-gnu/11/../../../../riscv64-linux-gnu/bin/ld: libbitcoin_node.a(libbitcoin_node_a-net_processing.o): in function `.L11743':
net_processing.cpp:(.text+0x1cc82): undefined reference to `std::optional<std::vector<unsigned char, std::allocator<unsigned char> > > TryParseHex<unsig
...
💬 hebasto commented on pull request "util: add missing include and fix function signature":
(https://github.com/bitcoin/bitcoin/pull/27192#issuecomment-1456026433)
Post-merge ACK.
(https://github.com/bitcoin/bitcoin/pull/27192#issuecomment-1456026433)
Post-merge ACK.
💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1126270559)
> Is m_ready_to_calculate an error indication, since it seems it can be false only If we hit the DoS limit?
No it's not just for indicating DoS limit reached, notice that it is also set to false after `BuildMockTemplate` is called. It prevents somebody from constructing a `MiniMiner` and calling `CalculateBumpFees()` or `CalculateTotalBumpFees()` multiple times, which could result in very incorrect results if the target feerate changes.
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1126270559)
> Is m_ready_to_calculate an error indication, since it seems it can be false only If we hit the DoS limit?
No it's not just for indicating DoS limit reached, notice that it is also set to false after `BuildMockTemplate` is called. It prevents somebody from constructing a `MiniMiner` and calling `CalculateBumpFees()` or `CalculateTotalBumpFees()` multiple times, which could result in very incorrect results if the target feerate changes.
💬 glozow commented on pull request "Implement Mini version of BlockAssembler to calculate mining scores":
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1126320439)
nit: It seems the rename to `GatherClusters` should have been squashed into efeee8b51d, but is in 5ee6f89e40 instead?
(https://github.com/bitcoin/bitcoin/pull/27021#discussion_r1126320439)
nit: It seems the rename to `GatherClusters` should have been squashed into efeee8b51d, but is in 5ee6f89e40 instead?
📝 MarcoFalke opened a pull request: "ci: Remove unused EXPECTED_TESTS_DURATION_IN_SECONDS env var"
(https://github.com/bitcoin/bitcoin/pull/27209)
Remove long unused travis leftover
(https://github.com/bitcoin/bitcoin/pull/27209)
Remove long unused travis leftover
👍 fanquake approved a pull request: "ci: Remove unused EXPECTED_TESTS_DURATION_IN_SECONDS env var"
(https://github.com/bitcoin/bitcoin/pull/27209)
ACK 3fffff50f625ed5e3c45139b3ae874f63f121a1e
(https://github.com/bitcoin/bitcoin/pull/27209)
ACK 3fffff50f625ed5e3c45139b3ae874f63f121a1e
💬 willcl-ark commented on pull request "util: Use steady clock in SeedStrengthen, FindBestImplementation, FlushStateToDisk":
(https://github.com/bitcoin/bitcoin/pull/27189#issuecomment-1456083159)
Concept ACK for moving to monotonic clocks.
Am I correct in my understanding that there are no current tests/benchmarks which are calling these while adjusting the system clock, but rather this is _additional_ hardening for a user whose system clock happens to be changed _while_ one of these is executing?
I couldn't easily find any callsites which might be directly affected by this, only indirectly. i.e. The system clock happens to change during a call to `GetRandHash()` or `GetRandBytes()
...
(https://github.com/bitcoin/bitcoin/pull/27189#issuecomment-1456083159)
Concept ACK for moving to monotonic clocks.
Am I correct in my understanding that there are no current tests/benchmarks which are calling these while adjusting the system clock, but rather this is _additional_ hardening for a user whose system clock happens to be changed _while_ one of these is executing?
I couldn't easily find any callsites which might be directly affected by this, only indirectly. i.e. The system clock happens to change during a call to `GetRandHash()` or `GetRandBytes()
...
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126384700)
well, this method is just a push function. It doesn't have to tell anything about the context where it is used. The struct is basically a map container with a cache, it doesn't need to explain how APS works.
If you are strong over the bool args usage, that would be the argument to make the changes for me.
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126384700)
well, this method is just a push function. It doesn't have to tell anything about the context where it is used. The struct is basically a map container with a cache, it doesn't need to explain how APS works.
If you are strong over the bool args usage, that would be the argument to make the changes for me.
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126384935)
sounds good
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126384935)
sounds good
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126387031)
pushed, thanks
(https://github.com/bitcoin/bitcoin/pull/25806#discussion_r1126387031)
pushed, thanks
💬 furszy commented on pull request "wallet: group outputs only once, decouple it from Coin Selection":
(https://github.com/bitcoin/bitcoin/pull/25806#issuecomment-1456099290)
Updated per feedback, thanks S3RK 👌🏼.
[Small diff](https://github.com/bitcoin/bitcoin/compare/1cfed6ce6cd8c9f9f1e93662ff8f92cd5a5b91c4..6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44), changes are in the last test scenario and the test commit message.
(https://github.com/bitcoin/bitcoin/pull/25806#issuecomment-1456099290)
Updated per feedback, thanks S3RK 👌🏼.
[Small diff](https://github.com/bitcoin/bitcoin/compare/1cfed6ce6cd8c9f9f1e93662ff8f92cd5a5b91c4..6a302d40dfcc33b15bf27a1d3fcc04bfd3cbda44), changes are in the last test scenario and the test commit message.
💬 MarcoFalke commented on pull request "util: Use steady clock in SeedStrengthen, FindBestImplementation, FlushStateToDisk":
(https://github.com/bitcoin/bitcoin/pull/27189#issuecomment-1456125811)
> Am I correct in my understanding that there are no current tests/benchmarks which are calling these while adjusting the system clock, but rather this is additional hardening for a user whose system clock happens to be changed while one of these is executing?
yes, I don't think the system clock is ever adjusted in tests. Usually, we'd use `NodeClock` for a mockable clock. `faketime` may be a way to mock the system clock in tests, but faketime may not be available on Windows.
About the use
...
(https://github.com/bitcoin/bitcoin/pull/27189#issuecomment-1456125811)
> Am I correct in my understanding that there are no current tests/benchmarks which are calling these while adjusting the system clock, but rather this is additional hardening for a user whose system clock happens to be changed while one of these is executing?
yes, I don't think the system clock is ever adjusted in tests. Usually, we'd use `NodeClock` for a mockable clock. `faketime` may be a way to mock the system clock in tests, but faketime may not be available on Windows.
About the use
...
📝 vasild opened a pull request: "gui: use the stored CSubNet entry when unbanning"
(https://github.com/bitcoin-core/gui/pull/717)
The previous code visualized the `CSubNet` object as string, then parsed that string back to `CSubNet`. This is sub-optimal given that the original `CSubNet` object can be used directly instead.
This avoids calling `LookupSubNet()` from the GUI.
(https://github.com/bitcoin-core/gui/pull/717)
The previous code visualized the `CSubNet` object as string, then parsed that string back to `CSubNet`. This is sub-optimal given that the original `CSubNet` object can be used directly instead.
This avoids calling `LookupSubNet()` from the GUI.
💬 vasild commented on pull request "Handle CJDNS from LookupSubNet()":
(https://github.com/bitcoin/bitcoin/pull/27071#issuecomment-1456152963)
@mzumsande, it is possible to avoid calling `LookupSubNet()` from the GUI, see https://github.com/bitcoin-core/gui/pull/717. If that gets merged then this PR will be simplified - no need to touch the GUI code from here.
(https://github.com/bitcoin/bitcoin/pull/27071#issuecomment-1456152963)
@mzumsande, it is possible to avoid calling `LookupSubNet()` from the GUI, see https://github.com/bitcoin-core/gui/pull/717. If that gets merged then this PR will be simplified - no need to touch the GUI code from here.
💬 vasild commented on pull request "gui: use the stored CSubNet entry when unbanning":
(https://github.com/bitcoin-core/gui/pull/717#issuecomment-1456154849)
This would simplify https://github.com/bitcoin/bitcoin/pull/27071.
(https://github.com/bitcoin-core/gui/pull/717#issuecomment-1456154849)
This would simplify https://github.com/bitcoin/bitcoin/pull/27071.
💬 petertodd commented on issue "Allow several OP_RETURN in one tx and no limited size":
(https://github.com/bitcoin/bitcoin/issues/27043#issuecomment-1456161328)
So the next step here for people who want to actually see this implemented is code, and perhaps more importantly, tests.
The code change is fairly simple: change the `datacarrierlimit` to default to max-int, and make the rule that checks if there is more than one OP_Return only take action if the limit isn't the default. I don't think there is any reason to implement a separate rule to check if there is more than one OP_Return if you aren't limiting the size.
Second, tests. You need to che
...
(https://github.com/bitcoin/bitcoin/issues/27043#issuecomment-1456161328)
So the next step here for people who want to actually see this implemented is code, and perhaps more importantly, tests.
The code change is fairly simple: change the `datacarrierlimit` to default to max-int, and make the rule that checks if there is more than one OP_Return only take action if the limit isn't the default. I don't think there is any reason to implement a separate rule to check if there is more than one OP_Return if you aren't limiting the size.
Second, tests. You need to che
...
👍 dergoegge approved a pull request: "ci: Remove unused EXPECTED_TESTS_DURATION_IN_SECONDS env var"
(https://github.com/bitcoin/bitcoin/pull/27209)
ACK 3fffff50f625ed5e3c45139b3ae874f63f121a1e
(https://github.com/bitcoin/bitcoin/pull/27209)
ACK 3fffff50f625ed5e3c45139b3ae874f63f121a1e