💬 hodlinator commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750865337)
Why `this->` everywhere? Is your plan to change all code you touch to use `this->` for clarity going forward?

Like the `static_cast`s and `cbegin()`/`cend()` though.

💬 hodlinator commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750876143)
nit: Might call it something like `script_byte_array_u8_vector_equivalence`?

💬 hodlinator commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750870251)
nit: Don't like the `_`-prefix of `_data`, but it's from the original. `_`-prefixes otherwise signify something internal or reserved.
Since you are touching all lines, maybe switch it to `data`/`data2`/ `data16` + `data`/`data4`/`data32`?

💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750910623)
It's appending to the end - but if you say `push` is the accepted terminology, I'll rename them, thanks.

💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750910776)
You're right, we don't have an `end` parameter anymore so I'll revert these back to just `end()`, thanks!

💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750910847)
or just `data` since they don't overlap.

💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750910911)
done

💬 fjahr commented on pull request "test: add subsidy sum test, iterating every block":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2339066578)
Since I didn't make it explicit before: Approach NACK to having duplicate tests with minor differences in the codebase. Aside from duplication generally being bad I explained above how this will just introduce new discussion-heavy PRs and might lead to the same outcome as the direct refactoring with an indirection in between. This means if the refactoring is rejected this is not a viable approach either.

> > I still think this is not worth the effort of review
>
> Your NACK will prevent ot
...

💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1750918625)
Not necessary to be done in this PR because there's only one callsite, but I suppose in the follow-up it'll make sense to add a `template <typename... Args> tfm::format(util::ConstevalFormatString<sizeof...(Args)> fmt, const Args&... args)` overload?

💬 stickies-v commented on pull request "util: Use consteval checked format string in FatalErrorf":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1750371020)
typo nit
```suggestion
if (count_nor && count_pos) throw "Format specifiers must be all positional or all non-positional!";
```

🤔 stickies-v reviewed a pull request: "util: Use consteval checked format string in FatalErrorf"
(https://github.com/bitcoin/bitcoin/pull/30546#pullrequestreview-2290090683)
Concept ACK. Doing this at compile time is much more helpful, besides the bugs being fixed.

✅ l0rinc closed a pull request: "test: add subsidy sum test, iterating every block"
(https://github.com/bitcoin/bitcoin/pull/30699)

💬 l0rinc commented on pull request "test: add subsidy sum test, iterating every block":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2339078081)
Ok, thanks for taking the time to explain it.
I don't want others to approve it if you've NACK-ed it. While I don't fully understand the reasoning, I respect your decision, so I'll just close this off instead.

💬 achow101 commented on pull request "multiprocess: Add -ipcbind option to bitcoin-node":
(https://github.com/bitcoin/bitcoin/pull/30509#issuecomment-2339090654)
ACK 30073e6b3a24cbe417c45cd5df6a3a2de0251e9d

🚀 achow101 merged a pull request: "multiprocess: Add -ipcbind option to bitcoin-node"
(https://github.com/bitcoin/bitcoin/pull/30509)

💬 brunoerg commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2339208164)
Concept ACK

👋 tdb3's pull request is ready for review: "rpc: add `revelant_blocks` to `scanblocks status`"
(https://github.com/bitcoin/bitcoin/pull/30713)

💬 tdb3 commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#issuecomment-2339214391)
Will add release notes when incorporating reviewer comments.

💬 tdb3 commented on pull request "rpc: add `revelant_blocks` to `scanblocks status`":
(https://github.com/bitcoin/bitcoin/pull/30713#discussion_r1750990127)
While leaving `scanblocks()` without resetting `g_relevant_blocks` isn't ideal, resetting `g_relevant_blocks` just before `BlockFiltersScanReserver` is used by the `start` branch seemed to be a good way to prevent `status` from accidentally seeing an empty `g_relevant_blocks` (at least in common cases). Please let me know if I'm overlooking a likely concurrency case, and it can be adjusted.

📝 theStack opened a pull request: "build: drop obj/ subdirectory for generated build.h"
(https://github.com/bitcoin/bitcoin/pull/30856)
As indicated by the TODO, the obj subdirectory is not needed anymore now for the generated build.h header, since autotools are gone and we don't have in-source builds anymore.