💬 theuni commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653641372)
Heh, really sorry @fanquake .

I think the out-of-tree approach makes sense for (for example) [plugins where we can create attributes and assign them meaning](https://github.com/theuni/bitcoin-core-clang-plugin/commit/05fb23f21df32df4445951123d281172b12ee61b). In that case, the "atomicity" issue goes away, because the code either respects the attribute or it doesn't.

But yeah, now that @MarcoFalke mentions it, that approach is cumbersome when we have to worry about things getting out of syn
...

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1653647053)
> @fanquake Want to keep going here, or shall I open a new PR?

I can convert this over. I think retaining the current build as-is, is ok.

💬 vasild commented on pull request "Relay own transactions only via short-lived Tor or I2P connections":
(https://github.com/bitcoin/bitcoin/pull/27509#discussion_r1276302188)
Yeah, it does not provide a direct access to an arbitrary element. This is `O(unbroadcast_txids.size())` if faster is required we have to change the container (or extend it with supplementary vector). I shortened this a bit to:

```cpp
const auto tx_iter = std::next(unbroadcast_txids.begin(), FastRandomContext{}.randrange(unbroadcast_txids.size()));
```

This is still linear, but at least a bit short and (I hope) as easy to read.

💬 ajtowns commented on pull request "contrib: add tool to convert compact-serialized UTXO set to SQLite database":
(https://github.com/bitcoin/bitcoin/pull/27432#issuecomment-1653739351)
Approach ACK. Seems like a fine idea to me.

> What is the rationale for encoding as text rather than bytes? SQLite can store byte values as BLOBs.

It's a python conversion script: can't you just add a command-line option for the resulting db to have hex txids or big/little endian blobs if there's user demand for it? Hex encoding seems a fine default to me, for what it's worth.

If people end up wanting lots of different options (convert scriptPubKeys to addresses? some way to update the
...

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