💬 edilmedeiros commented on issue "Build error on macOS, tag v26.0 builds fine":
(https://github.com/bitcoin/bitcoin/issues/29289#issuecomment-1905127463)
How are you getting your dependencies?

I just compiled it in a box with Brew following the official instructions, and I could build it.
When using a box with Macports, the build breaks too.

Both have Apple clang version 15.0.0 (clang-1500.1.0.2.5) in Sonoma 14.2.

💬 luke-jr commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1905229921)
Concept NACK, I think? Isn't this used by libbitcoin and/or btcd??

💬 luke-jr commented on pull request "build: always set `-g -O2` in `CORE_CXXFLAGS`":
(https://github.com/bitcoin/bitcoin/pull/29205#issuecomment-1905239736)
>While always overriding Autotools' defaults, are there any particular reasons not using -O3 instead of -O2?

AFAIK `-O3` isn't safe.

💬 jarolrod commented on issue "Build error on macOS, tag v26.0 builds fine":
(https://github.com/bitcoin/bitcoin/issues/29289#issuecomment-1905241363)
I would check the clang version on your system, in any case the [dependencies doc](https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md) now states that the minimum required clang is 14.

Cannot replicate on a fresh install of macOS 12.0.1 which has Apple Clang version 13.1.6 and the command line developers tools script installed Xcode 13.4. Note that, according to [this](https://trac.macports.org/wiki/XcodeVersionInfo#Xcode12.5.1), It does seem that depending on when you had ins
...

💬 luke-jr commented on pull request "Support self-hosted Cirrus workers on forks (and multi-user)":
(https://github.com/bitcoin/bitcoin/pull/29274#issuecomment-1905270810)
>I'm somewhat puzzled why Knots and Inquisition are not running into the same issues

Maybe because I switch the self-hosted CI to not-self-hosted?

💬 luke-jr commented on pull request "refactor: Fix prevector iterator concept issues":
(https://github.com/bitcoin/bitcoin/pull/29275#issuecomment-1905271507)
Can we split the fixes from the C++20 stuff? The latter shouldn't be backported...

💬 luke-jr commented on pull request "RPC: Wallet: Add `maxfeerate` and `maxburnamount` startup option":
(https://github.com/bitcoin/bitcoin/pull/29278#issuecomment-1905273019)
I agree this feels more like it should be a wallet option.

💬 maflcko commented on pull request "refactor: Fix prevector iterator concept issues":
(https://github.com/bitcoin/bitcoin/pull/29275#issuecomment-1905468506)
While it is safe to backport, I don't anything should be backported, unless there is a reason and need. A missing (default) constructor, if it was needed, would result in a compile failure. Given that the previous releases compile, this is not needed. Similar arguments can be made for the other commits.

In any case, every commit is already a separate commit, so it is not possible to split further. Anyone is free to backport any or all commits, or none at all.

💬 maflcko commented on pull request "[WIP] C++20 std::views::reverse":
(https://github.com/bitcoin/bitcoin/pull/28687#issuecomment-1905470153)
> > WIP: our `prevector` implementation requires modification for `std::ranges::views::reverse` to work. More specifically: it needs to implement the [`bidirectional_range`](https://en.cppreference.com/w/cpp/ranges/bidirectional_range) concept, which currently it seems not to (see `static_assert(std::ranges::bidirectional_range<prevector>)` output below) as we're not satisfying [`range`](https://en.cppreference.com/w/cpp/ranges/range). Once that's done, we can update the last usage of `reverse_i
...

💬 maflcko commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1905488484)
> > `$ ./src/qt/bitcoin-qt -datadir=/tmp -regtest`
>
> Do we not consider regtest "testing"?

Regtest should have the same properties as `main` for the purposes here. It should be possible to reproduce on main as well, if you want to do the POW.

💬 maflcko commented on pull request "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`":
(https://github.com/bitcoin/bitcoin/pull/29287#issuecomment-1905546034)
lgtm ACK 19fbbb1d00f40095b0b6da5941ff069376800e7f

💬 delta1 commented on pull request "depends: Do not override `CFLAGS` when building SQLite with `DEBUG=1`":
(https://github.com/bitcoin/bitcoin/pull/29287#issuecomment-1905548068)
ACK 19fbbb1

💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1905560370)
@maflcko `WriteBinaryFile` is used by Tor and I2P to cache the service private key.

But I might still close this PR if all that's needed is one-liner.

💬 ajtowns commented on pull request "RFC: Deprecate libconsensus":
(https://github.com/bitcoin/bitcoin/pull/29189#issuecomment-1905602751)
> btcd

libbitcoin optionally uses https://github.com/libbitcoin/libbitcoin-consensus which has a similar name, but doesn't seem to have any code in common. AFAIK, btcd reimplements all the consensus logic in go.

💬 willcl-ark commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1905642808)
Ah I see now, thanks. The repro steps threw me off there. I did now also verify that it is hit on (a custom) signet (with `-checkblocks` set).

📝 Sjors converted_to_draft a pull request: "Make (Read/Write)BinaryFile work with char vector"
(https://github.com/bitcoin/bitcoin/pull/29229)
ReadBinaryFile and WriteBinaryFile current work with `std::string`. This PR adds support for `std::vector<unsigned char>>`.

This is used in #28983 to store the static key for the Template Provider, in a manner very similar to how we store the Tor v3 and i2p key. However it made no sense to me to store a `CKey` as plain text. See commit "Persist static key for Template Provider" for how it's used.

It uses a template and leverages the fact that both `std::string` and `std::vector<unsigned ch
...

💬 Sjors commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1905684497)
It's not quite a one-liner because you still need to open a close a `FILE` and deal with various errors. So instead I modified `[Write/Write]BinaryFile` to use `AutoFile` instead of `fwrite` / `fread`.

However, `AutoFile{f} >> output` only returns a fraction of the file in the test...

💬 maflcko commented on pull request "Make (Read/Write)BinaryFile work with char vector":
(https://github.com/bitcoin/bitcoin/pull/29229#issuecomment-1905703500)
> However, `AutoFile{f} >> output` only returns a fraction of the file in the test...

(De)serialization of vectors or strings assumes the run-time length to be encoded first. Only arrays and spans assume no length, because it is assumed to be known at compile-time.

💬 fanquake commented on pull request "Avoid non-self-contained Windows header":
(https://github.com/bitcoin-core/gui/pull/789#discussion_r1463031223)
Comment should be removed? We are removing these "for xyz" comments in any case, but this one would also now be incorrect.

💬 brunoerg commented on pull request "fuzz: extend ConsumeNetAddr() to return I2P and CJDNS addresses":
(https://github.com/bitcoin/bitcoin/pull/26859#issuecomment-1905743813)
Concept ACK