💬 glozow commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466648992)
Do you mean all of the sibling eviction logic? That doesn't work because we'd only enforce that the tx pays enough to evict siblings OR conflicting transactions; it needs to pay for both.
If you mean the logic adding stuff to `m_conflicts`, `m_iters_conflicting`, etc., it's because I don't like in-out parameters.
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466648992)
Do you mean all of the sibling eviction logic? That doesn't work because we'd only enforce that the tx pays enough to evict siblings OR conflicting transactions; it needs to pay for both.
If you mean the logic adding stuff to `m_conflicts`, `m_iters_conflicting`, etc., it's because I don't like in-out parameters.
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466651006)
That base64 decodes to:
```sh
#!/usr/bin/env bash
echoerr() { echo "$@" 1>&2; }
echoerr " (:::docker->)podman ${@}"
podman "${@}"
```
That doesn't seem any different from my `DOCKER_RUN="podman run --replace"` alias.
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466651006)
That base64 decodes to:
```sh
#!/usr/bin/env bash
echoerr() { echo "$@" 1>&2; }
echoerr " (:::docker->)podman ${@}"
podman "${@}"
```
That doesn't seem any different from my `DOCKER_RUN="podman run --replace"` alias.
💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466652244)
Or I guess it's the `echoerr` doing some magic
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466652244)
Or I guess it's the `echoerr` doing some magic
💬 instagibbs commented on pull request "policy: enable sibling eviction for v3 transactions":
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466657588)
the later, but at least you gave a reason why not
(https://github.com/bitcoin/bitcoin/pull/29306#discussion_r1466657588)
the later, but at least you gave a reason why not
💬 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.