💬 fanquake commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2437809265)
> I know this can be cleaned up, but i wanted to keep it minimal and focused on dropping UPnP. I'm happy to do a few cleanups (such as the ones you suggest) but i don't want to start refactoring the whole file.
Sure, that's fine, any more-invasive changes could be done later, but we shouldn't be leaving around dead code, or commentary that is incorrect.
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2437809265)
> I know this can be cleaned up, but i wanted to keep it minimal and focused on dropping UPnP. I'm happy to do a few cleanups (such as the ones you suggest) but i don't want to start refactoring the whole file.
Sure, that's fine, any more-invasive changes could be done later, but we shouldn't be leaving around dead code, or commentary that is incorrect.
💬 fanquake commented on pull request "optimization: Preallocate addresses in GetAddr based on nNodes":
(https://github.com/bitcoin/bitcoin/pull/29608#issuecomment-2437813806)
> I don't think this is a particularly impactful change but it's very small and straightforward to review.
I agree, and I think we are probably overdue for a review of what is in `src/bench`, and cleaning out benchmarks that aren't (very) meaningful. IIRC `clang-tidy` has a check for exactly this kind of change, so I think if any more were going to be made, they could be done using that.
(https://github.com/bitcoin/bitcoin/pull/29608#issuecomment-2437813806)
> I don't think this is a particularly impactful change but it's very small and straightforward to review.
I agree, and I think we are probably overdue for a review of what is in `src/bench`, and cleaning out benchmarks that aren't (very) meaningful. IIRC `clang-tidy` has a check for exactly this kind of change, so I think if any more were going to be made, they could be done using that.
💬 laanwj commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2437814832)
> i don't want to start refactoring the whole file. It can always happen in a follow-up if someone is interested and more familiar with this code.
i agree. Let's make sense there are no clearly wrong comments, or dead code. But as we're sure that the only port mapping method we want to keep is PCP/NATPMP, a lot of the logic in this file can be removed and methods can be renamed. But might as well do that in a future follow-up to #30043, as some of those need larger restructures to the loop t
...
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2437814832)
> i don't want to start refactoring the whole file. It can always happen in a follow-up if someone is interested and more familiar with this code.
i agree. Let's make sense there are no clearly wrong comments, or dead code. But as we're sure that the only port mapping method we want to keep is PCP/NATPMP, a lot of the logic in this file can be removed and methods can be renamed. But might as well do that in a future follow-up to #30043, as some of those need larger restructures to the loop t
...
💬 jonatack commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816714440)
Thanks for the example. As the primary author of this code, I think the run-time error would be more clear, though a compile-time error would catch it one CLI command earlier. I haven't found this kind of error to be an issue for me over the past half decade on this code.
Also, this code path is manually tested by myself, reviewers and users. Perhaps for that reason, and because it's non-critical code, a maintainer explicitly asked that no test coverage be written for it.
So either way is
...
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816714440)
Thanks for the example. As the primary author of this code, I think the run-time error would be more clear, though a compile-time error would catch it one CLI command earlier. I haven't found this kind of error to be an issue for me over the past half decade on this code.
Also, this code path is manually tested by myself, reviewers and users. Perhaps for that reason, and because it's non-critical code, a maintainer explicitly asked that no test coverage be written for it.
So either way is
...
💬 jonatack commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816718706)
> I'm just going with our clang-format settings as defined in src/.clang-format
It's not a hard requirement to retouch existing code with that tool, and I'd prefer avoiding the churn.
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816718706)
> I'm just going with our clang-format settings as defined in src/.clang-format
It's not a hard requirement to retouch existing code with that tool, and I'd prefer avoiding the churn.
🚀 fanquake merged a pull request: "optimization: Preallocate addresses in GetAddr based on nNodes"
(https://github.com/bitcoin/bitcoin/pull/29608)
(https://github.com/bitcoin/bitcoin/pull/29608)
💬 instagibbs commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2437829923)
@stickies-v `IsWellFormedPackage` coverage already looks good to me, added our unit test for single tx package processing, thanks
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2437829923)
@stickies-v `IsWellFormedPackage` coverage already looks good to me, added our unit test for single tx package processing, thanks
🤔 marcofleon reviewed a pull request: "fuzz: fuzz connman with non-empty addrman + ASMap"
(https://github.com/bitcoin/bitcoin/pull/29536#pullrequestreview-2395370037)
Code review ACK 552cae243a1bf26bfec03eccd1458f3bf33e01dc. Changes match the PR description.
(https://github.com/bitcoin/bitcoin/pull/29536#pullrequestreview-2395370037)
Code review ACK 552cae243a1bf26bfec03eccd1458f3bf33e01dc. Changes match the PR description.
💬 fanquake commented on pull request "optimization: Preallocate addresses in GetAddr based on nNodes":
(https://github.com/bitcoin/bitcoin/pull/29608#issuecomment-2437847307)
The `clang-tidy` check I was thinking of was https://clang.llvm.org/extra/clang-tidy/checks/performance/inefficient-vector-operation.html.
(https://github.com/bitcoin/bitcoin/pull/29608#issuecomment-2437847307)
The `clang-tidy` check I was thinking of was https://clang.llvm.org/extra/clang-tidy/checks/performance/inefficient-vector-operation.html.
💬 real-or-random commented on pull request "cleanse: switch to SecureZeroMemory for Windows cross-compile":
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2437850544)
> @real-or-random @laanwj's comment is mostly correct. iirc at the time this was one of a few MSVC specific defines we had left in the codebase. Since then others have been added, but mostly to just workaround, or ignore bugs in MSVC.
Thanks, makes sense.
>
> > (except perhaps performance if the implementation of SecureZeroMemory on mingw-w64 is worse than what the compiler can come up with for the memset).
>
> I would think the code would not be worse performance wise, as it's implem
...
(https://github.com/bitcoin/bitcoin/pull/26950#issuecomment-2437850544)
> @real-or-random @laanwj's comment is mostly correct. iirc at the time this was one of a few MSVC specific defines we had left in the codebase. Since then others have been added, but mostly to just workaround, or ignore bugs in MSVC.
Thanks, makes sense.
>
> > (except perhaps performance if the implementation of SecureZeroMemory on mingw-w64 is worse than what the compiler can come up with for the memset).
>
> I would think the code would not be worse performance wise, as it's implem
...
💬 brunoerg commented on pull request "[tests] New fuzz target wallet_rpc":
(https://github.com/bitcoin/bitcoin/pull/30570#issuecomment-2437854933)
I think we will have here the same problems that we usually have for other wallet targets. The size of keypool, wallet encryption, etc... may cause timeout issues.
(https://github.com/bitcoin/bitcoin/pull/30570#issuecomment-2437854933)
I think we will have here the same problems that we usually have for other wallet targets. The size of keypool, wallet encryption, etc... may cause timeout issues.
💬 stickies-v commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816740327)
> As the primary author of this code, I think the run-time error would be more clear
I'm sorry, what? For what reason would we not use the logic we already have to catch (some/most) of these format errors at compile-time, and defer them to run-time instead, relying on manual review to make sure each code-path is ran, potentially crashing user software? That seems like the opposite of security engineering best practices to me.
> a maintainer explicitly asked that no test coverage be written
...
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816740327)
> As the primary author of this code, I think the run-time error would be more clear
I'm sorry, what? For what reason would we not use the logic we already have to catch (some/most) of these format errors at compile-time, and defer them to run-time instead, relying on manual review to make sure each code-path is ran, potentially crashing user software? That seems like the opposite of security engineering best practices to me.
> a maintainer explicitly asked that no test coverage be written
...
👍 TheCharlatan approved a pull request: "tinyformat: enforce compile-time checks for format string literals"
(https://github.com/bitcoin/bitcoin/pull/31149#pullrequestreview-2395385698)
Nice, ACK 23560b468c474bbc6a3abd6c0778da62766ac8f7
(https://github.com/bitcoin/bitcoin/pull/31149#pullrequestreview-2395385698)
Nice, ACK 23560b468c474bbc6a3abd6c0778da62766ac8f7
🤔 hodlinator reviewed a pull request: "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte"
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2395174160)
Concept ACK a3dc138798e2f2c7aa1c9b37633c16c1b523a251
Operating on CPU words rather than individual bytes. :+1:
Not entirely clear to me from https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1814741753 whether the optimizer is able to use SIMD. Guess picking through the binary of a GUIX-build would give a definitive answer.
The verbosity of `std::memcpy` hurts readability but alignment issues are real.
(https://github.com/bitcoin/bitcoin/pull/31144#pullrequestreview-2395174160)
Concept ACK a3dc138798e2f2c7aa1c9b37633c16c1b523a251
Operating on CPU words rather than individual bytes. :+1:
Not entirely clear to me from https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1814741753 whether the optimizer is able to use SIMD. Guess picking through the binary of a GUIX-build would give a definitive answer.
The verbosity of `std::memcpy` hurts readability but alignment issues are real.
💬 hodlinator commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1816662396)
Might as well use `std::span` for new code.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1816662396)
Might as well use `std::span` for new code.
💬 hodlinator commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1816612914)
Seems like prior author just rounded off to some *not too unreasonable* tab-indentation (efd2474d17098c754367b844ec646ebececc7c74). Function isn't touched in this PR so should probably resist touching here and below.
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1816612914)
Seems like prior author just rounded off to some *not too unreasonable* tab-indentation (efd2474d17098c754367b844ec646ebececc7c74). Function isn't touched in this PR so should probably resist touching here and below.
💬 hodlinator commented on pull request "optimization: pack util::Xor into 64/32 bit chunks instead of doing it byte-by-byte":
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1816619457)
Experimented with changed to brace-initialization which uncovered some slight narrowing/widening occurring. (Thought I had an angle for making the code more robust in a more material way but that attempt failed).
```suggestion
auto expected_xor{[](Span<std::byte> write, Span<const std::byte> key, size_t key_offset) {
if (key.size()) {
for (auto& b : write) {
b ^= key[key_offset++ % key.size()];
}
}
}};
FastRandomConte
...
(https://github.com/bitcoin/bitcoin/pull/31144#discussion_r1816619457)
Experimented with changed to brace-initialization which uncovered some slight narrowing/widening occurring. (Thought I had an angle for making the code more robust in a more material way but that attempt failed).
```suggestion
auto expected_xor{[](Span<std::byte> write, Span<const std::byte> key, size_t key_offset) {
if (key.size()) {
for (auto& b : write) {
b ^= key[key_offset++ % key.size()];
}
}
}};
FastRandomConte
...
💬 rkrux commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1816744003)
Thanks for adding this comment, helpful!
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1816744003)
Thanks for adding this comment, helpful!
💬 rkrux commented on pull request "functional test: Additional package evaluation coverage":
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1816746874)
IIUC shouldn't `usage` be used here instead of `bytes` because the first argument of assert uses memory usage?
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mempool.cpp#L699-L700
(https://github.com/bitcoin/bitcoin/pull/31152#discussion_r1816746874)
IIUC shouldn't `usage` be used here instead of `bytes` because the first argument of assert uses memory usage?
https://github.com/bitcoin/bitcoin/blob/master/src/rpc/mempool.cpp#L699-L700
💬 TheCharlatan commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816753074)
> I'd prefer avoiding the churn.
We usually call things churn that change things multiple times within the same PR. Indenting the lines below a changed line that belong to the same logical unit is a good thing in my eyes. If we would not do that anywhere, the code would look weird and probably make `clang-format` unusable after a while.
(https://github.com/bitcoin/bitcoin/pull/31149#discussion_r1816753074)
> I'd prefer avoiding the churn.
We usually call things churn that change things multiple times within the same PR. Indenting the lines below a changed line that belong to the same logical unit is a good thing in my eyes. If we would not do that anywhere, the code would look weird and probably make `clang-format` unusable after a while.