💬 murchandamus commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1462385881)
In "Implement ImprovesFeerateDiagram" (9257f33d259bf68f1df1cc41b7b3caaea341782f):
Nit: Spaces around that minus? It took me a moment to realize that dashes aren’t a thing in variables.

💬 murchandamus commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1462457196)
In "test: Add tests for CompareFeerateDiagram and CheckConflictTopology" (b3d415fe84be7edfbed567a79a25d406b438622b):

Nit: in the new diagram, the last two chunks should be one chunk because the last chunk is better (151 sats, 150 vB) than the prior (249 sats, 250 vB).

💬 murchandamus commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1462412782)
In "Implement ImprovesFeerateDiagram" (9257f33d259bf68f1df1cc41b7b3caaea341782f):

```suggestion
* @param[in] direct_conflicts All transactions that would be removed directly (not by being descendants of conflicting transactions)
```

💬 murchandamus commented on pull request "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2":
(https://github.com/bitcoin/bitcoin/pull/29242#discussion_r1462462868)
In "test: Add tests for CompareFeerateDiagram and CheckConflictTopology" (b3d415fe84be7edfbed567a79a25d406b438622b):

This is the same old_diagram. Did you mean to repeat the same transactions,
```suggestion

old_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 399}};
new_diagram = {FeeFrac{0, 0}, FeeFrac{950, 300}, FeeFrac{1050, 400}};
```
or just test in both directions?
```suggestion
```

🤔 murchandamus reviewed a pull request: "Mempool util: Add RBF diagram checks for single chunks against clusters of size 2"
(https://github.com/bitcoin/bitcoin/pull/29242#pullrequestreview-1837475718)
Concept ACK, tentative crACK f094a0370db7085b8a89842de0b6d12272d826cb

⚠️ gentlee opened an issue: "Mac App UI is freezed most of the time, with no active peers"
(https://github.com/bitcoin/bitcoin/issues/29293)
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current behaviour

App is not responding most of the time. It is totally unusable. And also there is no active peers - progress is stopped.

### Expected behaviour

1. App never freezes UI and is always responsive.
2. There are active working peers without the need of manually add them.

### Steps to reproduce

Run the app.

### Relevant log output

If it contains personally identifying information then sor
...

💬 mzumsande commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1905064561)
> So there is unexpected behavior here if bitcoin-qt is crashing without these conditions

This is not specific to `bitcoin-qt`, or assumeutxo. For example, It also happens with `bitcoin-cli -regtest -generate 1` (after creating an empty wallet) on an empty datadir.

I think the comment `// For testing, allow transaction counts to be completely unset.` is wrong. `CheckBlockIndex` traveerses the entire block index tree, no matter if we have the block on disk or not. If we don't have it, `Rece
...

💬 ryanofsky commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1905087355)
Oops, sorry I missed that context so my explanation doesn't really describe what the checks are doing (although it does describe what they were intending to do). If the PR is just going to change the comments and delete the words "For testing," that sounds good.

💬 edilmedeiros commented on pull request "depends: Update libmultiprocess library to fix C++20 macos build error":
(https://github.com/bitcoin/bitcoin/pull/29276#issuecomment-1905114677)
Tested ACK.

I was watching the issue in chaincodelabs/libmultiprocess#92 and confirmed the fix in chaincodelabs/libmultiprocess#93.

I built and executed bitcoin-core tests with this PR following the official setup with Brew.

I could not compile this with dependencies coming from Macports, but the issue seems to be not related to the fix here as it does not compile either with `--disable-multiprocess` (I understand Macports is not supported anyhow, see #15175).

💬 andrewtoth commented on pull request "rpc,rest,zmq: faster getblock, NotifyBlock and rest_block by reading raw block":
(https://github.com/bitcoin/bitcoin/pull/26415#discussion_r1462614057)
Done, added as a separate commit at the start of this PR.
Yes, it could be handled by the caller, but it's really handled by `OpenBlockFile` which checks for a null `pos`. It's just that before that check it blindly decrements the nPos by 8. Now we check that and return the same error that would be returned by `OpenBlockFile` if `pos` is null.

💬 ryanofsky commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1905118176)
Other notes on improving this assert:

- The final pindex->IsAssumedValid() check added in [#28791](https://github.com/bitcoin/bitcoin/pull/28791) seems a little overbroad. I think the condition `(pindex->nChainTx == pindex->nTx + prev_chain_tx)` should actually be true for all assumed-valid blocks except the first one and the last one, so that check could be tightened up
- It might possible to add a test covering the assert failure that pindex->IsAssumedValid() from [#28791](https://github.c
...

💬 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
...