💬 dergoegge commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2388558223)
Concept ACK

This option seems useful for users that want to:

* prevent passive spying on their connections
* increase the cost of tampering with the data exchanged on their connections
* increase the cost of detecting that they run a bitcoin node

> Currently 59.71% of network supports v2

Have you checked how diverse these ~60% of v2 supporting nodes are? An extreme example: if they are all on running on AWS then I don't think we should add the option at this time.

💬 Sjors commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2388568119)
@hebasto now I can't build QT with depends anymore (291ec5e2a876622a051baf6e33a4d4d592e6568e): https://gist.github.com/Sjors/e42242b27b71c98ee10d60d58a20872a

(since the last time I tested I uninstalled Xcode as part of debugging #30978)

💬 josibake commented on issue "RFC: Multiprocess binaries and packaging options":
(https://github.com/bitcoin/bitcoin/issues/30983#issuecomment-2388619208)
Echoing others, I like option 2 (side-binaries). A `bitcoin <cmd>` wrapper on top of multiple binaries also seems quite nice. While this might cause some confusion for end users, I think this will be far less confusing than having a side release (and much less work for maintainers and guix builders).

Regarding option 3, one of the value propositions of multiprocess in my mind is being able to run a binary for a specific purpose, with only the code needed for that purpose in the binary, e.g.,
...

💬 instagibbs commented on pull request "refactor: TxDownloadManager + fuzzing":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2388665904)
reACK ce205abe10ebccfdbea60adf4c568a8ba61390c3

thanks for picking up on some of those old nits

📝 maflcko opened a pull request: "test: Treat exclude list warning as failure in CI"
(https://github.com/bitcoin/bitcoin/pull/31018)
An outdated exclude list or otherwise an error in the exclude list handling is usually a bug.

So make it fatal in the CI, instead of silently ignoring it.

Fixes https://github.com/bitcoin/bitcoin/pull/30872/files#r1757015334

Can be tested with something like (with and without `--ci`):

```
./bld-cmake/test/functional/test_runner.py wallet_disable -x wallet_disablee

💬 maflcko commented on pull request "test: fix exclude parsing for functional runner":
(https://github.com/bitcoin/bitcoin/pull/30872#discussion_r1784546679)
Fixed in https://github.com/bitcoin/bitcoin/pull/31018

💬 josibake commented on issue "rfc: DATUM mining interface requirements":
(https://github.com/bitcoin/bitcoin/issues/31002#issuecomment-2388674235)
I think this highlights a clear advantage of having a more "generic" mining interface exposed over an IPC interface: different mining protocols can use the same interface from Bitcoin Core, without needing protocol specific[^1] changes for each protocol to be implemented in Bitcoin Core.

As you mentioned, our current interface is designed with SV2 in mind, but given this is all relatively new code, it would be great to hear from the folks building DATUM whether or not this interface works out
...

💬 jonatack commented on pull request "doc: update IBD requirements in doc/README.md":
(https://github.com/bitcoin/bitcoin/pull/30992#issuecomment-2388680568)
> The commit message was created automatically when I clicked the "Commit Suggestion" button on your previous feedback. Fixing it now.

@Mackain The commit message wasn't updated yet, could you update it and re-push the same commit (thanks!)

💬 maflcko commented on pull request "rpc: add getorphantxs":
(https://github.com/bitcoin/bitcoin/pull/30793#issuecomment-2388688159)
Please report the CI failure. Otherwise, it will be harder to track and fix it. This is explained in the log:

```
test 2024-10-02T00:55:45.897000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log.
test 2024-10-02T00:55:45.897000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues

🤔 hodlinator reviewed a pull request: "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error"
(https://github.com/bitcoin/bitcoin/pull/30928#pullrequestreview-2342962102)
stickies-v:
> All format strings are known at compile-time, so any formatting errors are programming errors. Handling that with `util::Result` would be both a cumbersome interface and bad practice imo.

This is a good argument except we do have some strings only known at runtime, notably translations, which probably don't get full testing. Maybe there's something that automatically compares format strings for equivalence across translations though.

maflcko:
> I agree that `util::Result` d
...

💬 hodlinator commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#discussion_r1784535464)
Changing tinyformat to support `string_view` is definitely out of scope for this PR.

I would at least want to make it clearer that format strings are separate from other parameters, like this or with other words to that effect:
```suggestion
- *Rationale*: Tinyformat handles string parameters. Format strings
on the other hand should be known at compile-time, so using a string literal or
`constexpr const char*` allows for compile-time validation.
```
Even though I decided
...

💬 hodlinator commented on pull request "test: Prove+document ConstevalFormatString/tinyformat parity":
(https://github.com/bitcoin/bitcoin/pull/30933#issuecomment-2388722857)
@maflcko
> Are you still working on this?

I'm waiting on #30928 before un-drafting this.

💬 maflcko commented on pull request "build: Improve `ccache` performance for different build directories":
(https://github.com/bitcoin/bitcoin/pull/30861#issuecomment-2388728933)
Concept ACK

💬 l0rinc commented on pull request "cmake: Avoid hardcoding Qt's major version in Find module":
(https://github.com/bitcoin/bitcoin/pull/31010#discussion_r1784608370)
It seems to me we're still hardcoding the major version - can't we change this during migration instead?

💬 maflcko commented on pull request "tinyformat: refactor: increase compile-time checks and don't throw for tfm::format_error":
(https://github.com/bitcoin/bitcoin/pull/30928#issuecomment-2388768319)
> This is a good argument except we do have some strings only known at runtime, notably translations, which probably don't get full testing. Maybe there's something that automatically compares format strings for equivalence across translations though.

If translations are malformed to induce a tinyformat error where one wouldn't be present in the non-translated string, I don't think the solution would to use `Result`. This seems no different from an otherwise fully tinyformat-valid string that
...

💬 hebasto commented on pull request "cmake: Avoid hardcoding Qt's major version in Find module":
(https://github.com/bitcoin/bitcoin/pull/31010#discussion_r1784617293)
> It seems to me we're still hardcoding the major version

I meant the variable names. The full minimum required version was not supposed to change.

> can't we change this during migration instead?

It is done in https://github.com/bitcoin/bitcoin/pull/30997. This PR split from it to make the https://github.com/bitcoin/bitcoin/pull/30997 easier to review.

💬 vasild commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2388772718)
> increase the cost of detecting that they run a bitcoin node

How? Given that the spy does not need to inspect the traffic at all:

> My node will connect to publicly known bitcoin nodes and the mere fact that I have a TCP connection to an addr:port, where it is known that a bitcoin node is running, is already revealing enough.

💬 andremralves commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#issuecomment-2388779305)
Thanks for the feedback!

>I wonder if it's not easier to minimize what is being caught in the IOError try block and just remove missing bins from the BINARIES list? This list is used later on in the script too so in my opinion that is probably the better option. Otherwise we will end up referencing non-existing binaries/manpages in the existing manpages.

I agree and have updated the commit to reflect your suggestions. I just needed to make a copy of BINARIES so that it's possible to remove
...

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2388783616)
@fanquake
> Building [291ec5e](https://github.com/bitcoin/bitcoin/commit/291ec5e2a876622a051baf6e33a4d4d592e6568e) (on arm64):

Thanks! I explicitly disabled unused deployment tools and a few other features for the `native_qt` package.

💬 hebasto commented on pull request "build: Switch to Qt 6":
(https://github.com/bitcoin/bitcoin/pull/30997#issuecomment-2388792957)
> @hebasto now I can't build QT with depends anymore ([291ec5e](https://github.com/bitcoin/bitcoin/commit/291ec5e2a876622a051baf6e33a4d4d592e6568e)): https://gist.github.com/Sjors/e42242b27b71c98ee10d60d58a20872a
>
> (since the last time I tested I uninstalled Xcode as part of debugging #30978)

Based on the Qt source code, it appears that the build configuration may depend on Xcode. Could you please confirm whether reinstalling Xcode resolves the issue for you? If so, I will need to apply
...