💬 hebasto commented on pull request "Dialog for allowing the user to choose the change output when bumping a tx":
(https://github.com/bitcoin-core/gui/pull/700#issuecomment-2444551530)
`test_bitcoin-qt` fails in the CI: https://github.com/bitcoin-core/gui/actions/runs/11186485220.

💬 hodlinator commented on pull request "build: increase minimum supported Windows to 10.0":
(https://github.com/bitcoin/bitcoin/pull/31172#issuecomment-2444563081)
Using GUIX to produce binaries, I'm able to run the base commit of this PR (bitcoin-da10e0bab4a3-win64-setup-unsigned.exe), but unable to run the binaries installed from the current PR commit (bitcoin-7dd0ee89a092-win64-setup-unsigned.exe):
<img width="297" alt="Screenshot 2024-10-29 160928" src="https://github.com/user-attachments/assets/1d4d0731-898f-4925-8146-9030484b7245">

System:
Edition Windows 11 Home
Version 23H2
OS build 22631.4317

💬 l0rinc commented on pull request "refactor: prohibit direct flags access in CCoinsCacheEntry and remove invalid tests":
(https://github.com/bitcoin/bitcoin/pull/30906#discussion_r1821014127)
It seems this [was introduced in 2017](https://github.com/bitcoin/bitcoin/commit/50830796889ecaa458871f1db878c255dd2554cb#diff-f0ed73d62dae6ca28ebd3045e5fc0d5d02eaaacadb4c2a292985a3fbd7e1c77cR100), when the project was still on [C++11](https://github.com/bitcoin/bitcoin/blob/342b9bc3907edf8eae64440397a32833ed44fae4/configure.ac#L58), but `try_emplace` was only introduced in [C++17](https://en.cppreference.com/w/cpp/container/map/try_emplace).

When this was split out to [`EmplaceCoinInternalDA
...

✅ stickies-v closed a pull request: "tinyformat: enforce compile-time checks for format string literals"
(https://github.com/bitcoin/bitcoin/pull/31149)

💬 stickies-v commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#issuecomment-2444579999)
Closing this PR in favour of https://github.com/bitcoin/bitcoin/pull/31174, which I think achieves mostly the same goal but does so in a much more elegant way. Thanks for your review and suggestion @ryanofsky, and everyone else here for the persistent re-review as this work is evolving.

> and the other changes here are basically just a side-effect of the approach taken to implement it.

I think there is merit in making the less safe (i.e. `std::string` overload) less convenient to use so th
...

🤔 mzumsande reviewed a pull request: "rpc: Remove submitblock pre-checks"
(https://github.com/bitcoin/bitcoin/pull/31175#pullrequestreview-2402259498)
Haven't looked deeply yet, just wanted to mention #10146, which introduced the coinbase check and was even backported (see also #10190 for a regression test). Makes me wonder if there is more historical context to this.

💬 fanquake commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2444644193)
This seems fine, but it'd be good to primarily list the benefits to our project, rather than justifying making changes based on if some other open source project happens to do it, as other projects may have different constraints, or various reasoning for making the same change. Also, in this PR you say `This approach is widely adopted by the large projects, such as LLVM`, but in the secp256k1 thread for the same change you said that our current behaviour is ["the default behaviour adopted by man
...

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2444666677)
> My understanding is that this change is mostly a convenience change for native Windows developers, so that it becomes easier to run binaries after compilation (without installing?).

Correct.

> What I don't really understand is how every other project that uses CMake as we do now, has solved this issue, if they haven't made the same change as in this PR (or why this wouldn't be the default CMake behaviour, if it otherwise results in broken (native) Windows binaries).

There are alternat
...

💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1821063869)
re: https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820938133

> we still have unbounded increment without checking the end (I though we've fixed this already, maybe it got stuck in the comments...)

Thanks. I fixed this by just switching the string type to `const char*` instead of `string_view` since tinyformat already assumes the string is null terminated.

I think it would be possible to write a clean version of this code that used `string_view`, but it would have to be struc
...

💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820947389)
re: https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820457426

Thanks, I added the suggested test cases. The suggested tests that "should fail" didn't actually fail with 97dd5fe5128592332c83998825bbeda063815120 because it accepted `\0` as a valid specifier character, so I added an extra check to prevent that. I also added extra code to consume digits after `.` otherwise format strings like `"%1.2"` would be accepted treating `2` as the specifier.

💬 ryanofsky commented on pull request "tinyformat: Add compile-time checking for literal format strings":
(https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820954447)
re: https://github.com/bitcoin/bitcoin/pull/31174#discussion_r1820543443

Thanks, applied patch. The reason for having a `FormatString` class was to provide a cleaner escape from compile-time checks `strprintf(FormatString{"%*s"}, width, str)` before the first commit was implemented. But it's no longer necessary after that commit.

🤔 ryanofsky reviewed a pull request: "tinyformat: Add compile-time checking for literal format strings"
(https://github.com/bitcoin/bitcoin/pull/31174#pullrequestreview-2402142576)
Updated 1d16d6e6bac994fed7c695f530b9984edcd290bd -> e6086e00e32e486aaeeeb346ccca1377bbf647b2 ([`pr/tcheck.1`](https://github.com/ryanofsky/bitcoin/commits/pr/tcheck.1) -> [`pr/tcheck.2`](https://github.com/ryanofsky/bitcoin/commits/pr/tcheck.2), [compare](https://github.com/ryanofsky/bitcoin/compare/pr/tcheck.1..pr/tcheck.2)) addressing comments and making `ConstEvalFormatString` parsing stricter to reject incomplete specifiers.

💬 theuni commented on pull request "consensus: fix `OP_1NEGATE` handling in `CScriptOp`":
(https://github.com/bitcoin/bitcoin/pull/29589#issuecomment-2444682990)
I'm ~0 on this leaning towards NACK. As far as I can tell, all code paths are currently gated by `if (>= OP_1 && <= OP_16)` when calling `DecodeOP_N`/`DecodeOP_N`.

So this is really only changing the intention of those functions. And since it would be unsafe (because of `OP_RESERVED`) to change those gates in the callers to `>= OP_1NEGATE && <= OP_16`, it seems like `OP_1NEGATE` is going to have to remain a special case for callers anyway.

💬 fanquake commented on pull request "cmake: Add `FindZeroMQ` module":
(https://github.com/bitcoin/bitcoin/pull/30903#issuecomment-2444700876)
We should also follow up with refactoring the libevent module, to more generically use CMake/pkg-config, rather than restricting the CMake usage to `vcpkg`. At that point, we'd likely be able to dump pkg-config for the depends path entirely.

👍 fanquake approved a pull request: "cmake: Add `FindZeroMQ` module"
(https://github.com/bitcoin/bitcoin/pull/30903#pullrequestreview-2402413240)
ACK 915640e191b6a17a245f0502bc399d82a6502ccf

💬 hebasto commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2444707227)
re: https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2437357680

> It would be good if ctest/Windows actually printed the exception and error message instead of silently hiding it.

I'm not aware of the specific case discussed above, but exceptions in tests are generally printed as follows (with a patched code):
```
> ctest --test-dir build-static -j 8 -C Release -R amount --output-on-failure
Internal ctest changing into directory: C:/Users/hebasto/bitcoin/build-static
Test pro
...

💬 theStack commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2444734571)
> My understanding is that this change is mostly a convenience change for native Windows developers, so that it becomes easier to run binaries after compilation (without installing?).

Even as non-Windows developer, I personally find it quite handy if all binaries end up in a single folder and are not scattered around in different places, unnecessarily still reflecting the in-tree source structure (resulting sometimes in long nested paths, as one can see in the scripted-diff). Turned out to b
...

🚀 fanquake merged a pull request: "cmake: Add `FindZeroMQ` module"
(https://github.com/bitcoin/bitcoin/pull/30903)

💬 petertodd commented on issue "V2 Only Option":
(https://github.com/bitcoin/bitcoin/issues/29618#issuecomment-2444826150)
On Thu, Oct 24, 2024 at 02:52:55AM -0700, laanwj wrote:
> > CJDNS is still a routed ove rlay network, where packets do not always go the shortest (internet) route to the destination. That is a latency hit vs native IP.
>
> There are some edge cases where super low latency matters, such as mining (or spy nodes :sweat_smile: ), but in general, propagation of anything over the P2P network can be slow and that's fine. There's no tight UI loop. It's not worth to compromise privacy over latency-like
...

💬 ryanofsky commented on pull request "tinyformat: enforce compile-time checks for format string literals":
(https://github.com/bitcoin/bitcoin/pull/31149#issuecomment-2444839496)
> I think there is merit in making the less safe (i.e. `std::string` overload) less convenient to use so that developers who are less familiar with this code don't accidentally use it when they don't have to, but I'll leave a comment on your PR so we can have the conversation there.

I agree with this, and commit b6a39c81e85338bc82f3db924157a599aa7e25fa in this PR shows places where code is doing things like `strprintf("[" + comment + "]")` that get around compile time checking. It'd be great
...