💬 fanquake commented on pull request "QA: Expect PACKAGE_NAME rather than constant "Bitcoin Core"":
(https://github.com/bitcoin/bitcoin/pull/30308#issuecomment-2180175255)
> previous releaseas are hardcoded to be Bitcoin Core, so need to change that

It can be changed just for consistency, and to prevent similar issues in future when devs copy-paste code.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1647239779)
That first command:

```
WARNING: image platform (linux/arm64/v8) does not match the expected platform (linux/amd64)
{"msg":"exec container process `/usr/bin/uname`: Exec format error","level":"error","time":"2024-06-20T09:03:31.220776Z"}
```

Running the second command does not change anything.

💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1647247303)
What is the output of the second command?

Locally, everything works for me:

```
lscpu | grep AMD && cat /etc/os-release | grep Noble && podman --version
Vendor ID: AuthenticAMD
Model name: AMD EPYC Processor
VERSION="24.04 LTS (Noble Numbat)"
podman version 4.9.3

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1647249068)
```
Vendor ID: AuthenticAMD
Model name: AMD Ryzen 9 7950X 16-Core Processor
Virtualization: AMD-V
VERSION="24.04 LTS (Noble Numbat)"
podman version 4.9.3
```

💬 maflcko commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1647250884)
What is the output of `podman run --rm --privileged docker.io/multiarch/qemu-user-static --reset -p yes`?

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1647252124)
```
podman run --rm --privileged docker.io/multiarch/qemu-user-static --reset -p yes
Setting /usr/bin/qemu-alpha-static as binfmt interpreter for alpha
Setting /usr/bin/qemu-arm-static as binfmt interpreter for arm
Setting /usr/bin/qemu-armeb-static as binfmt interpreter for armeb
Setting /usr/bin/qemu-sparc-static as binfmt interpreter for sparc
Setting /usr/bin/qemu-sparc32plus-static as binfmt interpreter for sparc32plus
Setting /usr/bin/qemu-sparc64-static as binfmt interpreter for sp
...

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1647267693)
When I run both commands as root, it does work. But doing so does not change anything for the non-root user. At least that's a clue.

💬 ajtowns commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1647268489)
Not sure if you understood what I meant, apologies if you did already: I mean if I'm looking at the estimatesmartfee help, trying to figure out if I want to change from the (new) default of "economical", the existing explanation of what "conservative" does seems more helpful than an explanation of what the default option does. So just changing the default without changing the rest of the help text seems slightly better, doesn't it?

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1647279777)
Some background information on what this actually does (though not podman specific): https://stackoverflow.com/a/72890225

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1647316744)
Adding `cirrus1` to `/etc/sudoers` and running both commands with `sudo` works. But running only the `--reset` command with `sudo` is not enough.

Not really a solution, but at least I can run the CI job once to see what the performance is like.

💬 vasild commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1647331068)
I think I see your point, you prefer 1. over 2., assuming we got all the bytes we asked for from a read, then:

1. Ignore possible IO error by not checking for it
2. Check for an error and behave as if the read failed

I dropped this change.

💬 ismaelsadeeq commented on pull request "Fee Estimation: change `estimatesmartfee` default mode to `economical`":
(https://github.com/bitcoin/bitcoin/pull/30275#discussion_r1647332466)
I now understand thanks for clarifying.
yes it does it will even be better to document the short description of both modes and indicate `economical` is the default ?

💬 vasild commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1647333555)
> if (m_was_written) Assume(IsNull());

Done.

💬 vasild commented on pull request "util: check for errors after close and read in AutoFile":
(https://github.com/bitcoin/bitcoin/pull/29307#discussion_r1647335718)
Resolving this discussion. Changed the semantic of `AutoFile` to require all write users to call `fclose()`.

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1647348857)
Uhhh ... so I did a reboot. And now `podman run --rm -ti "docker.io/arm64v8/debian:bookworm" uname --machine` just works, doesn't need the reset command.

👍 vasild approved a pull request: "init: fixes file descriptor accounting"
(https://github.com/bitcoin/bitcoin/pull/30065#pullrequestreview-2130057647)
ACK fe76929d4d68a4ca6d27c58c1e243a64fdc70b2a

💬 m3dwards commented on pull request "ci: add option for running tests without volume":
(https://github.com/bitcoin/bitcoin/pull/30310#discussion_r1647363601)
The script makes sure the directories exist to mount into the container:

`mkdir -p "${DEPENDS_DIR}/built"`

This directory has to be in a location that GHA allows you to create a directory such as ${RUNNER_TEMP}. The default specified in `00_setup_env.sh` would be `/ci_container_base/depends`.

So even though this specific job doesn't use depends this was done to make sure the container could mount all of the directories.

I will look at solving this in a less confusing way, perhaps jus
...

💬 Sjors commented on pull request "Support self-hosted Cirrus workers on forks":
(https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1647365952)
If the job finishes in reasonable time, I'll add something like this to the documentation: https://github.com/Sjors/bitcoin/commit/d065dc11fb862f48fc88e674a95b7daf68269ca0

It seems that this either works trivially or is really hard to debug, so I prefer to keep `NO_ARM=1` around as an alternative.

💬 dergoegge commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1647358794)
Is clearing the filters is necessary in IBD?

💬 dergoegge commented on pull request "locks: introduce mutex for tx download, flush rejection filters on UpdatedBlockTip":
(https://github.com/bitcoin/bitcoin/pull/30111#discussion_r1645820087)
Why is UpdatedBlockTipSync called but not UpdatedBlockTip?