💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750805360)
> I'm not sure I see benefit of changing this from unsigned char to uint8_t
`uint8_t` was already used in the other branches
> changing uint8_t generally to value_type
I like that, thanks!
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750805360)
> I'm not sure I see benefit of changing this from unsigned char to uint8_t
`uint8_t` was already used in the other branches
> changing uint8_t generally to value_type
I like that, 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_r1750805634)
Thanks
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750805634)
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_r1750806256)
Dropped these after your simplification suggestion
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750806256)
Dropped these after your simplification suggestion
💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750811705)
Based on what I read C++23 solves some of these problems (e.g. `Every specialization of std::span is a TriviallyCopyable type. (since C++23)`) and while array and vector casts would likely be safe, thanks for pointing out that the layout may not always be the same and we need something more predictable here.
Wanted to avoid explicit pointer arithmetic or extra copying, let me know if the latest push is a good compromise in this area.
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750811705)
Based on what I read C++23 solves some of these problems (e.g. `Every specialization of std::span is a TriviallyCopyable type. (since C++23)`) and while array and vector casts would likely be safe, thanks for pointing out that the layout may not always be the same and we need something more predictable here.
Wanted to avoid explicit pointer arithmetic or extra copying, let me know if the latest push is a good compromise in this area.
💬 l0rinc commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750812828)
Because we were casting back and forth this way. But I think I found a good enough generalization with your help, we don't need to connect the two operators this way.
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1750812828)
Because we were casting back and forth this way. But I think I found a good enough generalization with your help, we don't need to connect the two operators this way.
💬 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.
(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
...
(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.
(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)
(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.
(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<<`).
(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.
(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`?
(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`?
(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.
(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!
(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.
(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
(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
...
(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?
(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?