💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909385059)
I believe it is safe, both with RVO and without (but since C++17 RVO is mandatory, so that suffices).

With RVO, `ret` is constructed directly in the caller's target destination, so this isn't a pointer to local stack space.

Without RVO, the `Ref(Ref&&)` move constructor is invoked, which will update the pointer to the caller's destination.

👍 theuni approved a pull request: "doc: upgrade license to 2025."
(https://github.com/bitcoin/bitcoin/pull/31611#pullrequestreview-2540859694)
ACK b537a2c02a9921235d1ecf8c3c7dc1836ec68131

Just missed it for 28.1, but this needs a backport in case we cut a future v28.

💬 hebasto commented on pull request "depends: Use base system's `sha256sum` utility":
(https://github.com/bitcoin/bitcoin/pull/31626#discussion_r1909390612)
Thanks! This change has been dropped.

💬 l0rinc commented on pull request "depends: Use base system's `sha256sum` utility":
(https://github.com/bitcoin/bitcoin/pull/31626#discussion_r1909395238)
hmmm, it was working for me on Sequoia - it likely comes from `coreutils`, right?

💬 hebasto commented on pull request "depends: Use base system's `sha256sum` utility":
(https://github.com/bitcoin/bitcoin/pull/31626#discussion_r1909397141)
FWIW, on my Sequoia (Intel):
```
% whereis sha256sum
sha256sum: /sbin/sha256sum /usr/share/man/man1/sha256sum.1
% sha256sum --version
sha256sum (Darwin) 1.0
```

👍 TheCharlatan approved a pull request: "depends: Fix spacing issue"
(https://github.com/bitcoin/bitcoin/pull/31627#pullrequestreview-2540881009)
ACK 8a46286da667d19414c30350df48ebf245589e32

💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909403690)
> Why is this doing an effective swap?

I think this is quite common, that move-construction is effectively performing a swap.

> I would expect this to call `UnlinkRef` on the moved-from value and reset its `m_graph` and `m_index`

That's possible too, and slightly more efficient I guess.

> Otherwise it wouldn't be unlinked until the moved-from variable goes out of scope, no?

Indeed. I don't think that's a problem.

💬 TheCharlatan commented on pull request "Safegcd-based modular inverses in MuHash3072":
(https://github.com/bitcoin/bitcoin/pull/21590#discussion_r1909426772)
I meant these checks:
https://github.com/bitcoin/bitcoin/blob/master/src/secp256k1/src/modinv32_impl.h#L414-L419
https://github.com/bitcoin/bitcoin/blob/master/src/secp256k1/src/modinv32_impl.h#L456-L459

But I guess they are not really that important and it is not worth adding the required utilities?

💬 luke-jr commented on pull request "util: detect and warn when using exFAT on MacOS":
(https://github.com/bitcoin/bitcoin/pull/31453#discussion_r1909438136)
The manpage says we need `#include <sys/param.h>` too

https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/statfs.2.html

📝 hannahz12 opened a pull request: "upstage install"
(https://github.com/bitcoin/bitcoin/pull/31631)
**upstage install**

✅ maflcko closed a pull request: "upstage install"
(https://github.com/bitcoin/bitcoin/pull/31631)

💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909458303)
Anyway, done!

💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909458391)
Done.

💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909462275)
It terminates because:
* Every chunk contains at least one element (added an Assume for that)
* In the inner loop, one element from that chunk is Reset() (added an Assume that it indeed resets a bit that was previously set).

I've changed it to a `do {} while(chunk.transactions.Any());` loop in the first commits, though it reverts back to a `while (true) { ... }` loop later, when the loop becomes a bit more complex.

💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909463152)
Done. I've also added a comment to the `Cluster::ApplyRemovals()` function definition stating that at least one element from the front of `to_remove` must belong to `this` Cluster (which is really why that requirement exists).

💬 sipa commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909463444)
Done. The third one disappears in a later commit, though.

📝 fanquake locked a pull request: "upstage install"
(https://github.com/bitcoin/bitcoin/pull/31631)
**upstage install**

🤔 mzumsande reviewed a pull request: "kernel: Move block tree db open to block manager"
(https://github.com/bitcoin/bitcoin/pull/30965#pullrequestreview-2540983303)
Code Review ACK b776e40554c8a315d832f3996d14ff2e3ae7b8cb

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#issuecomment-2581309699)
A few changes:
* Added a `TxGraph::DoWork()` functions which performs queued-up computations now, so that future operations are fast.
* Make `Ref&` arguments to `TxGraph::AddDependency`, `TxGraph::RemoveTransaction`, and `TxGraph::SetTransactionFee` const. They do not modify the `Ref` itself; they only modify what the `Ref` points to.
* Address review comments by @ismaelsadeeq here, and @theuni on #31553.

💬 sipa commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1909471752)
Done.