💬 Roasbeef commented on pull request "Add test for negative transaction version w/ CSV to tx_valid.json":
(https://github.com/bitcoin/bitcoin/pull/29291#issuecomment-1904769620)
> ACK. Somewhat surprisingly, decoderawtransaction is showing a positive version number:

From 2019 onwards, bitcoind started to display transactions versions as unsigned everywhere: https://github.com/bitcoin/bitcoin/pull/16525

This was prompted by a `rust-bitcoin` issue pointing out they were using a `u32` instead of a `i32` for their transaction struct.

💬 ryanofsky commented on issue "assumeutxo: nTx and nChainTx violations in CheckBlockIndex":
(https://github.com/bitcoin/bitcoin/issues/29261#issuecomment-1904776805)
> Do we not consider regtest "testing"?

The comment is vague, but it's really referring to unit tests.

When I suggested adding the two conditions allowing `pindex->nChainTx == 0` "for testing" in https://github.com/bitcoin/bitcoin/pull/27596#discussion_r1323226418, the reason was to avoid crashes in unit tests, because some unit tests skipped calling `ReceivedBlockTransactions` and therefore skipped setting a `nChainTx` value:

https://github.com/bitcoin/bitcoin/blob/651fb034d85eb5db561b
...

💬 brunoerg commented on pull request "rpc: improve submitpackage documentation and other improvements":
(https://github.com/bitcoin/bitcoin/pull/29292#discussion_r1462427700)
```suggestion
"The package must solely consist of a child and its parents. None of the parents may depend on each other.\n"
```

👋 LarryRuane's pull request is ready for review: "test: test_bitcoin: allow -testdatadir=<datadir>"
(https://github.com/bitcoin/bitcoin/pull/26564)

💬 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_r1462348934)
In "Implement ImprovesFeerateDiagram" (9257f33d259bf68f1df1cc41b7b3caaea341782f):
Nit: The documentation is in opposite order as the checks. Perhaps switch these two lines.

💬 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_r1462350586)
In "Implement ImprovesFeerateDiagram" (9257f33d259bf68f1df1cc41b7b3caaea341782f):

```suggestion
return strprintf("%s has both ancestor and descendant, exceeding cluster limit of 2", txid_string);
```

💬 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_r1462332681)
In the commit message of "Implement ImprovesFeerateDiagram" (9257f33d259bf68f1df1cc41b7b3caaea341782f):

> This new function takes the populated sets of
> direct and all conflicts computed in the current
> mempool, assuming the replacements are a single
> chunk, and computes a diagram check.

The first sentence is confusing to me. Could you perhaps clarify "the populated sets of direct and all conflicts" and split the sentence up?

💬 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_r1462365378)
In "Implement ImprovesFeerateDiagram" (9257f33d259bf68f1df1cc41b7b3caaea341782f):
Maybe that’s my unfamiliarity with the mempool code, but it is not obvious to me what "direct_conflicts" and "all_conflicts" are, and the description here is self-referential. Do these transactions belong to original, replacement, or both, etc.?

💬 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_r1462453899)
In "test: Add tests for CompareFeerateDiagram and CheckConflictTopology" (b3d415fe84be7edfbed567a79a25d406b438622b):
Nit: I found the comment here confusing. How about: "`new_diagram` is strictly better due to the first chunk, while the second chunk is worse."

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