💬 mbed101 commented on issue "Indefinite "Bitcoin Core is shutting down..."":
(https://github.com/bitcoin/bitcoin/issues/27848#issuecomment-1586245678)
i think once you requested shutdown, the "CheckForStaleTipAndEvictPeers()" function should not be scheduled to happen because core is shutting down.. so stale tip checking should be stopped immediately after requesting shutdown

📝 pinheadmz converted_to_draft a pull request: "Add unit & functional test coverage for blockstore"
(https://github.com/bitcoin/bitcoin/pull/27850)
This PR adds unit and functional tests to cover the behavior described in #2039. In particular, that bitcoind will crash on startup if a reindex is requested but the `blk` files are read-only. Eventually this behavior can be updated with https://github.com/bitcoin/bitcoin/pull/27039. This PR just commits the test coverage from #27039 as suggested in https://github.com/bitcoin/bitcoin/pull/27039#issuecomment-1584915782

🤔 fjahr reviewed a pull request: "validate package transactions with their in-package ancestor sets"
(https://github.com/bitcoin/bitcoin/pull/26711#pullrequestreview-1465458909)
Still gathering more context but leaving some first comments.

💬 fjahr commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1225867669)
I understand the rationale, but it seems dangerous to skip validation steps in special cases because of expected checks happening later in another scope (that's how I understand it). Such things can lead to nasty errors down the line.

💬 fjahr commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1219838215)
How about actually checking the result here?

💬 fjahr commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1225870352)
Kind of confusing and `grep` unfriendly that this test helper has the exact same name as that `Package` method. Maybe call it `TestAncestorPackage` or so?

💬 fjahr commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1225898875)
Just checking the last tx doesn't guarantee this alone. Maybe mention here that this works because subpackages are being evaluated by themselves as well?

💬 fjahr commented on pull request "validate package transactions with their in-package ancestor sets":
(https://github.com/bitcoin/bitcoin/pull/26711#discussion_r1225871000)
nit: Developer notes say "Class member variables have a `m_` prefix.".

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225913119)
latest push collects stats as a `CConnman` member, so we don't have to worry about iterating through all nodes

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225913181)
updated to incorporate your suggestions, thanks!

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225913245)
moved to member to prevent iterating through all nodes

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225913270)
done, thanks!

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225913626)
we have now removed any clearnet bundling, so this quirk is no longer relevant

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225914692)
question: I noticed you used array in your implementation but we were using vector. is there a strong reason for that?

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#issuecomment-1586321519)
thank you for the reviews @ajtowns & @vasild 🙌

significant changes with latest push:
* introduce a new member on `CConnman` called `m_network_conn_counts` which is a map from `Network` to an atomic that keeps track of the number of `OUTBOUND_FULL_RELAY` and `MANUAL` connections on the node broken down by network. this means we don't have to iterate through all nodes anymore.
* changed the interval from 1 min to 5 min.
* got rid of any "clearnet" bundling, now we treat IPV4 & IPV6 as s
...

💬 amitiuttarwar commented on pull request "p2p: Diversify automatic outbound connections with respect to networks":
(https://github.com/bitcoin/bitcoin/pull/27213#discussion_r1225915157)
updated to have a small connman member to keep track of counts. now we don't iterate through all peers anymore :)

💬 Giszmo commented on issue "Indefinite "Bitcoin Core is shutting down..."":
(https://github.com/bitcoin/bitcoin/issues/27848#issuecomment-1586325943)
So I killed it, started it, waited to catch up and now Exit took some 5 or so seconds.

I suspect that the issue might have to do with flaky internet.

💬 LarryRuane commented on pull request "test: allow BITCOIN_TEST_PATH to specify working dir":
(https://github.com/bitcoin/bitcoin/pull/26564#issuecomment-1586362907)
Rebased for merge conflict.

💬 sdaftuar commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1586454719)
ACK faa2976a56ea7cdfd77ce2580a89ce493b57b5d4

💬 ajtowns commented on pull request "p2p: Stop relaying non-mempool txs":
(https://github.com/bitcoin/bitcoin/pull/27625#issuecomment-1586490389)
ACK faa2976a56ea7cdfd77ce2580a89ce493b57b5d4