💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466637332)
podman-docker is not documented in the manual installation... https://podman.io/docs/installation#building-from-source
Let's see how I can install this without totally messing up my OS :-)
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466637332)
podman-docker is not documented in the manual installation... https://podman.io/docs/installation#building-from-source
Let's see how I can install this without totally messing up my OS :-)
💬 achow101 commented on pull request "wallet: guard against dangling to-be-reverted db transactions":
(https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1910567093)
> seems like something I would never expect to happen unless someone modified the code or manually corrupted the data on disk. So I'm wondering did we accidentally cause a breakage like that during development of these PRs?
It shouldn't ever happen.
In general, we don't have very robust error handling for when the database fails to read or write something. I think this should actually be more like an assert, but one that doesn't kill the whole software, maybe just unloads the wallet?
(https://github.com/bitcoin/bitcoin/pull/29253#issuecomment-1910567093)
> seems like something I would never expect to happen unless someone modified the code or manually corrupted the data on disk. So I'm wondering did we accidentally cause a breakage like that during development of these PRs?
It shouldn't ever happen.
In general, we don't have very robust error handling for when the database fails to read or write something. I think this should actually be more like an assert, but one that doesn't kill the whole software, maybe just unloads the wallet?
💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466643431)
You can try:
```sh
mkdir -p ~/bin/ && echo 'IyEvdXNyL2Jpbi9lbnYgYmFzaAoKZWNob2VycigpIHsgZWNobyAiJEAiIDE+JjI7IH0KCmVjaG9lcnIgIiAoOjo6ZG9ja2VyLT4pcG9kbWFuICR7QH0iCgpwb2RtYW4gIiR7QH0iCg==' | base64 --decode > ~/bin/docker && chmod +x ~/bin/docker
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1466643431)
You can try:
```sh
mkdir -p ~/bin/ && echo 'IyEvdXNyL2Jpbi9lbnYgYmFzaAoKZWNob2VycigpIHsgZWNobyAiJEAiIDE+JjI7IH0KCmVjaG9lcnIgIiAoOjo6ZG9ja2VyLT4pcG9kbWFuICR7QH0iCgpwb2RtYW4gIiR7QH0iCg==' | base64 --decode > ~/bin/docker && chmod +x ~/bin/docker
💬 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
...