💬 vasild commented on pull request "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py":
(https://github.com/bitcoin/bitcoin/pull/27452#discussion_r1245278854)
Does this suffice to ensure 1 block-only was opened:

<details>
<summary>[patch] check log at shutdown</summary>

```diff
diff --git i/test/functional/feature_anchors.py w/test/functional/feature_anchors.py
index e7cd642484..327533a340 100755
--- i/test/functional/feature_anchors.py
+++ w/test/functional/feature_anchors.py
@@ -91,29 +91,23 @@ class AnchorsTest(BitcoinTestFramework):
self.log.info("Ensure addrv2 support")
# Use proxies to catch outbound connections t
...

💬 ishaanam commented on pull request "wallet: track mempool conflicts with wallet transactions":
(https://github.com/bitcoin/bitcoin/pull/27307#discussion_r1245286239)
That makes sense, I'll do that using a [scripted diff.](https://github.com/bitcoin/bitcoin/blob/d6ee03507f39223889a5f039c4db7204ddfb91d5/doc/developer-notes.md#scripted-diffs)

👍 ryanofsky approved a pull request: "kernel: Rm ShutdownRequested and AbortNode from validation code."
(https://github.com/bitcoin/bitcoin/pull/27861#pullrequestreview-1503219053)
Code review ACK 6eb33bd0c21b3e075fbab596351cacafdc947472. No changes since last review other than rebase.

If I can make one more suggestion about PR the description, I would suggest to make "Replace all AbortNode calls..." the first and main paragraph, and put everything else that's there below it separated by a horizontal line. I think the other text which is abstract and relies on explanations in linked PRs is going to be intimidating and not to make much sense to people who aren't followin
...

💬 mzumsande commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1245290439)
oh, right, I misunderstood this, can be resolved.

💬 vasild commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#issuecomment-1611504716)
`9c46f3ba19...c1a169082a`: take https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245084467

💬 vasild commented on pull request "test: remove race in the user-agent reception check":
(https://github.com/bitcoin/bitcoin/pull/27986#discussion_r1245300595)
Done.

I only changed the first `p` to `info` because `p = [p for p ...` seemed too many `p`s in one place.
:exploding_head:

💬 vasild commented on issue "Finding peers to connect to after -onlynet changes may be problematic":
(https://github.com/bitcoin/bitcoin/issues/26035#issuecomment-1611534437)
@CocolinoFan what you describe is problem 2 from the OP. A fix for it is at https://github.com/bitcoin/bitcoin/pull/27213. It would be nice if you can test and/or review that.

💬 MarcoFalke commented on issue "libsecp256k1 not instrumented when building with sanitizers":
(https://github.com/bitcoin/bitcoin/issues/27990#issuecomment-1611549924)
Is this needed for anything? I guess it is odd that msan didn't complain about this, but other than that there shouldn't be anything needed here.

💬 dergoegge commented on issue "libsecp256k1 not instrumented when building with sanitizers":
(https://github.com/bitcoin/bitcoin/issues/27990#issuecomment-1611573124)
> Is this needed for anything? I guess it is odd that msan didn't complain about this, but other than that there shouldn't be anything needed here.

Our MSan job actually passes the correct `CFLAGS` otherwise it would (i assume) not work properly. Our oss-fuzz builds are also not affected since they also set their own `CFLAGS` and don't use `--with-sanitizers`.

I think it would be useful to do this for our fuzz targets that call secp code (e.g. the targets added in #27479), otherwise we end
...

📝 fanquake opened a pull request: "ci: build Valgrind (3.21) from source"
(https://github.com/bitcoin/bitcoin/pull/27992)
I've been using this branch for some time, for working Valgrind CI jobs on aarch64. Benefits include:
* Valgrind CI jobs work across x86_64 & aarch64.
* Can use latest (hopefully less buggy) Valgrind, rather than whatever the distro happens to package.
* No need to "bless" a specific compiler, (current discussion includes switching from Clang to GCC as a workaround).
* Valgrind from source runs significantly faster compared to the system package. i.e, when fuzzing under valgrind:

Master:
...

💬 furszy commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1245343374)
ok gotcha about the chain interface.

I thought about it because of the goal of removing the node internal types from the subdirectories. If we stick to that path, the code move into the `src/index/init.cpp` could be seen as a regression.

The good point about moving this code somewhere else is that it could be unit tested without having to add `init.h` and `NodeContext` dependencies.

At least for now, I'm a bit inclined for the `src/node/` option but not feeling strong over it neither. I
...

💬 mzumsande commented on pull request "init: return error when block index is non-contiguous, fix feature_init.py file perturbation":
(https://github.com/bitcoin/bitcoin/pull/27823#issuecomment-1611579553)
> Could rebase for CI?

done

💬 fanquake commented on pull request "test: Use same timeout for all index sync":
(https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1611579793)
> Ah, sorry that's a bug. Can you check log_path=${BASE_SCRATCH_DIR}/sanitizer-output/tsan?

```bash
# cat tsan.25230
==================
WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=25230)
Cycle in lock order graph: M0 (0xffff75666608) => M1 (0xffff7566a868) => M2 (0xffff7566aa18) => M0

Mutex M1 acquired here while holding mutex M0 in main thread:
#0 pthread_rwlock_wrlock <null> (test_bitcoin+0x1537a0) (BuildId: 501b437270597fa9a7a48cb4f66b32eca15a18
...

💬 TheCharlatan commented on pull request "refactor: Move sock from util to common":
(https://github.com/bitcoin/bitcoin/pull/27989#issuecomment-1611583183)
What about all the other networking and wallet related files in `/util`? E.g. we should probably also move `asmap.cpp`, `bip32.cpp`, `bytevectorhash.cpp`, `error.cpp`, `fees.cpp`, `syserror.cpp`, `message.cpp`, and `readwritefile.cpp` out of `/util`. These pure moves of entire files are very straight forward, so I feel like when we do this, we should do all of them at once as part of integrating the kernel library into the internal compilation units. My fear is that this will otherwise create to
...

💬 MarcoFalke commented on pull request "test: Use same timeout for all index sync":
(https://github.com/bitcoin/bitcoin/pull/27988#issuecomment-1611586817)
Yeah, tsan+bdb+aarch64 is impossible to run. You'll have to disable it.

💬 furszy commented on issue "index: ThreadSanitizer: data race on vptr ":
(https://github.com/bitcoin/bitcoin/issues/27355#issuecomment-1611601851)
Hmm interesting.
#27607 is getting quite close now. It's making some substantial changes to the index `Init` and `Start` code.
Will rebase furszy@8495c85 on top and push it so we can try that one too.

💬 MarcoFalke commented on pull request "ci: build Valgrind (3.21) from source":
(https://github.com/bitcoin/bitcoin/pull/27992#issuecomment-1611602478)
> No need to "bless" a specific compiler, (current discussion includes switching from Clang to GCC as a workaround).

I think this is the same as point one, which is about the "Source and destination overlap in memcpy" false positive? If so, it could make sense to combine it into one point.

> Can use latest (hopefully less buggy) Valgrind, rather than whatever the distro happens to package.

Does it mean we drop support for distro-shipped valgrind? Might be good to clarify, and also might
...

💬 fjahr commented on issue "ci: failure in `wallet_basic.py --descriptors`":
(https://github.com/bitcoin/bitcoin/issues/27974#issuecomment-1611648809)
Looks like a dublicate of #27249 ?

💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1245423693)
Thanks for looking into it. I like the idea of keeping them separate and avoiding including unneeded code, in general and for building with slower, older CPUs. So 👍 for your diff in https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1244795380. Great work so far AFAICT. Will continue reviewing the remaining commits.

💬 jonatack commented on pull request "test: add end-to-end tests for CConnman and PeerManager":
(https://github.com/bitcoin/bitcoin/pull/26812#discussion_r1245427231)
> In both cases all tests would be recompiled

Yes, but future changes to `util/net` would not require compiling all the tests.