💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276097526)
You'll need to pin this to a tag/commit/branch that changes when the features change. Otherwise the CI will be non-deterministic

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276101720)
Yea. I'm not sure exactly what we'll do yet, but I think we could just have a "stable" branch in the tidy-plugin repo.

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276102904)
That wouldn't work, the id of the tag/commit/branch needs to be different for each added or removed feature.

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653367400)
Also, CI (tidy) fails

💬 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?

💬 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.

💬 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/...

💬 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.

💬 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

💬 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.

💬 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.

💬 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.

💬 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?

👍 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

💬 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

💬 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.

🚀 fanquake merged a pull request: "test: remove unused code in `wallet_fundrawtransaction`"
(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.

💬 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.

💬 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?