💬 luke-jr commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#discussion_r1466685406)
I don't think Windows has strict defaults on permissions for new files, with regard to other users. [This StackExchange article](https://superuser.com/questions/242783/set-default-access-control-list-acl-to-everyone-when-creating-a-new-file-or-fo) at least suggests the default can be manipulated manually.

Also, typically when a feature is disabled at build time, we don't add it to argsman (except perhaps as a hidden_args, but I think that may be a bug in such cases).

💬 ryanofsky commented on pull request "refactor: Compile unreachable walletdb code":
(https://github.com/bitcoin/bitcoin/pull/29315#issuecomment-1910633588)
> This wouldn't work, because the `#ifdef` is still needed to "erase" the `MakeSQLiteDatabase` code. Recall that the header that declares `MakeSQLiteDatabase` isn't included when sqlite isn't selected by configure. (Same for bdb)

Good point. There could be a number of ways to fix this (rearranging headers, using forward declarations), but probably not worth it

👍 ryanofsky approved a pull request: "kernel: Remove dependency on CScheduler"
(https://github.com/bitcoin/bitcoin/pull/28960#pullrequestreview-1844264173)
Code review ACK 536429372dfd9759dc8f089962f3a15a94b54751

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466699638)
I got rid of sleep, in favor of `docker rm -f $CONTAINER_NAME` (haven't pushed that change yet, pending discussion on podman)

https://stackoverflow.com/questions/57631654/how-to-wait-for-a-docker-container-to-be-removed

⚠️ jsarenik opened an issue: "getdescriptorinfo returns unusable descriptor"
(https://github.com/bitcoin/bitcoin/issues/29320)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

NOTE: Replace `bch.sh` below in the code with `bitcoin-cli -signet`.

Let's have a random new testnet descriptor wallet with following private descriptors:

```sh
~/.bitcoin/signet$ bch.sh listdescriptors true | jq .descriptors[].desc | tail -1
"wpkh(tprv8ZgxMBicQKsPdpPsVvhB5XZtRfRWGqH1uwJNmG27dQgDwTS76oCd3hKbn3zERUZjG2fCgdKe8rqT2NomNbCgGSUQ829rJkTLUWvGKzyHX7Z/84h/1h/0h/1/*)#duvgfk6
...

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466702521)
The alternative polling approach suggested in that answer might be worth considering:

> it does a SIGKILL rather than the SIGTERM followed by SIGKILL the way docker stop does

Not sure how much this difference matters in our use.

💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466705109)
> Why do we add a dependency on podman at all?

A dependency on a container engine is needed, because this repo is the wrong place to implement a container engine from scratch.

> Is the plan to move to podman entirely?

For running the CI locally, any container engine should work. Inside the `RESTART_CI_DOCKER_BEFORE_RUN`, only podman is supported.

If you want to switch to docker, the podman commands would have to replaced by docker commands inside the `RESTART_CI_DOCKER_BEFORE_RUN` bl
...

💬 pinheadmz commented on issue "getdescriptorinfo returns unusable descriptor":
(https://github.com/bitcoin/bitcoin/issues/29320#issuecomment-1910678143)
Great catch! To summarize, the RPC is returning an extended PUBLIC key followed by a hardened derivation path. Which is usable.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466715496)
Oh, I see where my confusion comes from. I thought podman was just a wrapper for Docker. But it's actually an alternative which can run the same containers. Why not switch over entirely to Podman? It's quite confusing that we're using both.

💬 theStack commented on pull request "test: p2p: check disconnect due to lack of desirable service flags":
(https://github.com/bitcoin/bitcoin/pull/29279#issuecomment-1910708559)
> Concept ACK! i think flaky tests are possible because disconnection could happen before [`p2p_conn.wait_for_connect()`](https://github.com/bitcoin/bitcoin/blob/7699a1aab8fa2a7a602391025f3cfcde29ff2613/test/functional/test_framework/test_node.py#L728) in `add_outbound_connection()`?

True, good catch. The flaky case can be reproduced by adding an artificial delay before the `wait_for_connect()` call, e.g.:
```diff
diff --git a/test/functional/test_framework/test_node.py b/test/functional/te
...

📝 theStack converted_to_draft a pull request: "test: p2p: check disconnect due to lack of desirable service flags"
(https://github.com/bitcoin/bitcoin/pull/29279)
This PR adds missing test coverage for disconnecting peers which don't offer the desirable service flags in their VERSION message:
https://github.com/bitcoin/bitcoin/blob/5f3a0574c45477288bc678b15f24940486084576/src/net_processing.cpp#L3384-L3389
This check is relevant for the connection types "outbound-full-relay", "block-relay-only" and "addr-fetch" (see `CNode::ExpectServicesFromConn(...)`). Feeler connections always disconnect, which is also tested here.

In lack of finding a proper file
...

👍 Retropex approved a pull request: "Add a `-permitbarepubkey` option"
(https://github.com/bitcoin/bitcoin/pull/29309#pullrequestreview-1844359954)
tACK https://github.com/vostrnad/bitcoin/commit/8c1114aa61c46e58efb44368b5428d3ccb65f9d0

Steps to test:

1. Build [vostrnad branch](https://github.com/vostrnad/bitcoin/tree/permitbarepubkey)
1. Start a regtest with `-chain=regtest`
1. Create a wallet with `bitcoin-cli createwallet`
1. Get address with `bitcoin-cli getnewaddress`
1. Generate blocks to get bitcoins `generatetoaddress 500 <address obtained previously>`
1. Create a P2PK tx, this [repository](https://github.com/dianerey/bec
...

💬 luke-jr commented on pull request "init: Add option for rpccookie permissions (replace 26088)":
(https://github.com/bitcoin/bitcoin/pull/28167#issuecomment-1910735981)
Actually, why wouldn't fs::permissions work on Windows?

💬 MarnixCroes commented on pull request "debugwindow: update session ID tooltip":
(https://github.com/bitcoin-core/gui/pull/788#issuecomment-1910736573)
> Concept ACK
>
> nit: @MarnixCroes, maybe you can add the screenshot for transport v1 and session ID is empty/ not shown, if possible, even this PR doesn't affect that logic.

done

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1466747189)
old comment?

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1466751943)
at the introductory commit, I don't see how `m_ancestors_of_in_package_ancestors` is non-empty. unused?

I suspect replaced by `package_with_ancestors` usage?

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1466727264)
`has_mempool_sibling` since I'm thinking in reference to `ptx` and it's a binary result?

🤔 instagibbs reviewed a pull request: "v3 transaction policy for anti-pinning"
(https://github.com/bitcoin/bitcoin/pull/28948#pullrequestreview-1844316858)
`PreCheck`s loop looks so much cleaner, but there's some dead code and like before I'm not super clear which checks are *required* in `AcceptMultipleTransactions` vs anti-DoS type checks. For example, does `ApplyV3Rules` matter inside `AcceptMultipleTransactions`?

I'd like it to be crystal clear to not introduce regressions, and moving forward to cluster mempool we keep all this in mind for future simplification.

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1466746854)
can assert that `package_with_ancestors.ancestor_counts.size() == workspaces.size()`?

💬 instagibbs commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#discussion_r1466754879)
unused