👍 MarcoFalke approved a pull request: "ci: Run "macOS native x86_64" job on GitHub Actions"
(https://github.com/bitcoin/bitcoin/pull/28187#pullrequestreview-1554323041)
> ~0 Is there any reason we can't use the public beta of the arm64 runners? Have we tested that at-least?

Can you add a link to documentation on how to set that up? With details on the runners, etc. In any case, I don't think it matters, unless you can point to a bug that was found with arm64, but not with amd64. In practice, I expect all bugs found by CI to be found by both macOS arches.

💬 MarcoFalke commented on pull request "ci: Run "macOS native x86_64" job on GitHub Actions":
(https://github.com/bitcoin/bitcoin/pull/28187#discussion_r1279021074)
Or just remove the arch from the filename? The included `export HOST` should already take care of it and avoid a filename ping-ping.

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1657986362)
Dropped `DIR_IWYU`.
Updated logging tests with the above suggestion.

👋 fanquake's pull request is ready for review: "ci: Integrate `bitcoin-tidy` clang-tidy plugin"
(https://github.com/bitcoin/bitcoin/pull/26296)

💬 TheCharlatan commented on pull request "kernel: Prune leveldb headers":
(https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1657995803)
Re https://github.com/bitcoin/bitcoin/pull/28186#issuecomment-1657883999

> I don't think any user should include the dbwrapper either? No user should be directly writing or reading from the db.

It's hard to speculate on how this will be used in the future. Something calling the db directly for e.g. data science use cases would not be unheard of.

> And if you are worried about the txdb.h include, maybe do a pimpl there? IIRC https://github.com/bitcoin/bitcoin/pull/22242 did that, or so
...

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279032950)
```suggestion
git clone --depth=1 https://github.com/include-what-you-use/include-what-you-use -b clang_16 /include-what-you-use
cmake -B /iwyu-build/ -G 'Unix Makefiles' -DCMAKE_PREFIX_PATH=/usr/lib/llvm-16 -S /include-what-you-use
make -C /iwyu-build/ install "$MAKEJOBS"
```

nit: I wonder if this is better put directly under `/`? It would give the following (edge-case) benefits:

* When building into a CI image, it installs everything into a hard-coded path, making it less likel
...

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

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1658005023)
Also, the commit message of the last commit is wrong?

💬 Sjors commented on pull request "policy: Enable full-rbf by default":
(https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1658006964)
Thanks for also posting this policy change proposal on the mailinglist (https://github.com/bitcoin/bitcoin/pull/28132#issuecomment-1657206487).

The main benefit of this PR would be to undo the split in policy caused by adding this flag and widely advocating for it. I don't think that's just a fad at this point. However I'm inclined to wait a little bit longer, e.g. after the v26 release, to see if your observations are maintained and reproduced outside of OTS. The new mempool.space tracking i
...

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1658011739)
> Also, the commit message of the last commit is wrong?

What would you like to see adjusted?

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279044460)
I think we can save further adjustments to the CI system for a followup PR.

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1658020560)
> What would you like to see adjusted?

I think the link to the external repo can be dropped, given that the source code lives in this repo, and linking to an outside one could increase confusion as to which place is the correct one to submit patches to?

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279050723)
Then maybe drop the commit. Otherwise it seems odd to touch the same line of code twice, where the first one doesn't really have a rationale?

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#issuecomment-1658024064)
> I think the link to the external repo can be dropped,

Ok.

Also fixed the lint regex.

💬 willcl-ark commented on pull request "ci: Use qemu-user through container engine":
(https://github.com/bitcoin/bitcoin/pull/28087#issuecomment-1658031353)
@MarcoFalke

> Yes, this is expected. Instead of just running `bitcoind` through qemu, now the whole container is run through qemu.

Ok running this again... to setup and build Depends still takes more than an hour for me (with it taking 56 mins to progress past `building qt`) which seems relatively annoying. Am I correct that this build will use a depends cache on actual CI though, which should speed it up there significantly?

💬 naumenkogs commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#issuecomment-1658035143)
@pinheadmz yeah this is probably the best attempt at minimizing the problem I mentioned. Whether it's satisfactory is still at question :) I'm looking forward to seeing what others think.

💬 fanquake commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279057067)
The IWYU commit? The rationale was that you asked me above to make the change, which is the only reason I included it in this PR.

🚀 fanquake merged a pull request: "qa, doc: Fix comment"
(https://github.com/bitcoin/bitcoin/pull/28181)

💬 MarcoFalke commented on pull request "ci: Integrate `bitcoin-tidy` clang-tidy plugin":
(https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279066035)
I think I only suggested to move the export: https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1277709470 (not *remove* it).

On a second thought, I am wondering if it makes more sense to do something like https://github.com/bitcoin/bitcoin/pull/26296#discussion_r1279032950 instead.

I mean anything that is correct is fine here, but in light of multiple alternatives, it seems odd to pick one, then revert it, and pick something else? (Sorry for coming up with the second alternative)
...

💬 vasild commented on pull request "util: Type-safe transaction identifiers":
(https://github.com/bitcoin/bitcoin/pull/28107#discussion_r1279068859)
Is it not possible to replace all of that with just:

```cpp
class Txid : public uint256
{
};

class Wtxid : public uint256
{
};
```

It achieves the purpose of not allowing conversion and comparison between `Txid` and `Wtxid` and each one of them can be treated as `uint256`.