💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2582681441)
A silent conflict has been fixed.

💬 hodlinator commented on pull request "doc: merge required dependency tables":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1910360393)
(systemtap) :+1:

💬 hodlinator commented on pull request "doc: merge required dependency tables":
(https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2582686766)
Would be good to move discussion/question-type commentary out of the PR summary into its own comment as it becomes part of the commit message, regarding the "I was a bit confused ... please let me know ...". Alternatively re-word it.

💬 NicolaLS commented on pull request "doc: merge required dependency tables":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1910364764)
should I add another sub-header (`### Tracing`) in the `## Optional` section and put it there ?

💬 fanquake commented on pull request "doc: merge required dependency tables":
(https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2582707462)
Not sure. Re-arranging this to put two different compilers under "required" doesn't seem less confusing. If anything, I think the tables should be left as-is, and a "developer tools" header added, if there is any confusion.

💬 maflcko commented on pull request "doc: merge required dependency tables":
(https://github.com/bitcoin/bitcoin/pull/31634#discussion_r1910376416)
Yeah, either a new section, or merge the tables below as well into one:

| Dependency | Releases | Version used | Minimum required | Runtime |
| --- | --- | --- | --- | --- |
| [Fontconfig](../depends/packages/fontconfig.mk) (GUI) | [link](https://www.freedesktop.org/wiki/Software/fontconfig/) | [2.12.6](https://github.com/bitcoin/bitcoin/pull/23495) | 2.6 | Yes |
| [FreeType](../depends/packages/freetype.mk) (GUI) | [link](https://freetype.org) | [2.11.0](https://github.com/bitcoin/bitcoin
...

🚀 fanquake merged a pull request: "ci: build msan's libc++ with _LIBCPP_ABI_BOUNDED_*"
(https://github.com/bitcoin/bitcoin/pull/31612)

🚀 fanquake merged a pull request: "doc: Clarify min macOS and Xcode version"
(https://github.com/bitcoin/bitcoin/pull/31608)

💬 NicolaLS commented on pull request "doc: merge required dependency tables":
(https://github.com/bitcoin/bitcoin/pull/31634#issuecomment-2582763764)
> Not sure. Re-arranging this to put two different compilers under "required" doesn't seem less confusing. If anything, I think the tables should be left as-is, and a "developer tools" header added, if there is any confusion.

With a "development tools" header it would still be unclear which development tools are required (`cmake`, `python`), optional (`systemtap`) or mutual substitutes (`gcc` _or_ `clang`) and since required ones would mixed with the two different compilers in the same table
...

👍 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`.