💬 glozow commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466658531)
I think it's impossible to hit, since we're v3-only and it can't have another ancestor. Maybe we just omit this case? I figured somebody would advocate for having it just in case.
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466658531)
I think it's impossible to hit, since we're v3-only and it can't have another ancestor. Maybe we just omit this case? I figured somebody would advocate for having it just in case.
💬 glozow commented on pull request "v3 transaction policy for anti-pinning":
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1910596660)
Last push incorporated a rework to `PackageV3Checks` from @sdaftuar (thanks), addressing a problem [discussed in irc yesterday](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-01-24)
(https://github.com/bitcoin/bitcoin/pull/28948#issuecomment-1910596660)
Last push incorporated a rework to `PackageV3Checks` from @sdaftuar (thanks), addressing a problem [discussed in irc yesterday](https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-01-24)
💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466665081)
It is hard to debug your issue, but I presume you have docker installed on your system.
In this case, you can't install podman-docker without removing docker. Also, the CI system workers will not clean up after themselves, because the cleanup is done with the `podman` command, which can not clear docker containers.
My recommendation would be to start from a fresh VM (fresh install of Ubuntu) and then follow the docs: https://github.com/bitcoin/bitcoin/blob/ac923e70e7cec603abd207f104dbabfe6
...
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466665081)
It is hard to debug your issue, but I presume you have docker installed on your system.
In this case, you can't install podman-docker without removing docker. Also, the CI system workers will not clean up after themselves, because the cleanup is done with the `podman` command, which can not clear docker containers.
My recommendation would be to start from a fresh VM (fresh install of Ubuntu) and then follow the docs: https://github.com/bitcoin/bitcoin/blob/ac923e70e7cec603abd207f104dbabfe6
...
👍 ryanofsky approved a pull request: "refactor: Compile unreachable walletdb code"
(https://github.com/bitcoin/bitcoin/pull/29315#pullrequestreview-1844216896)
Code review ACK fa3373d3adbace7e4665cf391363319a55a09a96. This looks good, and should prevent code in the else blocks from accidentally breaking.
Maybe a way to make it a little clearer would be to define `use_bdb` and `use_sqlite` c++ constants like:
```
constexpr bool use_bdb{
#ifdef USE_BDB
true
#endif
};
```
Then the actual code could use `if constexpr(use_bsb)` like a normal conditional. This might be more verbose than the current approach, though, and the current approach do
...
(https://github.com/bitcoin/bitcoin/pull/29315#pullrequestreview-1844216896)
Code review ACK fa3373d3adbace7e4665cf391363319a55a09a96. This looks good, and should prevent code in the else blocks from accidentally breaking.
Maybe a way to make it a little clearer would be to define `use_bdb` and `use_sqlite` c++ constants like:
```
constexpr bool use_bdb{
#ifdef USE_BDB
true
#endif
};
```
Then the actual code could use `if constexpr(use_bsb)` like a normal conditional. This might be more verbose than the current approach, though, and the current approach do
...
💬 maflcko commented on pull request "refactor: Compile unreachable walletdb code":
(https://github.com/bitcoin/bitcoin/pull/29315#issuecomment-1910611268)
> Then the actual code could use `if constexpr(use_bsb)` like a normal conditional.
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)
(https://github.com/bitcoin/bitcoin/pull/29315#issuecomment-1910611268)
> Then the actual code could use `if constexpr(use_bsb)` like a normal conditional.
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)
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466679187)
Yes, Docker is installed in rootless mode: https://docs.docker.com/engine/security/rootless/
Forgot to link to the error: https://cirrus-ci.com/task/5247017476161536?logs=ci#L261
The "short-name resolution" error makes no sense to me based on what I can find about it, since the container URL includes `docker.io` (`docker.io/amd64/ubuntu:22.04` in the above example).
> you can't install podman-docker without removing docker
Ok, let's not go that route, since Docker currently works.
...
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466679187)
Yes, Docker is installed in rootless mode: https://docs.docker.com/engine/security/rootless/
Forgot to link to the error: https://cirrus-ci.com/task/5247017476161536?logs=ci#L261
The "short-name resolution" error makes no sense to me based on what I can find about it, since the container URL includes `docker.io` (`docker.io/amd64/ubuntu:22.04` in the above example).
> you can't install podman-docker without removing docker
Ok, let's not go that route, since Docker currently works.
...
👍 dergoegge approved a pull request: "build: Pass sanitize flags to instrument `libsecp256k1` code"
(https://github.com/bitcoin/bitcoin/pull/28875#pullrequestreview-1844249233)
reACK cbea49c0d32badb975fbf22d44f8e25cc7972af7
(https://github.com/bitcoin/bitcoin/pull/28875#pullrequestreview-1844249233)
reACK cbea49c0d32badb975fbf22d44f8e25cc7972af7
💬 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).
(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
(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
(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
(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
...
(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.
(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
...
(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.
(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.
(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
...
(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
...
(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
...
(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?
(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
(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