💬 MarcoFalke commented on pull request "build: Bump minimum supported Clang to clang-10":
(https://github.com/bitcoin/bitcoin/pull/27682#discussion_r1196171608)
Thanks, done.

💬 MarcoFalke commented on pull request "test: Add missed header":
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551017016)
Any understanding why or when this happened?

💬 fanquake commented on pull request "test: Add missed header":
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551020550)
I think it was https://github.com/bitcoin/bitcoin/pull/26715.

💬 MarcoFalke commented on pull request "test: Add missed header":
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551028017)
But CI passed on the pull and merge commit (https://github.com/bitcoin/bitcoin/runs/13481551706), no?

💬 hebasto commented on pull request "msvc: Rename `libbitcoinconsensus` to `libbitcoin_consensus` and other adjustments":
(https://github.com/bitcoin/bitcoin/pull/27615#issuecomment-1551029540)
> Other than renaming things, what does this acheive?

It is [required](https://github.com/bitcoin/bitcoin/pull/24773#issuecomment-1541888040) for https://github.com/bitcoin/bitcoin/pull/24773.

> Unclear it's worth doing if everything done here is going to be thrown away when we switch to CMake.

Sure. Not a priority at all.

💬 hebasto commented on pull request "test: Add missed header":
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551033551)
The MSVC link issue was kind of intermittent. I guess, it was dependent on the actual order of processing of the source files.

💬 MarcoFalke commented on pull request "test: Add missed header":
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551036780)
Ok, interesting. Maybe with iwyu this kind of bug will be fixed eventually :smiling_face_with_tear:

💬 fanquake commented on pull request "test: Add missed header":
(https://github.com/bitcoin-core/gui/pull/729#issuecomment-1551037870)
Yea. I guess the compiler is broken? Developers should not have to worry about putting files in the "right order" for it.

👍 hebasto approved a pull request: "build: Bump minimum supported Clang to clang-10"
(https://github.com/bitcoin/bitcoin/pull/27682#pullrequestreview-1430199164)
ACK fa199ee614a7ed99c6caf329093a3573ea5a664b

💬 jamesob commented on issue "Use muhash for assumeUTXO snapshot ":
(https://github.com/bitcoin/bitcoin/issues/27669#issuecomment-1551051475)
See the proposal for why I didn't use muhash to begin with: https://github.com/jamesob/assumeutxo-docs/tree/2019-04-proposal/proposal#how-will-users-and-reviewers-efficiently-verify-hashes-of-a-given-utxo-set

That said, the original rationale (supporting snapshot chunks) doesn't seem as relevant anymore, so this might be worth revisiting...

💬 Sjors commented on issue "Proposal for a new mempool design":
(https://github.com/bitcoin/bitcoin/issues/27677#issuecomment-1551056767)
Nice! It's also worth noting that although this proposal introduces a new limit, for the cluster size, it could potentially supersede some other limits (e.g. `-limitancestorcount`, `-limitancestorsize`, `-limitdescendantcount`, `-limitdescendantsize`) if the maximum cluster size is large enough. Though if the limit is in vbytes, then someone assuming a limit in count might still hit a non backwards compatible policy limit.

💬 hebasto commented on pull request "build: Bump minimum supported Clang to clang-10":
(https://github.com/bitcoin/bitcoin/pull/27682#issuecomment-1551090163)
Guix builds:
```
4837adede6c57a98c0bc0ddaa178870a543c3f6ce9c3c8cd47841262937637c7 guix-build-fa199ee614a7/output/arm64-apple-darwin/SHA256SUMS.part
7ff35a219b09300f27a03b00196367ae9f17effab610e440a60322b0dd6a22e8 guix-build-fa199ee614a7/output/arm64-apple-darwin/bitcoin-fa199ee614a7-arm64-apple-darwin-unsigned.dmg
4dcf0408d81b0558f30055749334b966b8b1d2269da32238ff1f219e3e027bfc guix-build-fa199ee614a7/output/arm64-apple-darwin/bitcoin-fa199ee614a7-arm64-apple-darwin-unsigned.tar.gz
169fe
...

📝 fanquake opened a pull request: "ci: remove `RUN_SECURITY_TESTS`"
(https://github.com/bitcoin/bitcoin/pull/27683)
We no-longer run any security/syymbol checks in the CI, and doubt we will in future (if we do, it'll be via Guix, where this var would be redundant in any case). The CI environment doesn't (exactly) match the release build environment (and is semi-regularly changing), and the binaries produced in the CI don't match how we build release binaries, so there is no point trying to run these checks, especially as we add more involved tests, i.e #26953.

💬 fanquake commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1551101900)
> @fanquake What sets RUN_SECURITY_TESTS?

Nothing. symbol/security checks are no-longer run in the CI, because the CI environment is semi-regularly changing, and the binaries produced don't represent release binaries, meaning there are some tests that will never pass. `RUN_SECURITY_TESTS` is leftover from when we used run (some) tests; I've opened a PR to remove it (#27683). The current way to run the security/symbol checks is to perform a Guix build.

💬 fanquake commented on pull request "ci: Use `macos-ventura-xcode:14.1` image for "macOS native" task":
(https://github.com/bitcoin/bitcoin/pull/26388#issuecomment-1551106363)
> As https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1550459263, this seems to have removed RUN_SECURITY_TESTS for x86 too?

See #27683.

📝 hebasto opened a pull request: "Avoid lock order inversion in `Chainstate::ConnectTip` function "
(https://github.com/bitcoin/bitcoin/pull/27684)
Due to the synchronous call of `CValidationInterface::BlockChecked` a lock order inversion [happens](https://github.com/bitcoin/bitcoin/issues/19303#issuecomment-1514928912):
```
PeerManagerImpl::m_peer_mutex
|
V
Peer::TxRelay::m_tx_inventory_mutex
|
V
CTxMemPool::cs
|
V
PeerManagerImpl::m_peer_mutex
```

This PR breaks the last link.

The other possible solution is to move `CValidationInterface::BlockChecked` to a background thread (see https://github.com/bitcoin/bit
...

💬 fanquake commented on pull request "macOS: Bump minimum required runtime version and prepare for building with upstream LLVM":
(https://github.com/bitcoin/bitcoin/pull/27676#issuecomment-1551127526)
The CI failure is unrelated, and will be fixed post-rebase. Guix currently fails to build. i.e:
```bash
time HOSTS="x86_64-apple-darwin arm64-apple-darwin" ./contrib/guix/guix-build
<snip>
Extracting libevent...
/home/ubuntu/sources/libevent-2.1.12-stable.tar.gz: OK
Preprocessing libevent...
Configuring libevent...
checking for a BSD-compatible install... /home/ubuntu/.guix-profile/bin/install -c
checking whether build environment is sane... yes
checking for x86_64-apple-darwin-strip..
...

💬 hebasto commented on pull request "ci: remove `RUN_SECURITY_TESTS`":
(https://github.com/bitcoin/bitcoin/pull/27683#issuecomment-1551132082)
> We no-longer run any security/symbol checks in the CI, and doubt we will in future (if we do, it'll be via Guix, where this var would be redundant in any case).

Concept ACK on that.

💬 MarcoFalke commented on pull request "Avoid lock order inversion in `Chainstate::ConnectTip` function":
(https://github.com/bitcoin/bitcoin/pull/27684#discussion_r1196280935)
Can you explain why this is fine in the context of logical races, as well as performance-wise in the context of new thread(pools) getting spawned?

💬 hebasto commented on pull request "Avoid lock order inversion in `Chainstate::ConnectTip` function":
(https://github.com/bitcoin/bitcoin/pull/27684#discussion_r1196283047)
Is `MaybePunishNodeForBlock` performance critical?