📝 instagibbs opened a pull request: "test: fix test_limit_enforcement_package"
(https://github.com/bitcoin/bitcoin/pull/34001)
The current test has a couple issues:

1) the parent_tx_good is regenerating the exact same transaction that is already in the cluster, so it's resulting in no replacements on submission
2) once fixed, the additional fee needs to be allocated to the parent transaction in the package, not the child. If the RBF fees are allocated to the child, this triggers the package RBF logic, which requires no in-mempool ancestors to be present.

Fix the bug and add a few assertions to protect against reg
...

💬 instagibbs commented on pull request "test: fix test_limit_enforcement_package":
(https://github.com/bitcoin/bitcoin/pull/34001#issuecomment-3608128047)
cc @sdaftuar @glozow

🤔 w0xlt reviewed a pull request: "doc: improvements to doc/descriptors.md"
(https://github.com/bitcoin/bitcoin/pull/33986#pullrequestreview-3536298711)
ACK https://github.com/bitcoin/bitcoin/pull/33986/commits/fc33626f66e7f93f39989fbbabfbf00222762071

💬 rkrux commented on pull request "refactor: replace manual promise with SyncWithValidationInterfaceQueue":
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2586156020)
Sorry for the late reply. I don't fully understand this comment though: https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2577151438

I suggested to remove this variable because the `if` condition below could be managed by the `wait_callback` and `node.validation_signals` properties directly and specially setting the `callback_set` variable in an `if` block above that doesn't do anything else now seemed unnecessary to me.

👍 ryanofsky approved a pull request: "mining: fix -blockreservedweight shadows IPC option"
(https://github.com/bitcoin/bitcoin/pull/33965#pullrequestreview-3536257630)
Code review ACK 4d686142e079765cd31851481deb70659a9e9376. Just updated a comment since last review. Still looks good! Only minor comments below, not important

💬 ryanofsky commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2586162219)
In commit "mining: fix -blockreservedweight shadows IPC option" (4d686142e079765cd31851481deb70659a9e9376)

Use MAX_BLOCK_WEIGHT constant?

💬 ryanofsky commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2586171036)
In commit "mining: fix -blockreservedweight shadows IPC option" (4d686142e079765cd31851481deb70659a9e9376)

Note: I guess if it did timeout you would get a confusing server error about trying to call a method on a null capability, so this error should be clearer

💬 ryanofsky commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2586089447)
re: https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2582425482

Yes that makes sense. I completely agree `MiningArgs` in #33966 should not use `std::optional` and that PR is correctly applying `DEFAULT_BLOCK_RESERVED_WEIGHT` in one place at the `ArgsMan` level, not the mining interface level.

I just don't think the implementation of this PR should be muddled in anticipation of #33966 before `MiningArgs` is introduced. In this PR, it makes sense to apply `DEFAULT_BLOCK_RESERVED_WEI
...

