💬 laanwj commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1852336507)
Maybe add an assertion on `solutions.size() >= 2`,as an internal check against future bugs in Solve (and maybe also in other places before the solutions vector is indexed).

💬 laanwj commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1853733972)
Doing both inserting into and reading from spks here makes it somewhat harder to be confident that the behavior is independent from the specific order of processing mapScripts, but the comment above helps.

💬 laanwj commented on pull request "wallet: Remove IsMine from migration code":
(https://github.com/bitcoin/bitcoin/pull/30328#discussion_r1853729309)
comment: in "spks"

💬 fanquake commented on pull request "guix: swap `moreutils` for just `sponge`":
(https://github.com/bitcoin/bitcoin/pull/31323#discussion_r1853761691)
Bash is implied/available, so we are able to drop this.

💬 maflcko commented on issue "ci: how to run native arm job on Apple silicon?":
(https://github.com/bitcoin/bitcoin/issues/31344#issuecomment-2493529510)
My recommendation would be to try without the cirrus overhead for debugging. Just copy-pasting the one-line bash command may be easier and faster for troubleshooting. See https://github.com/bitcoin/bitcoin/tree/master/ci#running-a-stage-locally


As for the error itself: It is not possible to run 32-bit arm binaries on a CPU that only supports 64-bit mode.

I don't know how to fix this, but if nothing helps, you'll have to select qemu-arm in UTM. (Is there a reason why you haven't done this
...

💬 hebasto commented on issue "MSVC 17.12.0 internal compiler error ":
(https://github.com/bitcoin/bitcoin/issues/31303#issuecomment-2493534971)
> Sorry about the regression; go ahead and file a report at https://developercommunity.visualstudio.com/cpp/report and we'll see if we can get it serviced quickly.

https://developercommunity.visualstudio.com/t/C1001:-Internal-compiler-error-in-the-Bi/10797559

👍 hebasto approved a pull request: "ci: Skip broken Wine64 tests by default"
(https://github.com/bitcoin/bitcoin/pull/31284#pullrequestreview-2454213024)
ACK fa5e7064597fc51366f082e3d07a4591e576db38, to avoid false-positives.

✅ Sjors closed a pull request: "mining: add early return to waitTipChanged()"
(https://github.com/bitcoin/bitcoin/pull/31297)

💬 Sjors commented on pull request "mining: add early return to waitTipChanged()":
(https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2493553214)
I tested that `m_tip_block_cv.wait_for` indeed immediately checks the condition before waiting. And I could see that `notifications().m_tip_block` isn't set when you start a node that is already fully caught up.

I'll open a new PR to try and deal with that.

👍 laanwj approved a pull request: "contrib: skip missing binaries in gen-manpages"
(https://github.com/bitcoin/bitcoin/pull/30986#pullrequestreview-2454225450)
Code review ACK 4bbd28baf33382231f4f1dab20681c05f9915af2

💬 maflcko commented on pull request "[POC] ci: Test cross-built Windows executables on Windows natively":
(https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2493572333)
I think this is useful, and while it is waiting for more review (and for the existing review to be addressed), it could make sense to schedule it to run regularly in a repo. For example, in one of your repos, or in a new repo, or in an existing one like https://github.com/maflcko/b-c-nightly/ .

⚠️ rkrux opened an issue: "Add `satToBtc()` and conversely `btcToSat()` util functions in functional tests"
(https://github.com/bitcoin/bitcoin/issues/31345)
### Motivation

In [functional tests](https://github.com/bitcoin/bitcoin/tree/master/test/functional), there are numerous instances of conversion code with patterns such as `/ COIN` and `* COIN` that are converting between units _satoshis_ to _BTC_.

Following details are as of the latest commit on master 2638fdb4f934be96b7c798dbac38ea5ab8a6374a.

### Patterns stats
```terminal
# satoshis to BTC conversion
➜ bitcoin git:(2638fdb4f9) ✗ git grep -n "/ COIN" -- '*.py' | wc -l
22


...

💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1853812830)
Quite a late reply here but I don't think I will get to this anytime soon, created an issue here for anyone else to pick up: https://github.com/bitcoin/bitcoin/issues/31345

💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853756499)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853379963

The idea is not to add an ignored test, but to add real test coverage. For example if there is a buggy `add()` function that thinks `2 + 2 = 5`, the ideal way to fix it is to first add a test commit that asserts `add(2, 2) == 5` with a comment noting the add function is buggy. After this, a separate commit should fix the bug and update the test to assert `add(2, 2) == 4`. Advantages of this approach:

1. Clearly shows
...

💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853811005)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853683566

> What's the reason for not wanting an `about` window regardless of the `-version` flag's value? Doesn't `gArgs.IsArgSet("-version")` make more sense here?

The idea is for -noversion and -version=0 to be the same as not specifying a -version argument. -version is just a binary option, it should choose between 2 behaviors, not 3

💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853779968)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853551532

I think the idea here is just for -version to take precedence over -help if both are specified. I don't know if this is the best behavior, but I don't see a reason to change in a commit that is just trying to handle -version=0 and -noversion more sensibly.

💬 ryanofsky commented on pull request "util: Improve documentation and negation of args":
(https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853807905)
re: https://github.com/bitcoin/bitcoin/pull/31212#discussion_r1853700315

> given that checking the args is not for free (triggers locks and merges from multiple sources), maybe we could introduce a `IsArgEnabled` which gets rid of this confusing combination (e.g. where `-noconf=0` would be set and not negated, I think)

#31260 gets away from this error-prone repeated retrieval of settings by forcing the type of every setting to be declared up front, and only providing a single `Get` accesso
...

💬 l0rinc commented on issue "ci: how to run native arm job on Apple silicon?":
(https://github.com/bitcoin/bitcoin/issues/31344#issuecomment-2493612110)
Not sure it helps, but I have tried reproducing it on my M4 Max, emulating an s390x platform (as I've done in https://github.com/bitcoin/bitcoin/pull/31144#issuecomment-2453108430) - while it's still very (very) slow:

<details>
<summary>docker setup</summary>

```bash
brew install podman pigz
softwareupdate --install-rosetta
podman machine init
podman machine start
docker run --platform linux/s390x -it ubuntu:22.04 /bin/bash
```

</details>

```bash
apt update && apt install -y
...

💬 Sjors commented on pull request "Add multiprocess binaries to release build":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2493614105)
Well that's nice, looks like only the ASan + LSan + UBsan job is failing.

📝 Sjors opened a pull request: "Set notifications m_tip_block in LoadChainTip()"
(https://github.com/bitcoin/bitcoin/pull/31346)
Ensure KernelNotifications `m_tip_block` is set even if no new block arrives.

Suggested in https://github.com/bitcoin/bitcoin/pull/31297#issuecomment-2486457573