📝 achow101 locked a pull request: "Delete .cirrus.yml"
(https://github.com/bitcoin/bitcoin/pull/28837)
@jackripper235

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`).
...

✅ achow101 closed a pull request: "Update CONTRIBUTING.md"
(https://github.com/bitcoin-core/gui/pull/776)

📝 achow101 locked a pull request: "Update CONTRIBUTING.md"
(https://github.com/bitcoin-core/gui/pull/776)

Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improve coverage are always welcome.
* All other changes should have accompanying unit tests (see `src/test/`) or
functional tests (see `test/`). Contributors should note which tests cover
modified code. If no tests exist for a region of modified code, new tests
should accompany the c
...

🤔 stratospher reviewed a pull request: "test: Make existing functional tests compatible with --v2transport"
(https://github.com/bitcoin/bitcoin/pull/28805#pullrequestreview-1724008322)
> These tests have in common that they use self.restart_node() with changing some unrelated extra_args, which would then cause --v2transport to be dropped.

thanks for the detailed explanation! so the problem was test failures happening when unrelated `extra_args` were passed during `TestNode` restart!

i was also initially confused about whether we really need an extra v2transport argument in `TestNode::__init()__` but https://github.com/bitcoin/bitcoin/pull/28805#issuecomment-1802573479 m
...

💬 stratospher commented on pull request "test: Make existing functional tests compatible with --v2transport":
(https://github.com/bitcoin/bitcoin/pull/28805#discussion_r1388909333)
68a9001: there's a behaviour difference when test is run with `--v2transport` option and `TestNode`(initially `"v2transport=0"`) is restarted with:
- `extra_args` supplied => `TestNode` restarts as v2 node
- `extra_args` not supplied => `TestNode` restarts as v1 node

maybe when `extra_args` is `None`, we could remove `"v2transport"` stuff it gets from the old `self.extra_args` using something like `extra_args = list(filter(lambda s: not s.startswith("-v2transport="), extra_args))`?

👋 TheCharlatan's pull request is ready for review: "[refactor] Check CTxMemPool options in ctor"
(https://github.com/bitcoin/bitcoin/pull/28830)

🤔 naumenkogs reviewed a pull request: "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full"
(https://github.com/bitcoin/bitcoin/pull/27600#pullrequestreview-1724197196)
Not sure if you missed the two comments:
[one](https://github.com/bitcoin/bitcoin/pull/27600/commits/0c0f2a2c3664d5ea81ee31230ddca40863dd76dd#r1387600158)
[two](https://github.com/bitcoin/bitcoin/pull/27600/commits/0c0f2a2c3664d5ea81ee31230ddca40863dd76dd#r1387598983)

💬 naumenkogs commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1389032216)
Apparently, in the previous commit 75868022a904c1f77871abf962bf9b88a9c5faf6 you assign `forced = true;` even for non-`forceInbound` peers.

So basically if a node had 8 connections which are non-`forceInbound` but still did eviction, a real `forceInbound` connection won't be able to join (see how you count nForced). Is that intended? Seems like a bug to me.

💬 naumenkogs commented on pull request "net: Add new permission `forceinbound` to evict a random unprotected connection if all slots are otherwise full":
(https://github.com/bitcoin/bitcoin/pull/27600#discussion_r1389028671)
8c2026848da910fdebff0a9f73e29f1f6ae81e43

So forced actually means two things to you:
1. Before connecting — something that *might* evict another peer
2. After connecting — something that *has* evicted another peer

I think this is confusing and better to use different words. RPC can expose "force_evicted" flag (to cover the latter case and apply to the `Connection`), while "forceInbound" can stay in the network/potential-connection context (the former case).

💬 maflcko commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1389093305)
Seems odd to add a TODO to add a test based on a feature that doesn't even exist.

💬 maflcko commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1389091265)
My diff was a hack, obviously. I don't think it makes sense to escape the datadir "sandbox" and potentially overwrite user data when running a test.

💬 maflcko commented on pull request "test: add assumeutxo wallet test":
(https://github.com/bitcoin/bitcoin/pull/28838#discussion_r1389094288)
nit: `blank` may not be needed? Also, could run a python formatter on this file to fix the whitespace?

💬 maflcko commented on pull request "test: Check error details with assert_debug_log on the assumeutxo invalid hash dump - follow-up #28698":
(https://github.com/bitcoin/bitcoin/pull/28835#issuecomment-1805334263)
lgtm ACK 7de76853728b423339d17f39224cf20305da1832

💬 naiyoma commented on pull request "doc: Add example of mixing private and public keys in descriptors":
(https://github.com/bitcoin/bitcoin/pull/28373#discussion_r1389124162)
I did make the changes suggested above

🚀 fanquake merged a pull request: "test: Check error details with assert_debug_log on the assumeutxo invalid hash dump - follow-up #28698"
(https://github.com/bitcoin/bitcoin/pull/28835)

💬 fanquake commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805441141)
cc @L0laL33tz @theStack

💬 fjahr commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805454319)
Can you link to the intermittent failures? Why not reduce the values instead?

💬 maflcko commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805471333)
> Can you link to the intermittent failures? Why not reduce the values instead?

You can simply run the test in a loop to see it fail. For example:

```
./test/functional/feature_init.py --timeout-factor=.1 --randomseed=4861605774237699286

...

AssertionError: [node 0] bitcoind should have exited within 6s with expected error Error opening block database.

💬 maflcko commented on pull request "test: Avoid intermittent failures in feature_init":
(https://github.com/bitcoin/bitcoin/pull/28831#issuecomment-1805472756)
> Why not reduce the values instead?

Yes, this patch is reducing the value.

💬 maflcko commented on pull request "refactor: Simplify CTxMempool/BlockAssembler fields, remove some external mapTx access":
(https://github.com/bitcoin/bitcoin/pull/28391#discussion_r1389223982)
first commit: I think here the result can not be nullptr, so a reference makes more sense, no?

diff:

```diff
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index e019ab9595..3930280797 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -806,8 +806,8 @@ public:
{
if (!m_node.mempool) return;
LOCK2(::cs_main, m_node.mempool->cs);
- for (const auto& entry : m_node.mempool->entryAll()) {
- notifications.trans
...