💬 naumenkogs commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1276078272)
nit: may be worth explaining that this randomization helps to get to NET_I2P option without traversing the entire NET_ONION set of potentially rubbish or buggy stuff :)

💬 naumenkogs commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1276083657)
Do we have any risk of TOR and I2P peers bouncing each other forever?
Say, you have 7 ip4 peers and 1 Tor peer. I2P bounces Tor. In the next step, Tor (maybe another node, or maybe the same) bounces I2P node back.

📝 ChrisCho-H opened a pull request: "script: check op_verif and op_vernotif"
(https://github.com/bitcoin/bitcoin/pull/28169)
I suggest adding `SCRIPT_ERR_OP_VERIF_VERNOTIF` error to check whether it's either `OP_VERIF` or `OP_VERNOTIF` since,
1. As opcodes rejected even in an unexecuted branch, are checked and throw error with an appropriate message(like disabled and codeseperator in non-segwit) to identify, op_verif and op_vernotif(which are rejected even in an unexecuted branch as well) should be treated in same way which is clear to debug
2. It still makes script invalid now as both opcodes are in between op_if a
...

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