💬 mzumsande commented on pull request "wallet: fix rescanning inconsistency":
(https://github.com/bitcoin/bitcoin/pull/31629#issuecomment-2659846075)
> I think we could solve the first case here, so it can get inside the upcoming release, and leave the second case for a follow-up. Also, I'm not really against the fix proposed in this PR, it works. It just feels a bit hacky.

See https://github.com/mzumsande/bitcoin/tree/202502_rescan_initfix for this alternative. Happy to change it to that if people like it better. This fixes the issue during `AttachChain()` and is admittedly nicer, but it wouldn't fix the following race:
1) User runs `res
...

📝 hebasto opened a pull request: "cmake: Add `libbitcoinkernel` target"
(https://github.com/bitcoin/bitcoin/pull/31869)
This PR amends https://github.com/bitcoin/bitcoin/pull/31844 by:
1. Adding a convenience `libbitcoinkernel` target as a synonym for `bitcoinkernel`.
2. Renaming the `bitcoinkernel` component to `libbitcoinkernel`, as initially intended in https://github.com/bitcoin/bitcoin/pull/31844

💬 hebasto commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2659856484)
> Want to PR that along with a comment along the lines of "Add a convenience libbitcoinkernel target as a synonym for bitcoinkernel" and I'll give it a quick ACK?

https://github.com/bitcoin/bitcoin/pull/31869.

💬 theuni commented on pull request "optimization: speed up block serialization":
(https://github.com/bitcoin/bitcoin/pull/31868#discussion_r1956480589)
These are nice optims, but [as I mentoned here](https://github.com/bitcoin/bitcoin/pull/31519#discussion_r1929225185):

> In the future we could potentially add specializations for types with compile-time-known sizes via concepts.
>
> Presumably some of the streams could benefit from static extents, but that's waaay overkill for here.

I think it makes sense to wait for the `std::span` replacement (#31519) to do this, that way we can specialize for any static extent instead which should c
...

💬 theuni commented on pull request "refactor: Use std::span over Span":
(https://github.com/bitcoin/bitcoin/pull/31519#issuecomment-2659867175)
@fanquake do you have an opinion on this for v29? Getting it in would make future backports easier. Could also see waiting until after branch-off to be less disruptive though.

💬 hebasto commented on pull request "cmake: add a component for each binary":
(https://github.com/bitcoin/bitcoin/pull/31844#issuecomment-2659879903)
> ACK [9b033be](https://github.com/bitcoin/bitcoin/commit/9b033bebb18dfd609c02736292f37cc6589fcc8d).

My apologies not very solid testing, but this branch seems not always working:
```
$ cmake -B build -G Ninja -DBUILD_UTIL_CHAINSTATE=ON -DBUILD_SHARED_LIBS=OFF
$ cmake --build build --target bitcoinkernel
$ cmake --install build --component bitcoinkernel --prefix /home/hebasto/INSTALL
-- Install configuration: "RelWithDebInfo"
CMake Error at build/src/kernel/cmake_install.cmake:46 (file)
...

🤔 ismaelsadeeq reviewed a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2618143580)
Code Review e19bbc328236f64716034277857951184309cd14

It seems to me that this is designed with the flexibility to accommodate more levels, which explains the use of `std::vector` for `ClusterSet`

If yes then current approach is perfect else would it be better for `TxGraph` to maintain a `std::array` of two `ClusterSet? `main` and `staging and store their indexes in `TxGraph` as enums? This way, `Cluster` would only need to maintain that enum.

In `ClusterSet`, we could introduce a bool
...

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1956343988)
What do you mean by implicitly

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1956385389)
Should just refer to main and staging only instead of level-1, main, staging and top level?
It will be consistent this way.

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1956359892)
```suggestion
* does exist in main, the cluster it is in is unmodified in staging.
```

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1956400806)
hmm, above you mentioned "If it doesn't exist in main, it doesn't exist in staging either.".

And now there is a state where the transaction exits in staging but not in main

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1956469873)
```suggestion
Assume (level - MAX_LEVELS - 1);
auto& entry = m_entries[idx];
```

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1956442485)
```suggestion
int level = MAX_LEVELS - 1;

```

💬 ismaelsadeeq commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1956478391)
Should be set to avoid duplicates, will prevent de duplicating later on?

💬 glozow commented on pull request "p2p: improve TxOrphanage denial of service bounds and increase -maxorphantxs":
(https://github.com/bitcoin/bitcoin/pull/31829#discussion_r1956527816)
Hm, I don't want to manually change `-maxorphantxs`. But hey, this is the idea for introducing the test prior to increasing the default `-maxorphantxs`. At that point, the orphanage doesn't go past 100, so it's definitely doing evictions even with just 1010 + 101 orphans.

💬 achow101 commented on pull request "guix: Notarize MacOS app bundle and codesign all MacOS and Windows binaries":
(https://github.com/bitcoin/bitcoin/pull/31407#issuecomment-2659951439)
> If these all appear on bitcoincore.org/bin/ then it's starting to look a bit overwhelming for a MacOS (or Windows) user to correctly select what they need.

We used to upload only the binaries, but started to upload all of the guix output because the SHA256SUMS lists all of that. Previously, if you downloaded everything for a release and then tried to verify the SHA256SUMS, it would give you an error about missing files, and we thought that might be confusing.

I think it would be reasonab
...

🤔 stickies-v reviewed a pull request: "init: Take lock on blocks directory in BlockManager ctor"
(https://github.com/bitcoin/bitcoin/pull/31860#pullrequestreview-2617967456)
Concept ACK on:
- making LockDirectory more strict, it confused me recently that I was able to acquire a lock multiple times (within the same process)
- making BlockManager manage the lock instead of init

As per https://github.com/TheCharlatan/bitcoin/pull/25, I would be a fan of going further and making the `BlockManagerOptions` ctor take a `BlockDirLock` (or, as per my comment a `DirectoryLock`) to allow more granular error feedback, but this is already a step in the right direction regar
...

💬 stickies-v commented on pull request "init: Take lock on blocks directory in BlockManager ctor":
(https://github.com/bitcoin/bitcoin/pull/31860#discussion_r1956244596)
It seems like this class could easily be reused for chainstate, datadir, wallet, ... directories in future commits. Perhaps a `class DirLock` would make sense?

💬 stickies-v commented on pull request "init: Take lock on blocks directory in BlockManager ctor":
(https://github.com/bitcoin/bitcoin/pull/31860#discussion_r1956484013)
At this point, I don't see the benefit of this function anymore. Let's just inline it?

<details>
<summary>git diff on 436df349f0</summary>

```diff
diff --git a/src/init.cpp b/src/init.cpp
index 579669712e..3a984488af 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -1107,12 +1107,6 @@ static bool LockDirectory(const fs::path& dir, bool probeOnly)
} // no default case, so the compiler can warn about missing cases
assert(false);
}
-static bool LockDirectories(bool probeOn
...

💬 stickies-v commented on pull request "init: Take lock on blocks directory in BlockManager ctor":
(https://github.com/bitcoin/bitcoin/pull/31860#discussion_r1956371573)
Copy ctor and assignment operator should probably be deleted?