💬 stickies-v commented on pull request "rest: bugfix, fix crash error when calling `/deploymentinfo`":
(https://github.com/bitcoin/bitcoin/pull/27853#discussion_r1226690678)
Yeah I get that, but I don't see why we wouldn't just run the hash-specified test immediately on the older hash to avoid doing this twice? Also just querying the previous hash seems more efficient than running generate?

Upon further thought, I think it does indeed make sense to include it in this PR.

```diff
diff --git a/test/functional/interface_rest.py b/test/functional/interface_rest.py
index 559249101..1ba8f60d9 100755
--- a/test/functional/interface_rest.py
+++ b/test/functional/i
...

💬 instagibbs commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1226708783)
IIRC circles back to the diamond problem: https://github.com/bitcoin/bitcoin/pull/27609#issuecomment-1542695316

Both the parents should contribute to the grandparent being included, but in ancestor packages we can't take that into account(without a common descendant). Instead the whole ancestor package is included, even though the child is likely to be immediately evicted.

💬 ismaelsadeeq commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1587414647)
Post Merge code review ACK faa2976a56ea7cdfd77ce2580a89ce493b57b5d4

This PR removes `mapRelay` and introduces `m_recent_block_txs`.
`m_recent_block_txs` retains the transactions of the most recent mined block.
When a new block is mined, all the block transactions in our mempool are dropped, because of `BLOCK` `MemPoolRemovalReason`.

- Storing the most recently mined block transactions in `m_recent_block_txs` allows us to relay the dropped transactions to peers upon request, which we mig
...

💬 sipa commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587485219)
I'm Concept ACK on eventually replacing the current `getblocktemplate` with another interface, for the following reasons:
* The poll-based nature introduces unnecessary latency over a hypothetical push-based one.
* Being RPC based, `getblocktemplate` is bandwidth and CPU inefficient due to JSON and hex encoding (for large amounts of data being transferred on every call).
* `getblocktemplate` provides too much information for clients that do not care about performing their own transaction sele
...

💬 brunoerg commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1226788428)
Why did you add one more node?

💬 brunoerg commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#issuecomment-1587500451)
> So we would still need to track the elapsed time to not run the check more often than necessary. So I think just using the 24h scheduler is cleaner and just as good.

I agree.

💬 brunoerg commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1226805905)
Since `test_asmapp_health_check` is called after `test_empty_asmap`, node0 is not running, so we don't need to stop it here again.

💬 brunoerg commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1226815492)
```suggestion
void ASMapHealthCheck(const std::vector<CNetAddr>& clearnet_addrs) const;
```

💬 brunoerg commented on pull request "net: Continuous ASMap health check":
(https://github.com/bitcoin/bitcoin/pull/27581#discussion_r1226815793)
nit:
```suggestion
for (const auto& addr : clearnet_addrs) {
```

💬 MarcoFalke commented on pull request "fuzz: Fix mini_miner_selection running out of coin":
(https://github.com/bitcoin/bitcoin/pull/27806#discussion_r1226828650)
Style nit (only if you retouch), feel free to ignore: Can be written shorter via the included check in `at`:

```suggestion
auto prevout = available_coins.at(0);
```

💬 Fi3 commented on pull request "[WIP] add a stratum v2 template provider":
(https://github.com/bitcoin/bitcoin/pull/27854#issuecomment-1587553321)
> An alternative could still be a more "neutral" interface that is also push-based, binary, and efficient, and keep the StratumV2 logic as a separate daemon.

Given that Sv2 logic for TP is already minimal. The option of not using an already existent interface (zmq,rpc) neiter using the already specified Sv2 interface but redesign from scratch a new one, do not make much sense to me.

>
Advantages:
Less complexity in Bitcoin Core to review and maintain.
Less tied to
...

💬 pinheadmz commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1226843760)
I am guessing we need a new global here because we can't otherwise access `NodeContext::exit_status`?

👍 pinheadmz approved a pull request: "Return EXIT_FAILURE on post-init fatal errors"
(https://github.com/bitcoin/bitcoin/pull/27708#pullrequestreview-1475189635)
ACK 61c569ab6069d04079a0831468eb713983919636

A few non-breaking questions below. Reviewed each commit and ran all tests locally. Confirmed that #2039 behavior now exits with code `1` instead of `0` like it did on master. Also confirmed that remaining `StartShutdown()` calls are all user-triggered.

<details><summary>Show Signature</summary>

```
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

ACK 61c569ab6069d04079a0831468eb713983919636
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdF
...

💬 pinheadmz commented on pull request "Return EXIT_FAILURE on post-init fatal errors":
(https://github.com/bitcoin/bitcoin/pull/27708#discussion_r1226838300)
Does this make `InitShutdownState()` required before calling `AbortNode()` ?
I suppose that could be added to the comment here. I'm just making sure I understand, I don't think you need to retouch:

https://github.com/bitcoin/bitcoin/blob/6f5f37eefd425faabd9a97a3c3028395c34f08c4/src/shutdown.h#L14-L17

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#discussion_r1226851026)
Done as suggested

💬 achow101 commented on pull request "wallet: Have the wallet store the key for automatically generated descriptors":
(https://github.com/bitcoin/bitcoin/pull/26728#issuecomment-1587570689)
Added a commit to make sure that migrated wallets also have the global hd key.

💬 mzumsande commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1226797402)
Is the use of `ConnectedThroughNetwork()` instead of `addr.GetNetwork()` on purpose?
The name and the extra logic for inbound onions suggest that it's tailored towards inbound connections, and there is the subtle difference that it calls `addr.GetNetClass()` instead of `addr.GetNetwork()`, which means a different behavior for linked IPV4 addresses.

I think that in general it would be good to have more consistency between these two functions, which is out of scope for this PR.
But we should
...

💬 mzumsande commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1226781222)
nit: This belongs in the first commit, not the second.

💬 pinheadmz commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1226868475)
@LarryRuane you are correct but what we are changing (in the current state of this branch) is that `noban` nodes can now force disconnection of other peers. Since many users may already have `noban` set, maybe even for a large range of IPs, this branch would introduce a new vulnerability they may not be aware of. Because of that I think it does make more sense to specify a new permission so users can narrow the attack surface

💬 pinheadmz commented on pull request "net: Allow inbound whitebind connections to more aggressively evict peers when slots are full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1226882383)
sure!