💬 vasild commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2359525565)
I dropped the changes to the parameters since they are not the aim of this PR and they caused disproportional amount of discussion.

💬 vasild commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2359527018)
Dropped altogether.

💬 Eunovo commented on pull request "Silent Payments: Receiving":
(https://github.com/bitcoin/bitcoin/pull/32966#issuecomment-3307718331)
I changed `CKey::TweakAdd()` to `CKey::ComputeBIP352Key()`

💬 purpleKarrot commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#issuecomment-3307759393)
Unrelated to the PR, but it is error prone that `DebugLogHelper` is copyable: https://godbolt.org/z/YdT5EWzMM

I suggest to augment the PR with a commit that contains:
```cpp
DebugLogHelper(const DebugLogHelper&) = delete;
DebugLogHelper& operator=(const DebugLogHelper&) = delete;
```

💬 sdaftuar commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2359615241)
Happy to clear this comment up to get rid of double-negatives, but just to make sure I've got this right:
* AcceptSubPackage() will, in the case of single transaction acceptance, enable sibling eviction, right?
* And in package validation, sibling eviction is disabled?

💬 vasild commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#issuecomment-3307790546)
@purpleKarrot,
> > Throwing an exception from the destructor of a class is a bad practice because the destructor will be called when an object of that type is alive on the stack and another exception is thrown, which will result in "exception during the exception". This would terminate the program without any messages.

> This is not precisely what happens. If an exception is emitted from a function marked `noexcept(false)`, the current terminate handler will be invoked.

Hmm, not what I ob
...

💬 vasild commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#issuecomment-3307822493)
@purpleKarrot
> I suggest to augment the PR with a commit that contains...

Done

💬 instagibbs commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2359689084)
correct

💬 purpleKarrot commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#issuecomment-3307930444)
> If an exception is emitted from a function marked `noexcept(false)`, the current terminate handler will be invoked.

My bad. That is the behavior for `noexcept(true)`, of course. I agree that marking a destructor with `noexcept(false)` is bad.

💬 sdaftuar commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2359723768)
Yeah the limits that were previously enforced by earlier versions of this function would be calculated including the tx itself, but not the ancestor set. Thanks for catching.

💬 purpleKarrot commented on pull request "test: don't throw from the destructor of DebugLogHelper":
(https://github.com/bitcoin/bitcoin/pull/33388#discussion_r2359730526)
The original error had the message string in quotes. I think this should be preserved because it is easier to debug when the string contains whitespace at the end.

💬 dergoegge commented on pull request "RPC: Return `permitbaremultisig` and `maxdatacarriersize` in `getmempoolinfo`":
(https://github.com/bitcoin/bitcoin/pull/29954#issuecomment-3307965154)
Added this to the release notes: https://github.com/bitcoin-core/bitcoin-devwiki/wiki/v30.0-Release-Notes-Draft

💬 sdaftuar commented on pull request "Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r2359774840)
I don't think we need the cpu protection now that we're using txgraph to do things (versus all the pointer chasing that would happen before), and only left the 500 size limit to preserve existing behavior. But I can move this back into the for-loop if people have concerns.

I believe that after cluster mempool, mini miner can be reworked to eliminate this function altogether. With the disclaimer that I haven't thought about it in a while, I put together a branch demonstrating this a while
...

👋 fanquake's pull request is ready for review: "[27.x] Backports"
(https://github.com/bitcoin/bitcoin/pull/33416)

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2359949673)
Thanks, done.

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2359951971)
Ah nice catch, I completely missed this. Done.

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#discussion_r2359953034)
Thanks, I believe I've fixed all of the camel case.

💬 b-l-u-e commented on pull request "net: Fix Discover() not running when using -bind=0.0.0.0:port":
(https://github.com/bitcoin/bitcoin/pull/32757#discussion_r2359990114)
Thanks for the feedback I will make changes

💬 john-moffett commented on pull request "rpc: Always return per-wtxid entries in submitpackage tx-results":
(https://github.com/bitcoin/bitcoin/pull/33427#issuecomment-3308365732)
> I thought "unknown-not-validated" would be better since it matches the error string for skipped transactions, but I think it's better to have separate messages for "nothing was validated because the package was ill-formed" vs "this particular transaction wasn't validated."

Right now the code can only ever return the former, as both the RPC [code](https://github.com/bitcoin/bitcoin/blob/74fa028da1ea38c5348f988464074899684cebcf/src/rpc/mempool.cpp#L1054) and the actual [acceptance code](https
...

💬 Crypt-iQ commented on pull request "fuzz: compact block harness":
(https://github.com/bitcoin/bitcoin/pull/33300#issuecomment-3308381089)
The latest push adds two commits:
- a5619ea631bd8b93b4ef02a20abb8c1c0705d8e4 "test: add setup_validation_interface_no_scheduler to TestOpts"
- Disables the scheduler completely if set. This is needed because this harness creates a `TestingSetup` inside the fuzz test and scheduling a [promise](https://github.com/bitcoin/bitcoin/blob/74fa028da1ea38c5348f988464074899684cebcf/src/test/util/setup_common.cpp#L249) can be non-deterministic as the scheduler's `serviceQueue` may start with a non-empt
...