💬 Sjors commented on pull request "Drop testnet3":
(https://github.com/bitcoin/bitcoin/pull/31974#issuecomment-3442437485)
Trivial rebase after #32579 (touching `chainparams.cpp`).

💬 Sjors commented on pull request "ci: Doc ASLR workaround for sanitizer tasks":
(https://github.com/bitcoin/bitcoin/pull/33674#discussion_r2459746743)
What does `-w` do?

💬 Sjors commented on pull request "ci: Doc ASLR workaround for sanitizer tasks":
(https://github.com/bitcoin/bitcoin/pull/33674#discussion_r2459749250)
```
-w, --write
Force all arguments to be write arguments and print an error if they cannot be parsed this way.
```

💬 Sjors commented on pull request "ci: Doc ASLR workaround for sanitizer tasks":
(https://github.com/bitcoin/bitcoin/pull/33674#issuecomment-3442450144)
Post merge ACK

💬 Sjors commented on issue "TSAN/MSAN fails with vm.mmap_rnd_bits=32 even with llvm 18.1.3":
(https://github.com/bitcoin/bitcoin/issues/30674#issuecomment-3442459167)
FWIW it's not really fixed, #33674 just a workaround instruction. But with the move to Cirrus runners (and its Github CI fallback) in #32989 I'm no longer running self-hosted CI on my fork(s), so I no longer care about the issue.

💬 Sjors commented on pull request "[POC] wallet: Add Support for BIP-353 DNS-Based Bitcoin Address via External Resolver":
(https://github.com/bitcoin/bitcoin/pull/33069#issuecomment-3442484138)
To recap what I think the main arguments are for closing this for now:

Until we have Silent Payment (sending) support, fetching addresses via DNS is an anti-feature because it encourages address reuse. When we do, it still leaves the dilemma that we don't want to rely on external(ly generated) binary nor are we excited about adding RSA and such to our own codebase. But we could revisit that then.

💬 Sjors commented on pull request "wallet: warn against accidental unsafe older() import":
(https://github.com/bitcoin/bitcoin/pull/33135#issuecomment-3442500399)
As I mentioned in the PR description, there are applications - including the lightning protocol - that use these extra bits. I don't know if they use descriptors internally to represent them, but if they do, then banning these values would mean they can't import these descriptors.

There's also nothing in the BIP that bans (or even discourages or warns about) these values, so we wouldn't be compatible with other wallets.

💬 maflcko commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459831105)
I want to keep if move-only for now.

💬 maflcko commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459831283)
thx, done

💬 maflcko commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459831454)
thx, done

💬 maflcko commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459831637)
I actually don't know if it is needed. The existing code and `test/lint/lint-python-utf8-encoding.py` give different vibes. Not sure what to do, but I can remove it in a follow-up or the next push.

💬 Sjors commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-3442543288)
TL&DR on why this was closed?

💬 maflcko commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459847582)
I guess it can be dropped after https://peps.python.org/pep-0686/

💬 maflcko commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459858429)
Yes, but `run` does not throw. You can find the function above and it behaves roughly equal to `set -o errexit` in Bash (exit immediately on failure).

💬 maflcko commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459859221)
> if I remember correctly, whenever this error occurred, all concurrent CI builds failed

In the linked issue in OP only one task failed

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

If we had to clean up anything, it would be an upstream bug in Docker or Podman, which makes them less reproducible.

💬 maflcko commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459859370)
Yes, it should be correct. My understanding is that `export CI_CONTAINER_ID` is roughly equal to `os.environ["CI_CONTAINER_ID"] = container_id` in Python.

You can also see in the CI logs that it works. https://github.com/bitcoin/bitcoin/actions/runs/18776395325/job/53571955105?pr=33677#step:9:247

💬 maflcko commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#discussion_r2459859484)
I don't think this works with auto-formatters

💬 Sjors commented on pull request "Extend signetchallenge to set target block spacing":
(https://github.com/bitcoin/bitcoin/pull/29365#issuecomment-3442583924)
Concept NACK

I like the idea of custom target block spacing, but I don't like the complexity of a wrapped signet challenge.

Why not just add `-signetblockinterval`? Anyone running a custom signet would just have to copy both the challenge and the custom interval.

💬 l0rinc commented on pull request "ci: Retry image building once on failure":
(https://github.com/bitcoin/bitcoin/pull/33677#issuecomment-3442591502)
Code review reACK 5555bce994b648f836c35a02570f22ae9ad36da3
(only suggested nits applied since last review)

💬 Sjors commented on pull request "doc: external-signer, example 'createwallet' RPC call requires eight argument":
(https://github.com/bitcoin/bitcoin/pull/30354#issuecomment-3442597467)
Instead of `bitcoin-cli -named` let's recommend `bitcoin rpc` (introduced in #31375 and shipped as of v30).