👍 hodlinator approved a pull request: "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls"
(https://github.com/bitcoin/bitcoin/pull/31490#pullrequestreview-2542174622)
ACK 223081ece651dc616ff63d9ac447eedc5c2a28fa

Thanks for reorganizing the first commits!
Confirmed that `git -c log.showSignature=false log --oneline --follow src/bench/readwriteblock.cpp` shows 7 commits.

Cool with the sanity-check in the scripted diff, not sure I've seen that before.


### Commit message dfb2f9d004860c95fc6f0d4a016a9c038d53a475

Might add some more context:
"Similarly +to `UndoWriteToDisk` in parent commit+, `WriteBlockToDisk` wasn't really extracting"


### Com
...

💬 hodlinator commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1910364421)
Thanks for resolving the terminology @l0rinc and for the back-story @ryanofsky!

💬 hodlinator commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1910222878)
nit: Could at least remove newline, and possibly switch to more modern way of non-filterable non-categorized warning:
```suggestion
LogWarning("Failed to flush undo file %05i", pos.nFile);
```

💬 hodlinator commented on pull request "refactor: inline `UndoWriteToDisk` and `WriteBlockToDisk` to reduce serialization calls":
(https://github.com/bitcoin/bitcoin/pull/31490#discussion_r1910226418)
nit: Could modernize logging when moving the log statement to avoid touching this line twice, resulting in git blame churn. Here and for "OpenBlockFile failed". But I guess you prefer the current commit-style.

💬 Eunovo commented on pull request "Ensure assumevalid is always used during reindex":
(https://github.com/bitcoin/bitcoin/pull/31615#discussion_r1910413909)
I created another test to check if blocks rejected in the normal IBD run get loaded during a `-reindex`. The test reveals that it does load the invalid block and it will connect it if script checks are skipped, see https://github.com/Eunovo/bitcoin/commit/1ff4e5ee78418a30ba1ea9c384b8c7ee43435ce6

IIUC During a reindex, the node does not know what blocks were connected in the previous run so it can't safely skip checks. It looks to me like we have to:
1. Sync headers before connecting blocks d
...

💬 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.