💬 ismaelsadeeq commented on pull request "BlockAssembler: return selected packages vsize and feerate":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1668196350)
This causes a fuzz [failure](https://cirrus-ci.com/task/4852900168466432) during the conversion of the size to feerate per KB because `CAmount` is `int64_t`.

So I reverted back and `static_cast` the `packageSize` to be `uint32_t`, it is safe to do so because we are certain that `nSizeWithAncestors` will always be positive in `BlockAssembler::addPackageTxs`.

---
It's already done https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/node/miner.cpp#L365

💬 maflcko commented on pull request "BlockAssembler: return selected packages vsize and feerate":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1668211525)
> This causes a fuzz [failure](https://cirrus-ci.com/task/4852900168466432) during the conversion of the size to feerate per KB because `CAmount` is `int64_t`.

The fuzz failure is because you also changed `MultiplicationOverflow` to `uint64_t`, which is wrong.

💬 maflcko commented on pull request "BlockAssembler: return selected packages vsize and feerate":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1668212847)
> So I reverted back and `static_cast` the `packageSize` to be `uint32_t`, it is safe to do so because we are certain that `nSizeWithAncestors` will always be positive in `BlockAssembler::addPackageTxs`.

Another thing to consider is that the package size is small enough to be representable in `uint32_t`.

💬 maflcko commented on pull request "Add clang-tidy check for thread_local vars":
(https://github.com/bitcoin/bitcoin/pull/30146#issuecomment-2213384181)
ACK 34c9cee380e7276cf3f85f2ce56a293e57afd676

💬 ismaelsadeeq commented on pull request "BlockAssembler: return selected packages vsize and feerate":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1668247568)
Thanks Yeah.
---
unrelated but I wonder why we are using this big data type to represent size, not only while working in this PR, there are size datype inconsistencies in several places which sometimes cause overflows during calculation.

Is it safe to instead represent a type alias for size just like it was done for `CAmount` in `consensus/amount.h`.
And prevent generating large values for size in the fuzz test?

Apart from the fuzz test size can never be greater than more than `4,000,
...

💬 ismaelsadeeq commented on pull request "BlockAssembler: return selected packages vsize and feerate":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1668255434)
> The fuzz failure is because you also changed `MultiplicationOverflow` to `uint64_t`, which is wrong.

Yeah, I did that because `MultiplicationOverflow` requires that the type of both parameters be the same.

💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1668259313)
```
// Create cluster D child ---> K
```

💬 dergoegge commented on pull request "net: fix race condition in self-connect detection":
(https://github.com/bitcoin/bitcoin/pull/30394#discussion_r1668290789)
I think I'd be nicer to not change this interface and I believe it would be trivial to just use `SendMessages`? i.e. push the version message to outbound peers at the start of `SendMessages` (if it hasn't been pushed already).

📝 maflcko opened a pull request: "tidy: modernize-use-equals-default"
(https://github.com/bitcoin/bitcoin/pull/30406)
Prior to C++20, `modernize-use-equals-default` could have been problematic because it could turn a non-aggregate into an aggregate. The risk would be that designated initializers would be enabled where the author did not intend to enable them.

With C++20, aggregate for those is forbidden either way. (https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1008r1.pdf)

So enabled it for code clarity, consistency, and possibly unlocking compiler optimizations. See https://clang.llvm.org/ext
...

💬 maflcko commented on pull request "BlockAssembler: return selected packages vsize and feerate":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1668302906)
> Apart from the fuzz test size can never be greater than `4,000,000`?

In blocks, yes. In the mempool, it may depend on the user's config.

💬 maflcko commented on pull request "Add clang-tidy check for thread_local vars":
(https://github.com/bitcoin/bitcoin/pull/30146#discussion_r1668305658)
> modernize-use-equals-default

See https://github.com/bitcoin/bitcoin/pull/30406

💬 maflcko commented on pull request "Use `WITH_LOCK` in `Warnings::Set`":
(https://github.com/bitcoin/bitcoin/pull/30404#issuecomment-2213518857)
lgtm ACK 6af51e819847e737449609daa214e16f9453e85d

💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1668292640)
There are 9 such assertion blocks with almost similar validations on ancestors and descendants.
If the author feels it's worth it, IMO this duplication can be eliminated by accepting few variables as the input in a function and calling it with different arguments.
```
validateTxRelations(txToValidate, ancestorTxs, descendantTxs)
```

👍 rkrux approved a pull request: "Fee Estimation: Ignore all transactions that are CPFP'd"
(https://github.com/bitcoin/bitcoin/pull/30079#pullrequestreview-2162718838)
tACK [cb95f95](https://github.com/bitcoin/bitcoin/pull/30079/commits/cb95f95907cf9fce3a18f2bbbb059e72a897e34e)

`make, test/functional` are successful.

Although, this round of review was focussed on [5e6ff0f](https://github.com/bitcoin/bitcoin/pull/30079/commits/5e6ff0f80e2ed7daadb3abbd5a773b542b9b0f6f)

💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1668268758)
```
// Create cluster D child ---> K
```

💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1668306440)
Interesting that `tx_set` is passed by value here and 2 separate copies are made at the time of calling, which is the need here for separate ancestors and descendants copies.

💬 rkrux commented on pull request "Fee Estimation: Ignore all transactions that are CPFP'd":
(https://github.com/bitcoin/bitcoin/pull/30079#discussion_r1668295683)
+1 on separating this function out in a file and adding focussed unit tests for it, I feel this is generic enough to be used in several places later.

🤔 glozow reviewed a pull request: "kernel: De-globalize validation caches"
(https://github.com/bitcoin/bitcoin/pull/30141#pullrequestreview-2162794247)
reACK 606a7ab

💬 ismaelsadeeq commented on pull request "BlockAssembler: return selected packages vsize and feerate":
(https://github.com/bitcoin/bitcoin/pull/30391#discussion_r1668354967)
thanks the commit is gone now.

🤔 glozow reviewed a pull request: "Use `WITH_LOCK` in `Warnings::Set`"
(https://github.com/bitcoin/bitcoin/pull/30404#pullrequestreview-2162862078)
ACK 6af51e819847e737449609daa214e16f9453e85d

Not familiar with this code at all, but it looks like the handler function `handleNotifyAlertChanged` calls `getStatusBarWarnings`
https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/qt/clientmodel.cpp#L256-L260
which calls `Node::getWarnings()`
https://github.com/bitcoin/bitcoin/blob/bd5d1688b4311e21c0e0ff89a3ae02ef7d0543b8/src/qt/clientmodel.cpp#L164-L167
which calls `GetMessages()`
https://github.com/bitcoi
...

💬 paplorinc commented on pull request "Don't empty dbcache on prune flushes: >30% faster IBD":
(https://github.com/bitcoin/bitcoin/pull/28280#discussion_r1668366154)
this should probably be moved to a [later commit](https://github.com/bitcoin/bitcoin/pull/28280/commits/92aa98712c604b3e424671b196b8425fabdf74d0#diff-095ce1081a930998a10b37358fae5499ac47f8cb6f25f5df5d88e920a54e0341R189) where you introduce the `Next()` method