💬 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.

💬 maflcko commented on pull request "ci: Turn CentOS task into native one":
(https://github.com/bitcoin/bitcoin/pull/31651#issuecomment-2595094075)
For reference, multiprocess was turned into i686, because the centos task didn't find one issue, see https://github.com/bitcoin/bitcoin/pull/22923

cc @hebasto

💬 hebasto commented on pull request "ci: Turn CentOS task into native one":
(https://github.com/bitcoin/bitcoin/pull/31651#issuecomment-2595113943)
> Turning the task into a native one makes it easier to run it on aarch64 or any other supported architecture.

Mind elaborating "easier to run it on aarch64"?

💬 maflcko commented on pull request "ci: Turn CentOS task into native one":
(https://github.com/bitcoin/bitcoin/pull/31651#issuecomment-2595119052)
> > Turning the task into a native one makes it easier to run it on aarch64 or any other supported architecture.
>
> Mind elaborating "easier to run it on aarch64"?

Thanks, replaced "easier" with "possible", because with current master it is not possible to run the task natively. (Since it is not a native task).

📝 fanquake locked 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
...

💬 Sjors commented on pull request "Implement BIP 370 PSBTv2":
(https://github.com/bitcoin/bitcoin/pull/21283#issuecomment-2595130819)
Aside from the existential question, it would be useful to have some projects to test this PR against: https://bitcoin.stackexchange.com/questions/125384/who-uses-or-wants-to-use-psbtv2-bip370

💬 dergoegge commented on pull request "fuzz: Expand script verification flag testing to segwit v0 and tapscript":
(https://github.com/bitcoin/bitcoin/pull/31460#issuecomment-2595132646)
> We already have a target for it, eval_script. It could be adapted...

Yes that would be an alternative to some extend but I want to keep this test to `VerifyScript` as that function uses the flags as well.

> The same applies to signature_checker and eval_script harnesses?

That is possible but I would consider that out of scope for this PR.

🤔 TheCharlatan reviewed a pull request: "versionbits refactoring"
(https://github.com/bitcoin/bitcoin/pull/29039#pullrequestreview-2552251401)
Reviewed up to the last commit. This looks very nice, I especially like the move of business logic out of the rpc into a more appropriate module. I also think the new place for `m_warning_caches` is much less confusing.

💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1916411893)
In commit dc37009c621f3ab851e4b78e3a797cb868d1a2ed
Doesn't this now return 144 for any non-mainnet chain? Wouldn't it be more correct to return `return m_chainman.GetConsensus().vDeployments[Consensus::DEPLOYMENT_TESTDUMMY].period;`?

💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1916389216)
Typo nit: Extra whitespace.

💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1916365131)
In commit 9bc41f1b48b2e0cc6abf9714e860a29989d7809c
Nit: Could make this a constexpr, and rename to indicate its global nature?

💬 TheCharlatan commented on pull request "versionbits refactoring":
(https://github.com/bitcoin/bitcoin/pull/29039#discussion_r1916585264)
Can you add a docstring here? I think the semantics of the return type should be explained.