💬 stickies-v commented on pull request "[refactor] Cleanup BlockAssembler mempool usage":
(https://github.com/bitcoin/bitcoin/pull/28843#discussion_r1806299068)
I feel a bit uneasy about adding this no-op, it makes understanding the program flow more difficult and I feel like it is a potential footgun. What do you think about asserting the mempool is configured instead?

<details>
<summary>git diff on 60a0f2ac88</summary>

```diff
diff --git a/src/node/miner.cpp b/src/node/miner.cpp
index 9e6954d257..afa410da10 100644
--- a/src/node/miner.cpp
+++ b/src/node/miner.cpp
@@ -141,7 +141,9 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNew
...

🤔 tdb3 reviewed a pull request: "tracing: explicitly cast block_connected duration to nanoseconds"
(https://github.com/bitcoin/bitcoin/pull/29877#pullrequestreview-2377938779)
post merge ACK cd0edf26c07c8c615f3ae3ac040c4774dcc8e650

Left on question, but it's minor.

💬 tdb3 commented on pull request "tracing: explicitly cast block_connected duration to nanoseconds":
(https://github.com/bitcoin/bitcoin/pull/29877#discussion_r1806407824)
`In case the duration unit is off, we'll detect it with this assert`

Looks like this detects a change from a less to a more granular duration unit. Would this detect a change from more granular to less granular (e.g. from nanoseconds back to microseconds)? Unlikely to encounter this though, now that the cast to nanoseconds is occurring.

💬 l0rinc commented on pull request "benchmark: Improve SipHash_32b accuracy to avoid potential optimization issues":
(https://github.com/bitcoin/bitcoin/pull/30349#discussion_r1806410808)
I couldn't reproduce the warning locally, but changed it to modify every value in `val.data()` instead to make sure none of them can be optimized away - would you be so kind and check if this solves the undefined behavior?

💬 kevkevinpal commented on pull request "Package validation: accept packages of size 1":
(https://github.com/bitcoin/bitcoin/pull/31096#issuecomment-2422470853)
Concept ACK

👍 tdb3 approved a pull request: "netinfo: add peer services column and outbound-only peers list"
(https://github.com/bitcoin/bitcoin/pull/30930#pullrequestreview-2378098839)
re ACK 17f8f03ec69b6dd3d61137a39b8f88201ec500dc

Code reviewed diff.

Ran sanity check using the new `outonly` arg:
```
$ build/src/bitcoin-cli -signet -netinfo 1
Bitcoin Core client v28.99.0-17f8f03ec69b signet - server 70016/Satoshi:28.99.0/

<-> type net serv v mping ping send recv txn blk hb addrp addrl age id
in npr nwl2 2 1 2 38 24 1 2 6 13
out full ipv6 nwl 1 82 91 24 24 3 . 1001
...

👍 stickies-v approved a pull request: "init: Some small chainstate load improvements"
(https://github.com/bitcoin/bitcoin/pull/31046#pullrequestreview-2377966456)
ACK 31cc5006c3de4dd6a1f7a238684163956604df45

💬 stickies-v commented on pull request "init: Some small chainstate load improvements":
(https://github.com/bitcoin/bitcoin/pull/31046#discussion_r1806424005)
unrelated nit / probably scope creep but it is a "small chainstate load improvement" so leaving here anyway, can be simplified:

<details>
<summary>git diff on 31cc5006c3</summary>

```diff
diff --git a/src/node/chainstate.cpp b/src/node/chainstate.cpp
index fa8d7a386f..4837135dee 100644
--- a/src/node/chainstate.cpp
+++ b/src/node/chainstate.cpp
@@ -240,11 +240,7 @@ ChainstateLoadResult LoadChainstate(ChainstateManager& chainman, const CacheSize
// A reload of the block inde
...

💬 stickies-v commented on pull request "init: Some small chainstate load improvements":
(https://github.com/bitcoin/bitcoin/pull/31046#discussion_r1806649506)
Hmm, then again, that breaks the pattern here of only (explicitly) returning early for failure, so could also leave as-is.

👍 pinheadmz approved a pull request: "Split CConnman"
(https://github.com/bitcoin/bitcoin/pull/30988#pullrequestreview-2373476515)
ACK 70c2f13f83a5cc740330d0b4af9cbd74515be6b2

Built and ran tests on macos/arm as well as debian/x86.

Synced to signet with this build on a debian server, then synced from that node locally using the macos build.

I left a lot of comments below but most are for myself to track during rebases, indicate to the maintainers the depth that I'm actually reviewing, and also some notes for myself -- I'll be rebaing my non-libevent http server on this banch and consuming the new SockMan.

I have
...

💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1803692315)
41c87ddb3d7a18d2c0fa7eccfbde57e9d6e898c2

Maybe these are expected to be set by callers in future commits, so I'm just leaving myself a note to look for `SO_KEEPALIVE` and `TCP_NODELAY`

💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1805112369)
b96beb27d2b3d06a45644c537c114a08f0ccd285

Love this, great optimization and makes a lot of sense.

💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1803679987)
1bfc1ca9b6b68c6356ffc23ecf01e417152ade95

Why do CNode .addr and .addrBind still need to be CAddress? maybe it makes some sense for .addr if that's where we also store the service flags for easier gossiping, but why would the local bind address need anything besides an IP and port?

💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1805115729)
b96beb27d2b3d06a45644c537c114a08f0ccd285

Why not use `GetNewNodeId()` here? (and below) Don't you have a Connman with its own `m_next_node_id`?

💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1805134391)
50cb52470ea6cc4de5a5e5260b8f308353942ec0

This is great, can be used for timed actions (like locking the wallet) we used to depend on libevent for

💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1805157675)
3bb0145514978092ae966e30693cf619e4034837

the `else {error}` looked backwards to me here at first, might be more clear to use `== 0` instead of `!` but i dunno what the official style is for this.

💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1805124507)
bb5b91d430026c4826a2107c8c1a311518cc97ce

nit s/temporary/temporarily

and above

💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1804970448)
b94f9d338fd0d3b47a4ea6165ce9cde48f3b19bc

Reminding myself to check for a test that covers a child class with i2p true but no Event listener

💬 pinheadmz commented on pull request "Split CConnman":
(https://github.com/bitcoin/bitcoin/pull/30988#discussion_r1804998403)
b94f9d338fd0d3b47a4ea6165ce9cde48f3b19bc

Seems weird to me to move the unique pointer to a connected socket out of sockman, implying that sockman itself doesn't own the connections. Peeking ahead at future commits though I think this gets resolved.

📝 sipa opened a pull request: "Improve parallel script validation error debug logging"
(https://github.com/bitcoin/bitcoin/pull/31112)
Builds on top of #31097. Fixes #30960.

So far, detailed information about script validation failures is only reported when running with `-par=1`, due to a lack of ability to transfer information from the script validation threads to the validation thread. Fix this by extending the `CCheckQueue` functionality to pass more results through than just success/failure, and use this to report the exact Script error, as well as the transaction input in which it occurred.