💬 l0rinc commented on pull request "doc: Fix description of byte order of hashes in ZMQ documentation":
(https://github.com/bitcoin/bitcoin/pull/31862#issuecomment-2659824080)
Concept ACK
(https://github.com/bitcoin/bitcoin/pull/31862#issuecomment-2659824080)
Concept ACK
💬 l0rinc commented on pull request "Update README.md":
(https://github.com/bitcoin/bitcoin/pull/31867#issuecomment-2659840197)
NACK - opinionated random changes
(https://github.com/bitcoin/bitcoin/pull/31867#issuecomment-2659840197)
NACK - opinionated random changes
✅ fanquake closed an issue: "CMake `CheckPIESupported` doesn't always work"
(https://github.com/bitcoin/bitcoin/issues/30771)
(https://github.com/bitcoin/bitcoin/issues/30771)
🚀 fanquake merged a pull request: "cmake: Add `CheckLinkerSupportsPIE` module"
(https://github.com/bitcoin/bitcoin/pull/31359)
(https://github.com/bitcoin/bitcoin/pull/31359)
💬 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
...
(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
(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.
(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
...
(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.
(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)
...
(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
...
(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
(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.
(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.
```
(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
(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];
```
(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;
```
(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?
(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.
(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.