💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653367770)
> Also, CI (tidy) fails
Obviously. Given there is a commit here to make it fail?
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653367770)
> Also, CI (tidy) fails
Obviously. Given there is a commit here to make it fail?
💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276105268)
I don't understand what you mean.
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276105268)
I don't understand what you mean.
💬 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
...