👍 stickies-v approved a pull request: "test: Remove tests violating hardened std::span"
(https://github.com/bitcoin/bitcoin/pull/33886#pullrequestreview-3472700020)
ACK fadb4f63cb0f0b544bc95e48cb42c7636c1dec15

It's unfortunate that we can't test the nullptr + non-zero length paths through the c++ wrapper, but removing these cases seems like the best approach for now.

💬 Sjors commented on pull request "mining: add getCoinbase()":
(https://github.com/bitcoin/bitcoin/pull/33819#issuecomment-3541868920)
Fixed MSan uninitialised `coinbase` complaint, which should also fix the `coinbase.value_remaining == 0' failure.

I also switched `coinbase.version` from using `CURRENT_VERSION` to `version`.

Updated some comments above to make it clear that this PR does deprecate `getCoinbaseTx()` and `getWitnessCommitmentIndex()` and an additional remark about `getCoinbaseCommitment()`.

👋 Sjors's pull request is ready for review: "mining: add getCoinbase()"
(https://github.com/bitcoin/bitcoin/pull/33819)

💬 maflcko commented on issue "guix build fails on RISC-V due to python-py-cpuinfo test failure":
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3541894978)
I presume the same fails upstream, without guix: https://github.com/workhorsy/py-cpuinfo/blame/f3f0fec58335b9699b9b294267c15f516045b1fe/tests/test_actual.py#L74

Though, upsteam is unmaintained, so i am not sure what can be done here.

💬 stringintech commented on pull request "kernel: Rename in-memory DB option setters and simplify API":
(https://github.com/bitcoin/bitcoin/pull/33877#issuecomment-3541912895)
> I don't know? I wouldn't find it unreasonable to use kernel for ephemeral in-memory operations.

Of course. I meant that maybe the case where we would "dynamically" decide to use in-memory mode wouldn't be that common.

I found the approach in this PR to be a slight improvement for the common use cases, making it a bit more straightforward. But I understand if you find the dynamic use cases more common than I do. Thanks for the feedback!

💬 maflcko commented on pull request "init: completely remove `-maxorphantx` option":
(https://github.com/bitcoin/bitcoin/pull/33872#issuecomment-3541919560)
lgtm ACK 0aebdac95da9a7d476264424c0107bd806ce5362

💬 ajtowns commented on issue "Standardness policy rules for legacy Multisig script is incoherent":
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3541949709)
> standardness requires solvable MULTISIG tx outputs to be at most _m_-of-3 and _m_ to be less than _n_.

The requirement is for `m` not to be greater than `n`, as far as I can see? (via both `IsStandard()` and `Solver()`)

> Solvability (and hence standardness) for script is tightened so that for script standardness purposes pubkey data must be between 33 and 65 bytes rather than 33 and 120 bytes.
>
> ::TODO:: see if any such UTXOs exist.

I believe there are four utxos that match this for bar
...

🚀 fanquake merged a pull request: "test: Remove tests violating hardened std::span"
(https://github.com/bitcoin/bitcoin/pull/33886)

📝 hodlinator opened a pull request: "doc: Improve CI docs on env and qemu-user-static"
(https://github.com/bitcoin/bitcoin/pull/33887)
Should at least partially fix #31199

💬 hodlinator commented on issue "CI: Improve documentation around replicating CI locally":
(https://github.com/bitcoin/bitcoin/issues/31199#issuecomment-3542031492)
PR is live: #33887

💬 waketraindev commented on pull request "test, refactor: Embedded ASMap [1/3]: Selected minor preparatory work":
(https://github.com/bitcoin/bitcoin/pull/33026#issuecomment-3542059320)
I think the `AutoFile::size` implementation and the subsequent refactoring would fit better in their own dedicated refactoring PR, or in a PR where they're actually required. They don't seem to be needed by either of the other two commits (the `LogWarning` change in `asmap.cpp` or the newly added test vectors in `netbase_tests.cpp`).

I understand this is listed as "selected minor preparatory work", and I realize this is nitpicking a bit, but it would still be cleaner to introduce these change
...

💬 marcofleon commented on pull request "fuzz: wallet: add target for `MigrateToDescriptor`":
(https://github.com/bitcoin/bitcoin/pull/32624#issuecomment-3542072403)
Nice, lgtm ACK 51917d94e657024cc0af8aa748a9d5d39a617e85

👍 maflcko approved a pull request: "doc: Improve CI docs on env and qemu-user-static"
(https://github.com/bitcoin/bitcoin/pull/33887#pullrequestreview-3472936753)
lgtm ACK a8afb65ce9b8cd5fe90fcc570087f175c3662cad

nit: Not sure if the commit message is fully correct

💬 maflcko commented on pull request "doc: Improve CI docs on env and qemu-user-static":
(https://github.com/bitcoin/bitcoin/pull/33887#discussion_r2534252107)
For reference, this was half-fixed in commit fd813bf863b1ffa91429de6342285b35bab2bfa4 for env vars that have nothing to do with the CI (like `DEBUGINFOD_URLS`). However, it is still needed for env vars that have a meaning outside and inside CI (like `HOST`)

However, your `HOST` is not set, so the comment "Should help in cases such as: https://github.com/bitcoin/bitcoin/pull/31176#issuecomment-2526410039" may not be accurate here.

💬 laanwj commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#discussion_r2534280150)
Isn't this `ip_bytes` now? (though bits is technically still valid ofc)

💬 maflcko commented on issue "CI: Improve documentation around replicating CI locally":
(https://github.com/bitcoin/bitcoin/issues/31199#issuecomment-3542114544)
> Another related idea was to move the CC/CXX/SANITZER/... build options into a cmake preset, which could make it easier to reproduce some CI failures. Ref: https://bitcoin-irc.chaincode.com/bitcoin-core-dev/2024-09-05#1050658 and some more discussion in [#30871 (comment)](https://github.com/bitcoin/bitcoin/pull/30871#issuecomment-2344047118)

this was moved to dev-mode in https://github.com/bitcoin/bitcoin/pull/33824, so maybe this is already a step in the right direction?

💬 roconnor-blockstream commented on issue "Standardness policy rules for legacy Multisig script is incoherent":
(https://github.com/bitcoin/bitcoin/issues/33882#issuecomment-3542129055)
> The requirement is for m not to be greater than n, as far as I can see? (via both IsStandard() and Solver())

Yeah, I meant to say "less than or equal to". However, AFAICT this inequality is only enforced in standardness, not solvability.

💬 laanwj commented on issue "guix build fails on RISC-V due to python-py-cpuinfo test failure":
(https://github.com/bitcoin/bitcoin/issues/33873#issuecomment-3542166449)
Right! i could take a look at fixing this upstream.

From a high level perspective, i don't quite understand why a cpuinfo module would be needed for deterministic cross-compiling in the first place. Nor why it'd run tests that depend on local CPU configuration. But i guess it's some deeper nested dependency that gets pulled in unconditionally (through LIEF in this case?). GUIX sadly has more of those, basically same complaint as in #30042.

💬 fanquake commented on pull request "ci: Enable experimental kernel stuff in most CI tasks via `dev-mode`":
(https://github.com/bitcoin/bitcoin/pull/33824#discussion_r2534330486)
Any idea why `BUILD_GUI_TESTS=OFF` was previously set? Looks like it predates CMake, and was just ported; I guess no-longer needed?

💬 maflcko commented on issue "upstream: GUIX closure contains too much unnecessary stuff":
(https://github.com/bitcoin/bitcoin/issues/30042#issuecomment-3542213579)
The linked issue is now closed with the last comment:

> With commit 9fa92acbf0c4dbc734ac7d83b31bd6d12e09a401 (https://codeberg.org/guix/guix/commit/9fa92acbf0c4dbc734ac7d83b31bd6d12e09a401) this is mostly
fixed. There’s still another path leading to libx11 though:

```
$ ./pre-inst-env guix graph --path guix libx11
guix@1.4.0-6.dc5430c
guile-avahi@0.4.1
avahi@0.8
dbus@1.14.0
libx11@1.8.1

(The same applies to “guix pull”.)

Not sure what can be done about it.

Ludo’.