🤔 furszy reviewed a pull request: "test: wallet rescan with reorged parent + IsFromMe child in mempool"
(https://github.com/bitcoin/bitcoin/pull/29179#pullrequestreview-1818866732)
Code review ACK df30247705, nits can be disregarded.

💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450823884)
nit:
leftover; should be "import descriptor on another wallet"

💬 furszy commented on pull request "test: wallet rescan with reorged parent + IsFromMe child in mempool":
(https://github.com/bitcoin/bitcoin/pull/29179#discussion_r1450823229)
nit:
leftover
```suggestion
self.log.info("Create a child tx and wait until all wallets are notified")
```

💬 Sjors commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1889817551)
> I'm not aware of a well defined process for making changes to bitcoin consensus (nor do I think there should be one), so I'm not sure how we can be too early in the process.

Here's a good place to start: https://datatracker.ietf.org/doc/html/rfc7282

(Non draft) pull requests to Bitcoin Core are not a good way to find consensus. Opening before such consensus most likely decreases of anyone being motivated to review it.

> CTV had (a year of?) workshops and review with Jeremy Rubin abo
...

💬 theuni commented on pull request "build: depends move macOS C(XX) FLAGS out of C & CXX":
(https://github.com/bitcoin/bitcoin/pull/29233#issuecomment-1889823070)
Hmm. I was curious to see if the compiler forwards `-mmacosx-version-min` to the linker, and it seems it does:
```bash

$ clang -mmacosx-version-min=11.2 test.c -c -o test.o

$ clang -mmacosx-version-min=11.2 test.o -o testing -v
Apple clang version 14.0.3 (clang-1403.0.22.14.1)
Target: arm64-apple-darwin22.5.0
Thread model: posix
InstalledDir: /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin
"/Applications/Xcode.app/Contents/Developer/Toolchain
...

🤔 mzumsande reviewed a pull request: "log mempool loading progress"
(https://github.com/bitcoin/bitcoin/pull/29227#pullrequestreview-1818890399)
tested ACK eb78ea4eebfe150bc1746282bfdad6eb0f764e3c

💬 reardencode commented on pull request "OP_CHECKTEMPLATEVERIFY, OP_CHECKSIGFROMSTACK(VERIFY), OP_INTERNALKEY validation (LNHANCE)":
(https://github.com/bitcoin/bitcoin/pull/29198#issuecomment-1889850492)
> > I'm not aware of a well defined process for making changes to bitcoin consensus (nor do I think there should be one), so I'm not sure how we can be too early in the process.
>
> Here's a good place to start: https://datatracker.ietf.org/doc/html/rfc7282
>
> (Non draft) pull requests to Bitcoin Core are not a good way to find consensus. Opening before such consensus most likely decreases of anyone being motivated to review it.

As I mentioned on my [post on delving](https://delvingbit
...

🤔 jonatack reviewed a pull request: "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo"
(https://github.com/bitcoin/bitcoin/pull/29058#pullrequestreview-1818940008)
Post-merge utACK, pending digging a bit deeper into the IRL effects.

💬 jonatack commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1450871488)
> a single connection / reconnection attempt doesn't waste a meaningful amount of resources of either side

Note that every I2P connection requires building 2 tunnels, one for each direction. These are meant to be long-lived connections, as they take network resources and waiting for tunnels to be built for every peer connection adds delay to connection setup time.

💬 jonatack commented on pull request "net, cli: use v2transport for manual/addrfetch connections, add to -netinfo":
(https://github.com/bitcoin/bitcoin/pull/29058#discussion_r1450864532)
9eed22e nit, code unneeded until/unless there is a `grant`
```diff
@@ -2316,12 +2316,12 @@ void CConnman::ProcessAddrFetch()
strDest = m_addr_fetches.front();
m_addr_fetches.pop_front();
}
- // Attempt v2 connection if we support v2 - we'll reconnect with v1 if our
- // peer doesn't support it or immediately disconnects us for another reason.
- const bool use_v2transport(GetLocalServices() & NODE_P2P_V2);
- CAddress addr;
CSemaphoreGrant grant(*
...

💬 murchandamus commented on pull request "wallet: Add CoinGrinder coin selection algorithm":
(https://github.com/bitcoin/bitcoin/pull/27877#issuecomment-1889878218)
I’ve added another fuzz target that validates the output of CoinGrinder against a bruteforce search for the smallest weight input set.

👍 jamesob approved a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1818940398)
ACK d63b2e88780dc78fd531b053653361a0bf3fcbea ([`jamesob/ackr/28960.1.TheCharlatan.kernel_remove_dependency`](https://github.com/jamesob/bitcoin/tree/ackr/28960.1.TheCharlatan.kernel_remove_dependency))

Reviewed and ran tests locally. Changes are pretty straightforward and common-sense.

Putting event queue execution into the hands of libbitcoinkernel users seems like the right design choice, and it is especially motivating that this change helps novel fuzz testing (#29158).

As an aside,
...

💬 jamesob commented on pull request "kernel: Remove dependency on CScheduler":
(https://github.com/bitcoin/bitcoin/pull/28960#discussion_r1450864735)
https://github.com/bitcoin/bitcoin/pull/28960/commits/d63b2e88780dc78fd531b053653361a0bf3fcbea

I guess this periodic call was originally included to demonstrate how you might use the scheduler from the libbitcoinkernel interface, but now that we've removed the scheduler from the interface there isn't really an equivalent. Clients would be expected to run their own async event managers and periodic schedulers, which makes sense - and I guess is the whole point of this change!

💬 kristapsk commented on issue "rpc: actually deprecate `rpcuser` & `rpcpass`":
(https://github.com/bitcoin/bitcoin/issues/29240#issuecomment-1889887704)
> If we are going to deprecate these options, now seems like a good time to do so. We could start with updating all exmaples / docs in this repository, to remove any `rpcuser`/`rpcpass` usage.

That should definitely be done first.

One thing with cookie auth, that currently needs hacks, is allowing cookie access for other users. #28167

💬 fjahr commented on pull request "During IBD, prune as much as possible until we get close to where we will eventually keep blocks":
(https://github.com/bitcoin/bitcoin/pull/20827#issuecomment-1889900815)
utACK d298ff8b62b2624ed390c8a2f905c888ffc956ff

While there may be slightly better configurations possible, as @0xB10C pointed out above, I think the 1MB assumption is alright and this is a clear improvement already, so this can be merged and potentially improved later.

🤔 sr-gi reviewed a pull request: "test/BIP324: functional tests for v2 P2P encryption"
(https://github.com/bitcoin/bitcoin/pull/24748#pullrequestreview-1816369526)
First pass, untested, will do a more thorough review next week

💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449320797)
nit: feels like this is something that you could do on `__init__()` instead of creating an empty array. It's not a big deal though, so feel free to disregard

💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449286884)
nit: queued / queue of messages / messages queued

💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449289058)
I think this field is redundant. This will always be `True` if `v2_state` is set, and `False` otherwise, so we could drop it and just check against whether v2_state is `None`.

If you think that's too verbose, you could also create a helper method:

```python
def supports_v2_p2p(self):
return self.v2_state is not None
```

💬 sr-gi commented on pull request "test/BIP324: functional tests for v2 P2P encryption":
(https://github.com/bitcoin/bitcoin/pull/24748#discussion_r1449336950)
nit: space missing after responder, also I guess add "the" before to match the previous suggestion if you happen to take it