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

🤔 MarnixCroes reviewed a pull request: "doc: clarify loadwallet path loading for wallets"
(https://github.com/bitcoin/bitcoin/pull/30302#pullrequestreview-2378402702)
cACK good clarification

💬 MarnixCroes commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1806678697)
```suggestion
"\nLoad wallet from the wallet dir:\n"
```
this is easier to understand imo, and is correct for default cases and when -walletdir is specified

💬 MarnixCroes commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1806705701)
```suggestion
+ HelpExampleCli("loadwallet", "\"/path/to/walletname/\"")
```
more clear imo
same for other ones

💬 MarnixCroes commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1806697883)
in what case would a user have this?
seems non standard, and not worth having an example for imo.

💬 MarnixCroes commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1806675500)
```suggestion
+ HelpExampleCli("loadwallet", "\"walletname\"")
```
_walletname_ is easier to understand than _foo_ imo

💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1806727152)
I think this should be `const CTransactionRef& tx` (and in the header file as well)

💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1806730678)
Perhaps we could let `bypass_limits` be filled in by the fuzzer, rather than hardcoded to false?

💬 sdaftuar commented on pull request "Ephemeral Dust":
(https://github.com/bitcoin/bitcoin/pull/30239#discussion_r1806765767)
Overall I think this is much easier to follow with the codepaths merged, thanks. However on further review, I am concerned about how much work we might do in this function.

As written, if a transaction had 1000 inputs, which all spend from a single parent with 1000 outputs, then we'd do 1M loop iterations, because we're not deduplicating parents for each input of a transaction (so for each input of the child, we'd be looking at all 1000 outputs of the parent). If we deduplicate the parents
...