💬 Raimo33 commented on pull request "crypto: optimize SipHash `Write()` method with chunked processing":
(https://github.com/bitcoin/bitcoin/pull/33696#issuecomment-3442131396)
> Why are you reopening a [nacked](https://github.com/bitcoin/bitcoin/pull/33325#issuecomment-3436142482) PR?

reopened as WIP. Just need to make the diff simpler. you were the only NACK.

✅ maflcko closed a pull request: "doc: add AGENTS.md"
(https://github.com/bitcoin/bitcoin/pull/33662)

💬 maflcko commented on pull request "doc: add AGENTS.md":
(https://github.com/bitcoin/bitcoin/pull/33662#issuecomment-3442206089)
Closing for now due to controversy. This should be a trivial and harmless change, but apparently it is not, so there is little value in lengthening the discussion further.

Please leave a comment if you want this to be re-opened.

💬 Raimo33 commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#issuecomment-3442252872)
Can't we use an already existing open source library instead of defining our own?

💬 l0rinc commented on pull request "crypto: optimize SipHash `Write()` method with chunked processing":
(https://github.com/bitcoin/bitcoin/pull/33696#issuecomment-3442257827)
Why are you doing that? Why not fix it in the original PR to retain context?
NACK, please use the original PR for this change, seems very weird to open a new PR when you don't like the feedback...

💬 Sjors commented on pull request "doc: add AGENTS.md":
(https://github.com/bitcoin/bitcoin/pull/33662#issuecomment-3442281585)
> Drahtbot is already fairly good at catching them through heuristics.

I only recently learned that we already auto-close many of these, so the ones that reviewers see are just a few leftovers.

Thanks for everyones feedback.

💬 Sjors commented on pull request "OP_CHECKCONTRACTVERIFY":
(https://github.com/bitcoin/bitcoin/pull/32080#issuecomment-3442292002)
@bigspider please leave a note here if and when you open a PR on Inquisition.

💬 Sjors commented on pull request "wallet: add codex32 argument to addhdkey":
(https://github.com/bitcoin/bitcoin/pull/32652#issuecomment-3442306026)
I still haven't gotten around to trying the paper booklet instructions. If and when I do that, I'll probably review this. But no idea when that is.

💬 Sjors commented on pull request "doc: update multisig tutorial to use multipath descriptors":
(https://github.com/bitcoin/bitcoin/pull/33286#issuecomment-3442398106)
utACK de7c3587cd4586bbed94a4ea6eae4a252301daee

💬 maflcko commented on pull request "rpc: refactor: use string_view in Arg/MaybeArg":
(https://github.com/bitcoin/bitcoin/pull/32983#issuecomment-3442406618)
Still looks like a nice change using more named `Arg` and `MaybeArg` helpers, removing the manual `params[idx].isNull` and `params[idx].get_str` legacy handling.

re-ACK b63428ac9ce2c903670409b3e47b9f6730917ae8 🎉

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line
...

💬 l0rinc commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#issuecomment-3442423293)
Code review ACK fa586e86c4aa20c07f37488c13b2b797b35fdbd9

The small focused commits with detailed messages made the review very easy.
I left a few suggestions for cases that could be made more pythonic (i.e. partial bash migration), but I don't mind keeping it as is, seems correct to me.

💬 l0rinc commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459537459)
nit to avoid modification after creation:
```suggestion
cmd_run = [
"docker", "run", "--rm", "--interactive", "--detach", "--tty"
```

💬 l0rinc commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459543707)
Seems correct to me, but to be sure: does this still work correctly now that `CI_CONTAINER_ID` is not exported before (only in `os.environ["CI_CONTAINER_ID"] = container_id`)?

💬 l0rinc commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459601513)
It seems to me we don't usually use this when `text` is set:
```bash
% git grep -r 'text=True' . | wc -l
37
% git grep -r 'encoding="ascii"' . | wc -l
3
```

💬 l0rinc commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459548877)
if I remember correctly, whenever this error occurred, all concurrent CI builds failed so 3 seconds and a single retry may not suffice in this case - but I don't mind trying as such and add some exponential backoff in a future PR if this isn't enough (e.g. 5 runs with `2 ** retry_count` sleep).

Q: do we need to clean up anything after the first try to make sure the second call has more chance of success?

💬 l0rinc commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459614427)
I don't see docker supporting multiple volume creations but maybe we can loop to avoid the repetition:
```python
for suffix in ["ccache", "depends", "depends_sources", "previous_releases"]:
run(["docker", "volume", "create", f"{os.environ['CONTAINER_NAME']}_{suffix}"], check=False)
```

💬 l0rinc commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459662420)
do we need to add `timeout=12345` for a few of these or will the default suffice?

💬 l0rinc commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459557285)
is my understanding correct that previously check was implicit, raising an exception on failure and now the first call is allowed to fail silently in which case we retry once with checks enabled?
So it's basically a
```python
try:
run(cmd_build)
except subprocess.CalledProcessError:
time.sleep(3)
run(cmd_build) # Retry once
```
?

💬 l0rinc commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459710828)
Ah, this is outside of the `if not os.getenv("DANGER_RUN_CI_ON_HOST"):` branch, so checking the CI host seems correct

💬 l0rinc commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459617726)
https://man7.org/linux/man-pages/man1/mkdir.1.html seems to support batch creation so maybe we can reduce the noise a bit:
```python
run([
"mkdir", "-p",
os.environ["CCACHE_DIR"],
f"{os.environ['DEPENDS_DIR']}/built",
f"{os.environ['DEPENDS_DIR']}/sources",
os.environ["PREVIOUS_RELEASES_DIR"],
os.environ["BASE_BUILD_DIR"], # Unset by default, must be defined externally
])
```

But Python does support native folder creation, not sure we should delegate to bash
...