💬 Eunovo commented on pull request "Ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r1910419783)
> The test reveals that it does load the invalid block and it will connect it if script checks are skipped, see [Eunovo@1ff4e5e](https://github.com/Eunovo/bitcoin/commit/1ff4e5ee78418a30ba1ea9c384b8c7ee43435ce6)

I'm still not sure if this causes any damage because I expect the node to eventually pick the most-work chain when it syncs headers

💬 l0rinc commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1910422237)
Wouldn't `LogWarning` replace `BCLog::BLOCKSTORAGE` with `BCLog::LogFlags::ALL`?

💬 l0rinc commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1910424074)
Yeah, I prefer changing it minimally when moving, to make sure I don't hide any changes there.

📝 brunoerg opened a pull request: "test: add coverage for unknown address type for `createwalletdescriptor`"
(https://github.com/bitcoin/bitcoin/pull/31635)
Calling `createwalletdescriptor` RPC with an unknown address type throws an error. This PR adds test coverage for it as done for other RPCs (`getnewaddress `, `getrawchangeaddress`, etc).

👍 rkrux approved a pull request: "test: raise an error in `_bulk_tx_` when `target_vsize` is too low"
(https://github.com/bitcoin/bitcoin/pull/30322#pullrequestreview-2542565892)
reACK 92787dd52cd4ddc378cf1bcb7e92a649916a0f42

Nit: The PR title can be updated to account for output value check as well.

Simulated the errors for the 2 checks in `feature_framework_miniwallet.py` by passing too high fees and too low target_vsize:

```
RuntimeError: UTXO value 50.00000000 is too small to cover fees 100.00000001
RuntimeError: target_vsize 10 is less than transaction virtual size 104
```

💬 hodlinator commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1910456541)
Correct, by "non-categorized" I was referring to `BCLog::LogFlags::ALL`.

💬 fanquake commented on pull request "test: add coverage for unknown address type for `createwalletdescriptor`":
(https://github.com/bitcoin/bitcoin/pull/31635#issuecomment-2582849166)
https://github.com/bitcoin/bitcoin/actions/runs/12711073565/job/35433597593?pr=31635#step:7:44312:
```bash
test 2025-01-10T14:30:30.975000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
File "/home/runner/work/_temp/test/functional/test_framework/util.py", line 160, in try_rpc
fun(*args, **kwds)
File "/home/runne
...

💬 fanquake commented on issue "cmake inconsistently overriding `-O3` (sometimes)":
(https://github.com/bitcoin/bitcoin/issues/31491#issuecomment-2582866149)
> Instead, I think we only want to do that if we're not overriding the user. Essentially the same as: https://github.com/bitcoin/bitcoin/blob/28.x/configure.ac#L298

So should we also be documenting that our `-DCMAKE_BUILD_TYPE=Release` doesn't give `-O3` (given it's the expected CMake default)? As users explicitly setting that are expecting `-O3`, without having to also provide it via `CMAKE_CXX_FLAGS_RELEASE`, but still wont get it via master or your branch above.

💬 brunoerg commented on pull request "test: add coverage for unknown address type for `createwalletdescriptor`":
(https://github.com/bitcoin/bitcoin/pull/31635#issuecomment-2582866679)
Thanks, @fanquake. Fixed.

💬 Sjors commented on pull request "multiprocess: Add bitcoin wrapper executable":
(https://github.com/bitcoin/bitcoin/pull/31375#issuecomment-2582882222)
I guess `bitcoin` is intentionally not added to `installable_targets` because it makes reference to the multiprocess binaries that aren't shipped?

I made a Windows guix build for 044c1129db06983da598f427dff85513d8480b3a. It got misidentified as `Trojan:Script/Wavatac.B1ml` again. But since the `bitcoin` binary isn't included yet, it can't be because of the `execvp` call.

Just to be sure, I also tried a guix build for master @ 62bd61de110b057cbfd6e31e4d0b727d93119c72 that this PR is based o
...

👍 brunoerg approved a pull request: "descriptor: Add proper Clone function to miniscript::Node"
(https://github.com/bitcoin/bitcoin/pull/30866#pullrequestreview-2542646880)
code review ACK 352391c2cf1a45231ae92ca92d2415b3786ab9ad

💬 stickies-v commented on pull request "httpserver, rest: improving URI validation":
(https://github.com/bitcoin/bitcoin/pull/27253#discussion_r1910489320)
Everything in your observation looks correct to me, yeah. It's fine in master but not in this PR.

📝 crStiv opened a pull request: "fix: grammatical errors"
(https://github.com/bitcoin/bitcoin/pull/31636)
<!--
*** 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 issue "Stratum v2 via IPC Mining Interface tracking issue":
(https://github.com/bitcoin/bitcoin/issues/31098#issuecomment-2582939898)
I moved Windows support to "can wait for later releases" and expanded the item a bit.

👍 hebasto approved a pull request: "depends: add *FLAGS to gen_id"
(https://github.com/bitcoin/bitcoin/pull/31125#pullrequestreview-2542706349)
ACK 01df180bfb82c7eafac4638ced249bee4409784b.

It might be better to place this [comment](https://github.com/bitcoin/bitcoin/pull/31125/files#r1878548250) in a more prominent location, such as the PR description or within a source code comment.

🚀 fanquake merged a pull request: "wallet: Cleanup accidental encryption keys in watchonly wallets"
(https://github.com/bitcoin/bitcoin/pull/28724)

💬 Sjors commented on pull request "Add multiprocess binaries to release build (except Windows)":
(https://github.com/bitcoin/bitcoin/pull/30975#issuecomment-2582967477)
Rebased to see if nothing new breaks. Clarified the PR description to indicate Windows support (probably) won't make it into this PR.

As described in the TODO, before marking this as ready review, there's one UndefinedBehaviorSanitizer left to resolve upstream in libmultiprocess. I might also wait for #31375 and #31161 to land first (added to description).

💬 0xB10C commented on pull request "mining: bugfix: Fix duplicate coinbase tx weight reservation":
(https://github.com/bitcoin/bitcoin/pull/31384#issuecomment-2582976824)
Concept ACK

⚠️ Lin-web-AI opened an issue: "比特币"
(https://github.com/bitcoin/bitcoin/issues/31637)
********@@********

✅ pinheadmz closed an issue: "比特币"
(https://github.com/bitcoin/bitcoin/issues/31637)