💬 davidgumberg commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#issuecomment-2594180974)
Rebased to address feedback, removed redundant arch specification from image names and switched to using `CI_IMAGE_PLATFORM="--platform os/arch"` that gets passed to `docker build` and `docker run` instead of using the `DOCKER_DEFAULT_PLATFORM` environment variable.

💬 theStack commented on pull request "test: p2p: fix sending of manual INVs in tx download test":
(https://github.com/bitcoin/bitcoin/pull/31658#issuecomment-2594215328)
> Good find! I think that `p2p_orphan_handling.py` has a similar [spot](https://github.com/bitcoin/bitcoin/blob/e7c479495509c068215b73f6df070af2d406ae15/test/functional/p2p_orphan_handling.py#L431).

Ah right, missed that one. If I interpret the `test_orphan_inherit_rejection` sub-test correctly, we want to send the INV with MSG_TX there to "announce again with potentially different witness", so the solution in this case to avoid ignoring the INV would be to add peer3 with wtxidrelay=False (ne
...

👍 tdb3 approved a pull request: "rpc: add gettarget , target getmininginfo field and show next block info"
(https://github.com/bitcoin/bitcoin/pull/31583#pullrequestreview-2554616902)
code review re ACK 712e3ffde5fa4fd26c0f217942b4a289cf37f361

Since last review, the test was updated to use spendable coinbase UTXOs.

📝 saikiran57 opened a pull request: "Added rescan option for import descriptors"
(https://github.com/bitcoin/bitcoin/pull/31667)
<!--
*** Please remove the following help text before submitting: ***

Pull requests without a rationale and clear improvement may be closed
immediately.

GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->

<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:

* Any test improvements or new tests that improv
...

💬 saikiran57 commented on pull request "Added rescan option for import descriptors":
(https://github.com/bitcoin/bitcoin/pull/31667#issuecomment-2594758942)
I've fixed issue regarding https://github.com/bitcoin/bitcoin/issues/31263

now we can pass rescan option as a request params

✅ saikiran57 closed a pull request: "Added rescan option for import descriptors"
(https://github.com/bitcoin/bitcoin/pull/31667)

📝 saikiran57 opened a pull request: "Added rescan option for import descriptors"
(https://github.com/bitcoin/bitcoin/pull/31668)
Fix related to issue: https://github.com/bitcoin/bitcoin/issues/31263

💬 hebasto commented on pull request "build: Make config warnings fatal if -DWERROR=ON":
(https://github.com/bitcoin/bitcoin/pull/31665#issuecomment-2594865842)
Related to https://github.com/bitcoin/bitcoin/issues/31476.

💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2594870730)
Rebased and trying https://github.com/chaincodelabs/libmultiprocess/pull/127.

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1918054673)
I don't know. @maflcko?

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1918061002)
> This seems ok, but I think I don't understand why it is useful to test that coins are spendable.

It's basically checking against a bug I introduced in an earlier version. Initially I used taproot addresses for the coinbase which are unspendable due to the `unexpected-witness".

However it might be simpler if I drop it and just leave a separate commit as a comment in the PR, if someone needs it later.

💬 maflcko commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#discussion_r1918103981)
It is a dummy value, so any value that passes can be used.

💬 Sjors commented on pull request "rpc: add gettarget , target getmininginfo field and show next block info":
(https://github.com/bitcoin/bitcoin/pull/31583#issuecomment-2594990105)
I dropped the test code that proves the coinbase outputs are spendable. For posterity, it's in https://github.com/Sjors/bitcoin/commit/ac516671b1846dbc40bea23c59399dbd7ec9b76b.

Added `test/functional/data/README.md` to explain how `mainnet_alt.json` was generated.

📝 hebasto opened a pull request: "cmake: Introduce `WITH_PYTHON` build option"
(https://github.com/bitcoin/bitcoin/pull/31669)
This was suggested in https://github.com/bitcoin/bitcoin/issues/31476#issuecomment-2541288435:
> An alternative would be to require python to be disabled explicitly. Otherwise, it seems odd that every setting in cmake has a static default that can only be overridden explicitly, except for some, which are silently downgraded?

This PR updates the build system to fail by default on systems where the minimum required Python version is unavailable. It introduces an option that allows users to exp
...

💬 maflcko commented on pull request "test: p2p: fix sending of manual INVs in tx download test":
(https://github.com/bitcoin/bitcoin/pull/31658#issuecomment-2595010566)
> Hm, that speeds up the test and can be done, but doesn't prevent inv-tx-type/wtxidrelay mismatch and unintended INV message ignoring in the future (that's what I meant with "issues like this")?

mocktime should allow to check for the absence of the tx for the timeout period. An alternative would be to use `ensure_for` with a duration less than the timeout, if you want to keep the wall clock.

My understanding is that a change like this would make the test fail on master, and pass with the
...

🤔 maflcko reviewed a pull request: "ci: Supply `--platform` argument to `docker` commands."
(https://github.com/bitcoin/bitcoin/pull/31657#pullrequestreview-2555387357)
lgtm, but you'll have to undo the last force-push

💬 maflcko commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1918144490)
The last push wasn't correct. This will fail https://cirrus-ci.com/task/4983203259219968?logs=ci#L194:

```
[19:46:15.608] + docker run '' --rm docker.io/ubuntu:24.04 bash -c 'env | grep --extended-regexp '\''^(HOME|PATH|USER)='\'''
[19:46:15.608] + tee --append /tmp/env-ci_worker_1731482059_419572074-ci_native_fuzz
[19:46:15.610] Emulate Docker CLI using podman. Create /etc/containers/nodocker to quiet msg.
[19:46:15.688] Error: repository name must have at least one component
```

I g
...

💬 maflcko commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#discussion_r1918150761)
style wise I don't like using unbounded vars as fallbacks for an empty string. Might as well just set the empty string explicitly?

Ref:

```bash
sh-5.1# set -o nounset
sh-5.1# export CI_IMAGE_PLATFORM=${CI_IMAGE_PLATFORM}
sh: CI_IMAGE_PLATFORM: unbound variable
sh-5.1#

💬 maflcko commented on pull request "ci: Supply `--platform` argument to `docker` commands.":
(https://github.com/bitcoin/bitcoin/pull/31657#issuecomment-2595054694)
(In the pull description you can also remove the struck out stuff, so that it does not end up in the merge commit)

💬 fanquake commented on pull request "cmake: Introduce `WITH_PYTHON` build option":
(https://github.com/bitcoin/bitcoin/pull/31669#issuecomment-2595087377)
Not sure. Why does this need another build option? Given that this is just for working around silent CMake warnings in the CI, couldn't this just be fixed by using explicit `*_TEST` options that otherwise fail if Python is missing?

It also doesn't solve #31476 generically, because every time a similar case comes up, we'll have to add even more code and/or options.