🤔 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

👍 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?