🚀 fanquake merged a pull request: "ci: Lint follow-ups"
(https://github.com/bitcoin/bitcoin/pull/33776)

💬 stringintech commented on pull request "kernel: Rename in-memory DB option setters and simplify API":
(https://github.com/bitcoin/bitcoin/pull/33877#issuecomment-3541646050)
Good point! I understand your rationale, but wouldn't the cases you're describing be mostly for debugging/testing purposes?
It seems to me that for typical use cases, we would know whether to enable in-memory mode at compile time, not something that is derived dynamically, so it feels more natural/clear to simply call a function that does that with no input parameter.
Also, having an `update(bool_value)` function might indicate that we're supporting some meaningful toggleable functionality, w
...

💬 laanwj commented on pull request "refactor, docs: Embedded ASMap [2/3]: Refactor asmap internals and add documentation":
(https://github.com/bitcoin/bitcoin/pull/33878#issuecomment-3541671065)
> The first three PRs were already part of https://github.com/bitcoin/bitcoin/pull/28792, the others are new.

i'm sure you mean the first three *commits*? 😄

💬 stickies-v commented on pull request "ci: Add IWYU job":
(https://github.com/bitcoin/bitcoin/pull/33810#issuecomment-3541688045)
Concept ACK, but a little more information in the PR description would be helpful, it's hard to parse what expected behaviour is - both from the CI, as well as from developers. Does passing CI become mandatory? Are we duplicating CI runs or moving it to a separate iwyu job? Should developers change anything in their workflow? Linking the previous discussion is helpful, but doesn't necessarily describe what's adopted in this PR.

Also, style preferences wrt includes should be added to `develope
...

🤔 yuvicc reviewed a pull request: "kernel: add btck_block_tree_entry_equals"
(https://github.com/bitcoin/bitcoin/pull/33855#pullrequestreview-3472582923)
Code Review ACK 096924d39d644acc826cbffd39bb34038ecee6cd

💬 stickies-v commented on pull request "kernel: Rename in-memory DB option setters and simplify API":
(https://github.com/bitcoin/bitcoin/pull/33877#issuecomment-3541746178)
> wouldn't the cases you're describing be mostly for debugging/testing purposes ... we would know whether to enable in-memory mode at compile time

I don't know? I wouldn't find it unreasonable to use kernel for ephemeral in-memory operations.

> Also, having an update(bool_value) function might indicate that we're supporting some meaningful toggleable functionality, while in reality once we construct the chainman, changing these options has no effect.

The same issue exist with either app
...

💬 hodlinator commented on pull request "qa: Account for unset errno in ConnectionResetError":
(https://github.com/bitcoin/bitcoin/pull/33875#discussion_r2534011313)
Will try to come up with something in case I re-touch.
```python
errno.ECONNRESET, # This might happen when the RPC server is in warmup, or throws an exception and disconnects us
```

💬 hodlinator commented on pull request "kernel, validation: Refactor ProcessNewBlock(Headers) to return BlockValidationState":
(https://github.com/bitcoin/bitcoin/pull/33856#discussion_r2534055408)
But if `state` contained anything interesting, wouldn't we already have returned it before reaching this block?
Callers of the function only see either `bg_state` or `state` as this PR stands anyway.

Thanks for taking my other suggestions!

👍 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