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

💬 hebasto commented on pull request "cmake: Set top-level target output locations":
(https://github.com/bitcoin/bitcoin/pull/31161#issuecomment-2581313846)
@ryanofsky

> Code review ACK [06ff942](https://github.com/bitcoin/bitcoin/commit/06ff9429d32943aa4debec38ea105c8af4283aec). It seems like a improvement to me overall if binaries in the build directory have the same layout as binaries in releases. I'm not sure current implementation of this is ideal but it seems like a a step in the right direction.
>
> Having binaries built in consistent locations that match install locations should make it is easier to find them, make scripts and wrapper
...

💬 theuni commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909477090)
> > 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.

Afaik the move/swap idiom is only safe if t
...

💬 theuni commented on pull request "depends: Use base system's `sha256sum` utility":
(https://github.com/bitcoin/bitcoin/pull/31626#discussion_r1909485967)
Yes, it's from `coreutils`. But shasum exists out of the box.

💬 theuni commented on pull request "cluster mempool: add TxGraph reorg functionality":
(https://github.com/bitcoin/bitcoin/pull/31553#discussion_r1909492867)
Ah, right, I missed that the move ctor would handle the update. Thanks for explaining.

💬 psgreco commented on issue "Potential crash (assert) rescanning wallet":
(https://github.com/bitcoin/bitcoin/issues/31474#issuecomment-2581367570)
Looks good, thank you!
Do you want me to ack or do anything in https://github.com/bitcoin/bitcoin/pull/31629 ?