💬 hebasto commented on pull request "build: pass sanitize flags to instrument libsecp256k1 code":
(https://github.com/bitcoin/bitcoin/pull/27991#issuecomment-1611454453)
Is it guaranteed that sanitizer flags for a C++ compiler are acceptible by a C compiler?

💬 ryanofsky commented on pull request "index: make startup more efficient":
(https://github.com/bitcoin/bitcoin/pull/27607#discussion_r1245253634)
I'd definitely avoid moving `StartIndexes` into the `interfaces::Chain` class, because because that class is supposed to expose limited node functionality to outside callers like wallets and indexes without giving them access to node internals. It is supposed to be a thin wrapper around node functions and data, not to contain any real functionality itself. Its methods are meant to be called by wallets and indexes, not by node code because it would add an unnecessary layer of indirection, bring i
...

💬 fanquake commented on pull request "ci: Start with clean env":
(https://github.com/bitcoin/bitcoin/pull/27976#issuecomment-1611469491)
Concept ACK

💬 dergoegge commented on pull request "build: pass sanitize flags to instrument libsecp256k1 code":
(https://github.com/bitcoin/bitcoin/pull/27991#issuecomment-1611472816)
Concept ACK

All of the CI task that make use of the `integer` sanitizer are probably gonna fail.

💬 furszy commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1611484179)
> What happened to [fa23292](https://github.com/bitcoin/bitcoin/commit/fa2329285c918eb7a627f31df2a493b967a9d660)?

Vanished after #24914 merge :).

We no longer iterate over all the database records without any ordering. Now the wallet loads each specific group of records separately (e.g. legacy records, descriptors records, txs, etc).

The reason behind fa232928 was that the upper level method `LoadWallet` was only treating few specific `ReadKeyValue` errors as corruption ones (we were ad
...

🤔 vasild reviewed a pull request: "test: cover addrv2 anchors by adding TorV3 to CAddress in messages.py"
(https://github.com/bitcoin/bitcoin/pull/27452#pullrequestreview-1502905052)
Approach ACK b5e0398d8ced2ee998756e67fb90bc8d604d4ec3

💬 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_r1245093887)
Can this use `self.ADDRV2_NET_NAME`?

```suggestion
assert self.net in self.ADDRV2_NET_NAME
```

💬 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_r1245108107)
When would this socket be closed if `keep_alive` is true?

💬 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_r1245183148)
The port is serialized last, but in the comment is comes after `networkID`. Would it be better to calculate the message size separately, given an array of addresses as input?

<details>
<summary>[patch] calculate addrv2 message size separately</summary>

```diff
diff --git i/test/functional/p2p_addrv2_relay.py w/test/functional/p2p_addrv2_relay.py
index 23d7c94fd0..f9a8c44be2 100755
--- i/test/functional/p2p_addrv2_relay.py
+++ w/test/functional/p2p_addrv2_relay.py
@@ -20,32 +20,27 @@
...

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