💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276110597)
The CI system caches built images. The cache-key is (among other stuff) this file. If the file does not change, the cache-key does not change either. However, if the `bitcoin-tidy-plugin` changes, everyone may have a different version of `bitcoin-tidy-plugin` in the cache, without the cache being invalidated. Thus, everyone may get different results for the same commit-id in this repo. (This is known as non-deterministic).
It can be fixed by pinning to a tag/commit/branch/...
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276110597)
The CI system caches built images. The cache-key is (among other stuff) this file. If the file does not change, the cache-key does not change either. However, if the `bitcoin-tidy-plugin` changes, everyone may have a different version of `bitcoin-tidy-plugin` in the cache, without the cache being invalidated. Thus, everyone may get different results for the same commit-id in this repo. (This is known as non-deterministic).
It can be fixed by pinning to a tag/commit/branch/...
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653380016)
Sure, no worries. Maybe keep it a draft for as long as CI is red or fix the errors? Otherwise, it can't be merged/ack'd anyway.
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653380016)
Sure, no worries. Maybe keep it a draft for as long as CI is red or fix the errors? Otherwise, it can't be merged/ack'd anyway.
💬 dergoegge commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653381135)
> Obviously. Given there is a commit here to make it fail?
Yes but it's also failing on a different LogPrint statement, so no need for that commit i guess
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653381135)
> Obviously. Given there is a commit here to make it fail?
Yes but it's also failing on a different LogPrint statement, so no need for that commit i guess
💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653386005)
> Yes but it's also failing on a different LogPrint statement, so no need for that commit i guess
Yea, assuming it's an actual issue, and not a false positive. If it is, then it means our current linter doesn't work anyways.
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653386005)
> Yes but it's also failing on a different LogPrint statement, so no need for that commit i guess
Yea, assuming it's an actual issue, and not a false positive. If it is, then it means our current linter doesn't work anyways.
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276120896)
For example, you can add `git checkout 1806ef33ff8b14256d766eb9f616a3528aad4464` in the next line.
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276120896)
For example, you can add `git checkout 1806ef33ff8b14256d766eb9f616a3528aad4464` in the next line.
💬 dergoegge commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653397352)
> Yea, assuming it's an actual issue, and not a false positive.
At least for the failure in `./test/util/chainstate.h` it's because the current linter only works on cpp files not headers.
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653397352)
> Yea, assuming it's an actual issue, and not a false positive.
At least for the failure in `./test/util/chainstate.h` it's because the current linter only works on cpp files not headers.
💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653407802)
Fixed the non-determinism.
Dropped the test commit.
Fixed the issue in chainstate.h.
> it's because the current linter only works on cpp files not headers.
I guess that's another potential +1 for this approach?
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653407802)
Fixed the non-determinism.
Dropped the test commit.
Fixed the issue in chainstate.h.
> it's because the current linter only works on cpp files not headers.
I guess that's another potential +1 for this approach?
👍 stickies-v approved a pull request: "refactor: Remove unused raw-pointer read helper from univalue"
(https://github.com/bitcoin/bitcoin/pull/28168#pullrequestreview-1549566799)
utACK 9999cfc1f3b728132e011f0aec24211212924de2
(https://github.com/bitcoin/bitcoin/pull/28168#pullrequestreview-1549566799)
utACK 9999cfc1f3b728132e011f0aec24211212924de2
💬 stickies-v commented on pull request "refactor: Remove unused raw-pointer read helper from univalue":
(https://github.com/bitcoin/bitcoin/pull/28168#discussion_r1276106247)
nit: can't get iwyu to work on this file, but i think `#include <cstring>` is now no longer required
(https://github.com/bitcoin/bitcoin/pull/28168#discussion_r1276106247)
nit: can't get iwyu to work on this file, but i think `#include <cstring>` is now no longer required
💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276134360)
Pinned to a commit here.
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276134360)
Pinned to a commit here.
🚀 fanquake merged a pull request: "test: remove unused code in `wallet_fundrawtransaction`"
(https://github.com/bitcoin/bitcoin/pull/28164)
(https://github.com/bitcoin/bitcoin/pull/28164)
💬 ismaelsadeeq commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1276161304)
Thank you.
I removed the clears.
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1276161304)
Thank you.
I removed the clears.
💬 ismaelsadeeq commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#issuecomment-1653452667)
> I get the same error on master branch too, so I dunno if it's really up to you and this PR to patch around it or not.
I think this should be fixed in follow-up PR.
(https://github.com/bitcoin/bitcoin/pull/28157#issuecomment-1653452667)
> I get the same error on master branch too, so I dunno if it's really up to you and this PR to patch around it or not.
I think this should be fixed in follow-up PR.
💬 MarcoFalke commented on pull request "refactor: Remove unused raw-pointer read helper from univalue":
(https://github.com/bitcoin/bitcoin/pull/28168#discussion_r1276174862)
cc @sipsorcery @hebasto Any idea how to make a newline after the `#include` statement here on Windows?
(https://github.com/bitcoin/bitcoin/pull/28168#discussion_r1276174862)
cc @sipsorcery @hebasto Any idea how to make a newline after the `#include` statement here on Windows?
📝 MarcoFalke converted_to_draft a pull request: "refactor: Remove unused raw-pointer read helper from univalue"
(https://github.com/bitcoin/bitcoin/pull/28168)
The helpers are unused outside of tests and redundant with the existing `bool read(std::string_view raw);`.
Fix both issues by removing them.
Also, simplify the tests code by removing a `std::string` constructor where possible.
(https://github.com/bitcoin/bitcoin/pull/28168)
The helpers are unused outside of tests and redundant with the existing `bool read(std::string_view raw);`.
Fix both issues by removing them.
Also, simplify the tests code by removing a `std::string` constructor where possible.
💬 ajtowns commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1276175156)
This example should be prevented by the "Protect extra full outbound peers by network" commit, shouldn't it? ie the process would be:
* 7 ipv4 + 1 tor
* 7 ipv4 + 1 tor + 1 i2p (via "network specific connection")
* 6 ipv4 + 1 tor + 1 i2p (tor and i2p nodes protected from eviction due to `if (!m_connman.MultipleManualOrFullOutboundConns(pnode->addr.GetNetwork())) return;` code path)
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1276175156)
This example should be prevented by the "Protect extra full outbound peers by network" commit, shouldn't it? ie the process would be:
* 7 ipv4 + 1 tor
* 7 ipv4 + 1 tor + 1 i2p (via "network specific connection")
* 6 ipv4 + 1 tor + 1 i2p (tor and i2p nodes protected from eviction due to `if (!m_connman.MultipleManualOrFullOutboundConns(pnode->addr.GetNetwork())) return;` code path)
💬 stickies-v commented on pull request "refactor: Remove unused raw-pointer read helper from univalue":
(https://github.com/bitcoin/bitcoin/pull/28168#discussion_r1276196195)
What about this? Adds a CR/LF so should work on windows? (but can't test)
```
<HeaderFromHexdump RawFilePath="%(JsonTestFile.FullPath)" HeaderFilePath="%(JsonTestFile.FullPath).h" SourceHeader="#include <string>
namespace json_tests{ static const std::string %(JsonTestFile.Filename){" SourceFooter="};}" />
```
(https://github.com/bitcoin/bitcoin/pull/28168#discussion_r1276196195)
What about this? Adds a CR/LF so should work on windows? (but can't test)
```
<HeaderFromHexdump RawFilePath="%(JsonTestFile.FullPath)" HeaderFilePath="%(JsonTestFile.FullPath).h" SourceHeader="#include <string>
namespace json_tests{ static const std::string %(JsonTestFile.Filename){" SourceFooter="};}" />
```
💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1276217017)
> IIUC this line schedules a replacement sensitive connection because the one we were passed in has the wrong services and so it gets dumped.
Yes.
> I think this is the only line in this function that makes it apply strictly to only sensitive relays. So I wonder if that should be asserted at the top? Or could be renamed to `PushUnbroadcastTxToSensistiveRelay()`? At the very least maybe sensitive-only should be explained in the function description on L933 ?
Right. While this function wi
...
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1276217017)
> IIUC this line schedules a replacement sensitive connection because the one we were passed in has the wrong services and so it gets dumped.
Yes.
> I think this is the only line in this function that makes it apply strictly to only sensitive relays. So I wonder if that should be asserted at the top? Or could be renamed to `PushUnbroadcastTxToSensistiveRelay()`? At the very least maybe sensitive-only should be explained in the function description on L933 ?
Right. While this function wi
...
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653550331)
Should this also remove the `/* Continued */` code-bloat?
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653550331)
Should this also remove the `/* Continued */` code-bloat?
💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276235709)
I wonder if atomic updates (for example the LogPrint macro being renamed, or similar) would be easier to do if the whole code is inside this repo, just like before for the python linter?
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276235709)
I wonder if atomic updates (for example the LogPrint macro being renamed, or similar) would be easier to do if the whole code is inside this repo, just like before for the python linter?
💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276240902)
I'm not at all opposed to this. In fact, yeah, that probably makes more sense. Could always move to a repo if it gets too big/busy here.
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276240902)
I'm not at all opposed to this. In fact, yeah, that probably makes more sense. Could always move to a repo if it gets too big/busy here.