💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653740441)
Reworked. I'm guessing some of our linters could fail against the new lint code? If so, I think we should just exclude it from them. Added some more fixups. Updated the PR description. Not 100% sure about the CI (build) dir handling, but this can also be improved.

Added a commit to drop the `/* Continued */` markers. Can be squashed into the previous commit which drops lint-logs.

💬 jamesob commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1653747991)
Concept ACK, will test soon

💬 mzumsande commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1653766453)
Maybe a release note would be nice?
Not because this would break anything, but so that users who are currently performing active measures to stay connected to multiple networks (e.g. `-addnode` together with regular monitoring if the added node is still online) know that there is another option now.

💬 TheCharlatan commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1276382028)
> I am also thinking of adding a util::Messages{Result&&} helper.

That sounds like a worthwhile improvement of the ergonomics here.

💬 TheCharlatan commented on pull request "refactor: Add util::Result failure values, multiple error and warning messages":
(https://github.com/bitcoin/bitcoin/pull/25665#discussion_r1276417417)
It strikes me as unfortunate that the move constructor cannot not move the failure values across different result value types, meaning the failure needs to be passed in as a separate argument. At the same time the user would be allowed to return a `{std::move(result), util::Error{Untranslated("str error")}`, potentially (and I am still not quite sure if that is what is actually happening here) without the user noticing that this will not move the failure value and instead initialize a `Monostate
...

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276418105)
Run clang-format on new code?

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276421094)
I guess the build is fast and not needed to be cached for now?

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276415678)
nit:
```suggestion
make -j$(nproc) -C build
```

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276437800)
Added.

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276438436)
clang-formatted everything using `src/.clang-format`.

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276439295)
Yea, it's only two files, so very fast, especially compared to the runtime of clang-tidy itself.

💬 stickies-v commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1276442786)
Can be made quite a bit more DRY:
```suggestion
self.log.info(f"Test -acceptstalefeeestimates option is not supported on non-regtest chains")
for chain in ["main", "test", "signet"]:
with open(inc_conf_file_path, 'w', encoding='utf-8') as conf:
conf.write(f'chain={chain}\n')
conf.write('acceptstalefeeestimates=1\n')
self.nodes[0].assert_start_raises_init_error(extra_args=[f"-conf={inc_conf_file_path}", "-allowignoredc
...

💬 stickies-v commented on pull request "test doc: tests `acceptstalefeeestimates` option is only supported on regtest chain":
(https://github.com/bitcoin/bitcoin/pull/28157#discussion_r1276443940)
Would generally avoid unrelated style nits, you're not touching this line (or even file, for that matter) for anything else, so best to just leave as is.

💬 achow101 commented on pull request "refactor: consistently use ApplyArgsManOptions for PeerManager::Options":
(https://github.com/bitcoin/bitcoin/pull/28148#issuecomment-1653841139)
ACK 8a3159728ae84cb8093e2e9fa5d2c2b0a7d545da

✅ fanquake closed a pull request: "Drop macOS ForceActivation workaround"
(https://github.com/bitcoin-core/gui/pull/744)

🚀 achow101 merged a pull request: "refactor: consistently use ApplyArgsManOptions for PeerManager::Options"
(https://github.com/bitcoin/bitcoin/pull/28148)

📝 furszy opened a pull request: "net: adaptive connections services flags"
(https://github.com/bitcoin/bitcoin/pull/28170)
Derived from #28120 discussion.

By relocating the peer desirable services flags into the peer manager, we
allow the connections acceptance process to handle post-IBD potential
stalling scenarios.

The peer manager will be able to dynamically adjust the services flags
based on the node's proximity to the tip (back and forth). Allowing the node
to recover from the following post-IBD scenario:
Suppose the node has successfully synced the chain, but later experienced
dropped connections a
...

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1276473986)
Just for reference, this isn't actually safe, since Bitcoin Core has multiple thread, which may log and interleave at will. But that is a pre-existing bug, and I presume this check just accommodates this, which is fine.

👍 MarcoFalke approved a pull request: "ci: Integrate `bitcoin-tidy` clang-tidy plugin"
(https://github.com/bitcoin/bitcoin/pull/26296#pullrequestreview-1550164036)
lgtm ACK

📝 pinheadmz opened a pull request: "test: blockstore test with chattr instead of chmod"
(https://github.com/bitcoin/bitcoin/pull/28171)
alternative to https://github.com/bitcoin/bitcoin/pull/27850

see https://github.com/bitcoin/bitcoin/pull/27850#issuecomment-1650601548

just testing ci for now