💬 kanzure commented on pull request "doc: add AGENTS.md":
(https://github.com/bitcoin/bitcoin/pull/33662#issuecomment-3426538091)
One other concern I have is that this is essentially a change that is meant
to be subversive to a developer's tools or development environment. In
part, it is justified as a defense against the waste of other project
contributor's time. I see the logic there. Indeed this is not far from a
recommending and providing a default linter config, or a default commit
message format recommendation. However there should be some thought put
into the subject of where the limit is with regards to other
possi
...

💬 maflcko commented on pull request "doc: add AGENTS.md":
(https://github.com/bitcoin/bitcoin/pull/33662#issuecomment-3426568442)
> However there should be some thought put into the subject of where the limit is with regards to other possibly-hostile prompt injection

I'd say using agents to amend the prompt is well-understood and well-docuemented. I think the change here is harmless and mildly useful. Though, if it doesn't work in the future, it can be removed again and it is probably not worth to bend over backwards to achieve undocumented prompt injection.

💬 maflcko commented on pull request "ci: detect outbound internet traffic generated while running tests":
(https://github.com/bitcoin/bitcoin/pull/31349#issuecomment-3426586630)
From the output, I think there may be several unit tests affected, and they should probably be fixed, even if the GHA CI does not catch this issue.

Also, I tried on a fresh Ubuntu VM with a fresh user account (not root) and the issue persists. So I think the issue generally uncovers via podman.

👍 l0rinc approved a pull request: "ci: Only write docker build images to Cirrus cache"
(https://github.com/bitcoin/bitcoin/pull/33639#pullrequestreview-3360383974)
Concept ACK, I understand that you already have an ACK but please consider my suggestions

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448019150)
hmmm, not sure I get why we need this randomness here, is it to allow multiple runs without synchronization?

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448054647)
I assume that changing the `MAYBE_CPUSET` position is irrelevant, right?

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448072260)
nit: we were logging `CI_IMAGE_NAME_TAG` before (just resolve if that's intentional)

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448057503)
for symmetry can we inline + unpack this?

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448120770)
slightly tangential, but maybe there's room for optimizing the docker layers by putting the variable parts as the last layers, using ARGs over ENVs, combining multiple layers, pinning versions and paths and variables early, pruning old layers, multi-stage builds, etc.

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448149644)
I don't yet understand why we're pinning CPUs here, what problem is that solving that `--cpus=` wouldn't?
The comment is a bit misleading since my understanding is that `--cpuset-cpus` doesn't actually limit CPU usage.

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448113232)
Do we still need this? My understanding is that we're not really using this - can we delete and add back when there's a well-defined usecase for it?

If we do, we might want to add `MAKEJOBS` guard as well since that is also needed

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448139543)
Is this related to the mentioned https://www.shellcheck.net/wiki/SC2086? Because we need split to handle quotes and escaping properly?

💬 l0rinc commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448081592)
Can we inline `cmd_build`?
```python
run(["docker", "buildx", "build",
f"--file={os.environ['BASE_READ_ONLY_DIR']}/ci/test_imagefile",
f"--build-arg=CI_IMAGE_NAME_TAG={os.environ['CI_IMAGE_NAME_TAG']}",
f"--build-arg=FILE_ENV={os.environ['FILE_ENV']}",
f"--build-arg=BASE_ROOT_DIR={os.environ['BASE_ROOT_DIR']}",
f"--platform={os.environ['CI_IMAGE_PLATFORM']}",
f"--label={CI_IMAGE_LABEL}",
f"--tag={os.environ['CONTAINER_NAME']}",
*maybe_cpuset(),
*shlex.split(os
...

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448342637)
Ah nice, changed all

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448344025)
thx, took your doc comment

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448344564)
(removed)

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448344952)
Yes, exactly, see also the commit message :)

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448346083)
In theory yes, but in practise it does not work due to a docker bug. Also, I don't think it is worth it in practise, see https://github.com/bitcoin/bitcoin/pull/33620

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448346402)
(removed)

💬 maflcko commented on pull request "ci: Only write docker build images to Cirrus cache":
(https://github.com/bitcoin/bitcoin/pull/33639#discussion_r2448346644)
No, a follow-up requires it to be separate :)