💬 ryanofsky commented on pull request "mining: fix -blockreservedweight shadows IPC option":
(https://github.com/bitcoin/bitcoin/pull/33965#discussion_r2586111251)
In commit "mining: fix -blockreservedweight shadows IPC option" (4d686142e079765cd31851481deb70659a9e9376)

Maybe update the comment to say this is being set to a high value to confirm that the -blockreservedweight option has no effect (IIUC). Current comment raises question of why the test is going out of its way to set an option that has no effect.

💬 fanquake commented on pull request "guix: use GCC 14.3.0 over 13.3.0":
(https://github.com/bitcoin/bitcoin/pull/33775#issuecomment-3608239216)
The non-determinism also goes away if you build with `_FORTIFY_SOURCE=2` instead of `_FORTIFY_SOURCE=3`. The difference there should be that the headers are using `__builtin_dynamic_object_size` in the `=3` case, instead of `__builtin_object_size`.

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2586259538)
Looks good, thanks!
https://github.com/bitcoin/bitcoin/compare/f07c765aa6bdba2511ceec56aa7f9755fa29a81e..d2f4bccbf70031b84b6af1a6c8480c4b3071bfd7

💬 romanz commented on pull request "rest: allow reading partial block data from storage":
(https://github.com/bitcoin/bitcoin/pull/33657#discussion_r2586262733)
Thanks!
https://github.com/bitcoin/bitcoin/compare/f07c765aa6bdba2511ceec56aa7f9755fa29a81e..d2f4bccbf70031b84b6af1a6c8480c4b3071bfd7

💬 0xB10C commented on pull request "p2p: reduce false-positives in addr rate-limiting":
(https://github.com/bitcoin/bitcoin/pull/33699#issuecomment-3608377779)
> Ignoring self-announcements seems to occur only in the time between our inbound peer (outbound from their perspective) 1) calling `SetupAddressRelay` when receiving version, and 2) receiving verack where it will set `fSuccessfullyConnected = true` and call `SendMessages` followed by `MaybeSendAddr`. The address entries are sent in order of insert, but then shuffled before processing since #22387 ([see rationale](https://github.com/bitcoin/bitcoin/pull/22387#discussion_r664238361)). If they wer
...

🤔 marcofleon reviewed a pull request: "Replace cluster linearization algorithm with SFL"
(https://github.com/bitcoin/bitcoin/pull/32545#pullrequestreview-3536407029)
Did a first pass of the changes in `cluster_linearize.h`, left a couple small comments. I'm running a few of the fuzz targets, including the new one, and I'll leave those going for a while.

Still need to look at the tests thoroughly. Let me know if there's anything specfic you think reviewers can do that would be useful.

💬 marcofleon commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2586214542)
```suggestion
SetInfo operator-(const SetInfo& other) const noexcept
```

💬 marcofleon commented on pull request "Replace cluster linearization algorithm with SFL":
(https://github.com/bitcoin/bitcoin/pull/32545#discussion_r2586249205)
nit: Is there a reason we recalculate here vs just using `chunk_rep` from the line above?

Also additional nit: The local `chunk_rep` is named the same as the function parameter and both are `TxIdx` I believe. Could be worth having different names?

👍 hodlinator approved a pull request: "rest: allow reading partial block data from storage"
(https://github.com/bitcoin/bitcoin/pull/33657#pullrequestreview-3536580365)
re-ACK d2f4bccbf70031b84b6af1a6c8480c4b3071bfd7

💬 maflcko commented on pull request "refactor: replace manual promise with SyncWithValidationInterfaceQueue":
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2586340053)
> in an `if` block above that doesn't do anything else now seemed unnecessary to me.

What the if-else block does is explained right after the `if`:

```cpp
if (auto mempool_tx = node.mempool->get(txid); mempool_tx) {
// There's already a transaction in the mempool with this txid.

💬 rkrux commented on pull request "refactor: replace manual promise with SyncWithValidationInterfaceQueue":
(https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2586371586)
Ah, there's another `if` above on line 66: https://github.com/bitcoin/bitcoin/blob/9a29b2d331eed5b4cbd6922f63e397b68ff12447/src/node/transaction.cpp#L66

I had missed that, good catch. I see now what you mean.

Setting `callback_set` in the `else` block seems reasonable. This suggestion can be reverted: https://github.com/bitcoin/bitcoin/pull/33962#discussion_r2571218739.

💬 marcofleon commented on pull request "http: replace WorkQueue and single threads handling for ThreadPool":
(https://github.com/bitcoin/bitcoin/pull/33689#issuecomment-3608532244)
> I don’t think this is an issue. You’re just massively oversubscribing the CPU and lowering the timeout to the point where all the context switching triggers it. Switching to notify_all() only forces all workers awake on every submission, which masks the OS scheduler starvation you get in this kind of extreme setup.

I've been playing with the fuzz test and found that this fixes it for me:

```c++
ThreadPool g_pool{"fuzz"};
size_t g_num_workers = 3;
std::atomic<bool> g_pool_started{false
...