💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755206069)
> but that's not what this does, that's below this condition.
Ah, yes, mixed them up. Still feels okay to ignore anything up to a '%' that way.
But I do also like l0rinc's approach in this thread https://github.com/bitcoin/bitcoin/pull/30546/files#r1754649705, although extra state tends to create room for more bugs.
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755206069)
> but that's not what this does, that's below this condition.
Ah, yes, mixed them up. Still feels okay to ignore anything up to a '%' that way.
But I do also like l0rinc's approach in this thread https://github.com/bitcoin/bitcoin/pull/30546/files#r1754649705, although extra state tends to create room for more bugs.
💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755352516)
Duplicates
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755352516)
Duplicates
💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755589971)
#### A) Interlacing `tfm::format`
Partially in response to @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755013263)...
> Potentially we could even extend the test to compare tinyformat behavior with ConstevalFormat behavior to make things even clearer, but just having a comment to clarify how these cases are treated by tinyformat would be very helpful.
...and partially out of my own curiosity about how `tfm::format` really behaves - I added `tfm::form
...
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755589971)
#### A) Interlacing `tfm::format`
Partially in response to @ryanofsky's [comment](https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755013263)...
> Potentially we could even extend the test to compare tinyformat behavior with ConstevalFormat behavior to make things even clearer, but just having a comment to clarify how these cases are treated by tinyformat would be very helpful.
...and partially out of my own curiosity about how `tfm::format` really behaves - I added `tfm::form
...
💬 maflcko commented on pull request "http: Use 'starts_with' for matching URI prefix":
(https://github.com/bitcoin/bitcoin/pull/30868#issuecomment-2344697879)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
(https://github.com/bitcoin/bitcoin/pull/30868#issuecomment-2344697879)
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
💬 hodlinator commented on pull request "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors":
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1755611792)
> Pushing has a specific meaning in the context of Bitcoin Script (it's an opcode that pushes data onto the stack), and that's not what's happening here so that would be very confusing.
The new pair of *private* functions together implement `OP_PUSHDATA`, and are *only used* in that context, that's the reason I recommended the naming.
(https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1755611792)
> Pushing has a specific meaning in the context of Bitcoin Script (it's an opcode that pushes data onto the stack), and that's not what's happening here so that would be very confusing.
The new pair of *private* functions together implement `OP_PUSHDATA`, and are *only used* in that context, that's the reason I recommended the naming.
💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755612640)
Thanks, I'll fix it up in the next follow-up, or if I have to re-push here, to avoid invalidating review over a harmless test style-nit.
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755612640)
Thanks, I'll fix it up in the next follow-up, or if I have to re-push here, to avoid invalidating review over a harmless test style-nit.
💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755647995)
> While I agree with maflcko that [keeping functionality hard to use outside of the intended context](https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752494409) has value, keeping tests straightforward is more important. If you prefer...
Thank you for the diff. However, I think that real code should be designed without taking tests into account (apart from being testable). Tests should always follow real code, not the other way round. I am sure the verbosity in the test can be redu
...
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755647995)
> While I agree with maflcko that [keeping functionality hard to use outside of the intended context](https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1752494409) has value, keeping tests straightforward is more important. If you prefer...
Thank you for the diff. However, I think that real code should be designed without taking tests into account (apart from being testable). Tests should always follow real code, not the other way round. I am sure the verbosity in the test can be redu
...
👍 TheCharlatan approved a pull request: "build: Add more cmake presets"
(https://github.com/bitcoin/bitcoin/pull/30871#pullrequestreview-2298717809)
ACK f15e817811e328423ea489870ead089128a6ef8c
(https://github.com/bitcoin/bitcoin/pull/30871#pullrequestreview-2298717809)
ACK f15e817811e328423ea489870ead089128a6ef8c
👍 hodlinator approved a pull request: "refactor: Allow `CScript`'s `operator<<` to accept spans, not just vectors"
(https://github.com/bitcoin/bitcoin/pull/30765#pullrequestreview-2298723649)
re-ACK 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7
`git range-diff master e6a5ab7 5e190cd`
- [Disagree](https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1755611792) with `AppendData`/`AppendDataSize` names over `PushData`/`PushDataSize`, but will let it go.
- Nice switch back from pointer arithmetic to `span` in `AppendData`.
- Touched up commit messages per my latest review, cheers!
- Implements legacy `operator<<` in terms of the other. I agree with l0rinc's reservations against
...
(https://github.com/bitcoin/bitcoin/pull/30765#pullrequestreview-2298723649)
re-ACK 5e190cd11f6ed8b7ab4db0f01192de63deaf6fd7
`git range-diff master e6a5ab7 5e190cd`
- [Disagree](https://github.com/bitcoin/bitcoin/pull/30765#discussion_r1755611792) with `AppendData`/`AppendDataSize` names over `PushData`/`PushDataSize`, but will let it go.
- Nice switch back from pointer arithmetic to `span` in `AppendData`.
- Touched up commit messages per my latest review, cheers!
- Implements legacy `operator<<` in terms of the other. I agree with l0rinc's reservations against
...
💬 hodlinator commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755702207)
> I think that real code should be designed without taking tests into account (apart from being testable). Tests should always follow real code, not the other way round.
Making the function return a value allowed for C) dropped negatives (and less runtime testing). But I can try to provide a new diff bringing them back and without changing *string.h*. My main point remains adding the interlaced tinyformat tests to prove parity.
> The diff you shared also changes `util::hex_literals::detail
...
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755702207)
> I think that real code should be designed without taking tests into account (apart from being testable). Tests should always follow real code, not the other way round.
Making the function return a value allowed for C) dropped negatives (and less runtime testing). But I can try to provide a new diff bringing them back and without changing *string.h*. My main point remains adding the interlaced tinyformat tests to prove parity.
> The diff you shared also changes `util::hex_literals::detail
...
💬 hodlinator commented on pull request "doc: clarify loadwallet path loading for wallets":
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1755723676)
Might be clearer like this:
```suggestion
{"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "You may pass in the absolute path of your wallet directory (or .dat file for legacy wallets). Otherwise, the interpreted path will be relative to the default wallet directory."},
```
(https://github.com/bitcoin/bitcoin/pull/30302#discussion_r1755723676)
Might be clearer like this:
```suggestion
{"filename", RPCArg::Type::STR, RPCArg::Optional::NO, "You may pass in the absolute path of your wallet directory (or .dat file for legacy wallets). Otherwise, the interpreted path will be relative to the default wallet directory."},
```
💬 maflcko commented on pull request "util: Use consteval checked format string in FatalErrorf, LogConnectFailure":
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755737721)
> > The diff you shared also changes `util::hex_literals::detail::Hex`, which I don't really understand.
>
> It's a consequence of clashes between `util::detail` namespaces. (Added note about that in a later edit).
Thanks for explaining. I think this is another reason to keep `string.h` as-is for now.
> My main point remains adding the interlaced tinyformat tests to prove parity.
I think the `CheckTooMany` may be a bit too verbose and mostly unit tests for tinyformat itself. Especial
...
(https://github.com/bitcoin/bitcoin/pull/30546#discussion_r1755737721)
> > The diff you shared also changes `util::hex_literals::detail::Hex`, which I don't really understand.
>
> It's a consequence of clashes between `util::detail` namespaces. (Added note about that in a later edit).
Thanks for explaining. I think this is another reason to keep `string.h` as-is for now.
> My main point remains adding the interlaced tinyformat tests to prove parity.
I think the `CheckTooMany` may be a bit too verbose and mostly unit tests for tinyformat itself. Especial
...
💬 maflcko commented on pull request "Lint: improve subtree exclusion":
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2344790531)
Are you still working on this?
(https://github.com/bitcoin/bitcoin/pull/29965#issuecomment-2344790531)
Are you still working on this?
👍 hodlinator approved a pull request: "test: support std::optional in BOOST_CHECK_* and increase FromUserHex fuzz feature coverage"
(https://github.com/bitcoin/bitcoin/pull/30618#pullrequestreview-2298831735)
ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa
Provides better error messages for tests.
nit: 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa - "Compare FromUserHex result against other hex validators and parsers" is not dependent on the other changes, so could be moved first instead of last if you re-touch.
(https://github.com/bitcoin/bitcoin/pull/30618#pullrequestreview-2298831735)
ACK 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa
Provides better error messages for tests.
nit: 1eac96a503b6bac3eaf5d0eb3d23ffde3bfbf9aa - "Compare FromUserHex result against other hex validators and parsers" is not dependent on the other changes, so could be moved first instead of last if you re-touch.
💬 kravens commented on issue "GUI event loop should be block free":
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2344798265)
Screen recording: [BitcoinCore-screen0.webm](https://github.com/user-attachments/assets/db653667-6b83-4089-b85e-69d84bcebede)
I'm testing 28.X release candidate branch, is this behaviour due to these lock-ups in the GUI code?
(https://github.com/bitcoin/bitcoin/issues/17145#issuecomment-2344798265)
Screen recording: [BitcoinCore-screen0.webm](https://github.com/user-attachments/assets/db653667-6b83-4089-b85e-69d84bcebede)
I'm testing 28.X release candidate branch, is this behaviour due to these lock-ups in the GUI code?
💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755770730)
Same re: `HaveCoin`
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755770730)
Same re: `HaveCoin`
💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755770946)
Same re: `HaveCoin`
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755770946)
Same re: `HaveCoin`
💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755753080)
This line should be moved up to just under the if statement to keep behavior the same. The coin is already moved into `ret->second.coin` before we potentially call `AddFlags`.
Although this will hopefully go away in #30673.
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755753080)
This line should be moved up to just under the if statement to keep behavior the same. The coin is already moved into `ret->second.coin` before we potentially call `AddFlags`.
Although this will hopefully go away in #30673.
💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755770549)
I don't think we should be changing `HaveCoin` lines in a commit about `GetCoin`.
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755770549)
I don't think we should be changing `HaveCoin` lines in a commit about `GetCoin`.
💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755771231)
Same re: `HaveCoin`
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1755771231)
Same re: `HaveCoin`