💬 fanquake commented on pull request "ci: use clang-16 in tidy task":
(https://github.com/bitcoin/bitcoin/pull/27404#issuecomment-1497286500)
> -I/usr/lib/llvm-16/lib/clang/16/include

Included this. I think we could merge this as-is, but should also figure out why it's needed. It feels like there is another issue here.

> Also https://github.com/bitcoin/bitcoin/pull/25466#discussion_r930891939

Removed the related suppressions.

💬 fanquake commented on pull request "ci: add unused-using-decls to clang-tidy":
(https://github.com/bitcoin/bitcoin/pull/25466#discussion_r1158349009)
Followed up with in #27404.

👋 fanquake's pull request is ready for review: "ci: use clang-16 in tidy task"
(https://github.com/bitcoin/bitcoin/pull/27404)

💬 dimitaracev commented on pull request "validation: Move warningcache to ChainstateManager and rename to m_warningcache":
(https://github.com/bitcoin/bitcoin/pull/27357#issuecomment-1497287457)
> ACK [5526849](https://github.com/bitcoin/bitcoin/commit/552684976b6df34ce563458f73812e6e494e3b0e)
>
> Looking at this, it occurs to me that we're actually being a bit wasteful scanning through ~780k historical headers 29 times (or 2.4M headers 29 times for testnet) every time we startup. Replacing `MinBIP9WarningHeight` with `MinBIP9WarningStartTime` (set it to say 2022-07-01 initially), and using it for `WarningBitsConditionChecker::BeginTime()`, and just bumping it at each release might b
...

🚀 fanquake merged a pull request: "log: Check that the timestamp string is non-empty to avoid undefined behavior"
(https://github.com/bitcoin/bitcoin/pull/27317)

👍 fanquake approved a pull request: "test: Remove windows workaround in authproxy"
(https://github.com/bitcoin/bitcoin/pull/27418)
ACK fa584b4d01ca055488a10b280f45dca323b474d4

👍 hebasto approved a pull request: "ci: use clang-16 in tidy task"
(https://github.com/bitcoin/bitcoin/pull/27404)
ACK a56c96507a9e943bbcd7e126bc827de9495f0ebd

💬 josibake commented on pull request "ci: Remove second user account":
(https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1497308824)
I was able to reproduce this locally on master by running:

```
make distclean
FILE_ENV="./ci/test/00_setup_env_native_tidy.sh" ./ci/test_run_all.sh
```

I ssh'd into the container and found that the `git diff` command is failing due to `dubious permissions`, which I think `./ci/lint/docker-entrypoint.sh` is supposed to fix with `git config --global --add safe.directory /bitcoin`? Seems like maybe that fix isn't working everywhere but we didn't catch it before because of the whole two use
...

💬 MarcoFalke commented on pull request "ci: Remove second user account":
(https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1497313723)
> `./ci/lint/` ...

The lint system is completely different from `./ci/test/`.

🚀 fanquake merged a pull request: "test: Remove windows workaround in authproxy"
(https://github.com/bitcoin/bitcoin/pull/27418)

💬 josibake commented on pull request "ci: Remove second user account":
(https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1497316092)
> The lint system is completely different from ./ci/test/.

yeah, just realized there is no overlap. seems like we just need to apply the same fix the lint system is using in `./ci/test/` ?

💬 MarcoFalke commented on pull request "ci: Remove second user account":
(https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1497316877)
As for the fix, anything should be fine. git `safe.directory`, or restoring the `chown`, or fiddling with the `rsync` command.

💬 josibake commented on pull request "ci: Remove second user account":
(https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1497318317)
> As for the fix, anything should be fine. git safe.directory, or restoring the chown, or fiddling with the rsync command.

cool, I'll open a PR

💬 Ayush170-Future commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1497321029)
I agree with @Sjors here. In my last review comment, I suggested the same idea. Though I'm not sure how the peer privacy issue is relevant here. But, it makes more sense to only log ASN when <code>fLogIPs</code> is already true.

Maybe changing the name of <code>-logips</code> flag to something like <code>-lognetinfo</code> will better reflect its use and avoid confusion, but that would require a release note. (just a school of thought)

💬 MarcoFalke commented on pull request "ci: use clang-16 in tidy task":
(https://github.com/bitcoin/bitcoin/pull/27404#issuecomment-1497326083)
lgtm ACK a56c96507a9e943bbcd7e126bc827de9495f0ebd

🚀 fanquake merged a pull request: "depends: add `NO_HARDEN=` option"
(https://github.com/bitcoin/bitcoin/pull/27406)

📝 josibake opened a pull request: "ci: fix git dubious permissions error"
(https://github.com/bitcoin/bitcoin/pull/27423)
fixes https://github.com/bitcoin/bitcoin/pull/27376#issuecomment-1496449588

this appears to be caused by a more recent version of git being sensitive to mismatched permissions on directories. we didn't notice this before because we were using two separate user accounts to fix up dir permissions in the container , but the second account was removed in #27376

there might be a more elegant way to do this, but this does the trick and seems to be the way others are fixing this issue around the
...

💬 josibake commented on pull request "ci: fix git dubious permissions error":
(https://github.com/bitcoin/bitcoin/pull/27423#issuecomment-1497346650)
cc @hebasto @MarcoFalke

💬 Sjors commented on pull request "logging, net: add ASN from peers on logs":
(https://github.com/bitcoin/bitcoin/pull/27412#issuecomment-1497347621)
Reading that original discussion I think the current code is fine. The main concern was exposing the origin of transactions in log files under the _default_ configuration. Since we don't log transaction relay by default (it's extremely noisy) we can safely log the ASN (or even IP) of a node when it connects.

We can clean up the documentation and/or behavior of `-logips` in another PR.

💬 josibake commented on pull request "ci: use clang-16 in tidy task":
(https://github.com/bitcoin/bitcoin/pull/27404#issuecomment-1497365274)
ACK https://github.com/bitcoin/bitcoin/pull/27404/commits/a56c96507a9e943bbcd7e126bc827de9495f0ebd

> Included this. I think we could merge this as-is, but should also figure out why it's needed. It feels like there is another issue here.

did a little digging and found this: https://github.com/include-what-you-use/include-what-you-use/issues/679#issuecomment-482798379 , which seems to still be an issue based on the discussion in the linked solution. fwiw, the fix you have here seems to be o
...