🤔 hebasto reviewed a pull request: "kernel: Rm ShutdownRequested and AbortNode from validation code."
(https://github.com/bitcoin/bitcoin/pull/27861#pullrequestreview-1503103864)
ACK 6eb33bd0c21b3e075fbab596351cacafdc947472, I have reviewed the code and it looks OK.

💬 hebasto commented on pull request "kernel: Rm ShutdownRequested and AbortNode from validation code.":
(https://github.com/bitcoin/bitcoin/pull/27861#discussion_r1245218670)
Would it be more safer to move the assertion to the top of the desctuctor's code?

💬 Sjors commented on pull request "wallet: bugfix, always use apostrophe for spkm descriptor ID":
(https://github.com/bitcoin/bitcoin/pull/27920#issuecomment-1611425177)
What happened to fa2329285c?

💬 fanquake commented on pull request "guix: Clean up manifest":
(https://github.com/bitcoin/bitcoin/pull/27811#issuecomment-1611440749)
Guix Build:
```bash
6edd213a90a043d09da144b99332ab16a320774098d35de28464262b25f260b2 guix-build-a51d7abf1e13/output/aarch64-linux-gnu/SHA256SUMS.part
1ececa8121d51a5c358e25e1a4b529413faadef5721387005db2b928c05cad3d guix-build-a51d7abf1e13/output/aarch64-linux-gnu/bitcoin-a51d7abf1e13-aarch64-linux-gnu-debug.tar.gz
fbc1724995b2d9a43c0bccbb3c20e02d115d9aeaecce0792bde46d9fedc0dbe0 guix-build-a51d7abf1e13/output/aarch64-linux-gnu/bitcoin-a51d7abf1e13-aarch64-linux-gnu.tar.gz
8a9edb553d6e6db1
...

📝 fanquake opened a pull request: "build: pass sanitize flags to instrument libsecp256k1 code"
(https://github.com/bitcoin/bitcoin/pull/27991)
secp256k1 functions are now instrumented:
```bash
# objdump --disassemble-symbols=_secp256k1_xonly_pubkey_serialize src/test/fuzz/fuzz | grep sanitizer_cov
100eb5244: 73 5d 01 94 bl 0x100f0c810 <___sanitizer_cov_trace_const_cmp8>
100eb53f0: cc 5c 01 94 bl 0x100f0c720 <___sanitizer_cov_trace_pc_indir>
100eb5418: c2 5c 01 94 bl 0x100f0c720 <___sanitizer_cov_trace_pc_indir>
100eb5444: b7 5c 01 94 bl 0x100f0c720 <___sanitizer_cov_trace_pc_indir>
```

Not sure if this is the best solutio
...

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