💬 l0rinc commented on pull request "test: add subsidy sum test, iterating every block":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2338961152)
@fjahr, @brunoerg, please reevaluate the concept NACKs. I'm no longer modifying the original test.
The point of the new one is to avoid skipping any blocks and to validate strict monotonicity after reaching 0 — i.e., a more thorough validation than before.

💬 fjahr commented on pull request "test: add subsidy sum test, iterating every block":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2338991268)
> I'm no longer modifying the original test.

You are still modifying the original test and even if you would remove those modifications I still think this is not worth the effort of review. Your new test doesn't add anything of substantial value and I would not be surprised if in a few weeks someone opens a PR to remove the first test because there is already another test that does the same thing which leads to the same result as a rewrite. Then we will have to have another argument about the
...

💬 l0rinc commented on pull request "test: add subsidy sum test, iterating every block":
(https://github.com/bitcoin/bitcoin/pull/30699#issuecomment-2339016088)
> You are still modifying the original test

No, it does the same steps as before.

> I still think this is not worth the effort of review

Your NACK will prevent others from exerting that effort, so what's the real reason for treating this area differently than the rest of the codebase?
I've explained my reasoning for why I think we need to be more thorough, hence my PR.

✅ l0rinc closed a pull request: "refactor: Migrate EmplaceCoinInternalDANGER to try_emplace"
(https://github.com/bitcoin/bitcoin/pull/30637)

🤔 hodlinator reviewed a pull request: "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors"
(https://github.com/bitcoin/bitcoin/pull/30765#pullrequestreview-2290825978)
Concept ACK 4574e45a99d8e11ccadd839f2ef8a80356e8955d.

Sad to see my weird fix in https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1748936794 go /s, but also glad the workaround was so smooth. Hope that case won't show up again or one of us remembers it.

💬 hodlinator commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750849674)
IMO `Append` -> `Push` to stay more consistent with the OP-codes themselves (if we keep this change of breaking out named methods from `operator<<`).

💬 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