💬 maflcko commented on pull request "refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options":
(https://github.com/bitcoin/bitcoin/pull/30232#issuecomment-2213162280)
IIRC the kernel will ship with the mempool, so it'll need to ship with policy. In any case, my commit was just for mempool_limits. The same would have to be done for `kernel/mempool_options`.

💬 maflcko commented on pull request "refactor: Make uint256S(const char*) consteval":
(https://github.com/bitcoin/bitcoin/pull/30377#issuecomment-2213171483)
> Like 3 but implement `SetHex(const char* str)` by calling the `std::string_view` version.

I don't think `const char*` overloads will need to be provided when `string_view` exists. Seems fine to just have a single `sting_view` function (and call it a fix at the same time).

💬 maflcko commented on pull request "depends: build libevent with CMake":
(https://github.com/bitcoin/bitcoin/pull/29835#issuecomment-2213181605)
I worked around the OSS-Fuzz issue with https://github.com/google/oss-fuzz/pull/12152/commits/a346f5686d5f4ac2805b23389d9c4210bd9b5495 for now.

💬 maflcko commented on pull request "refactor: Use designated initializer in test/util/net.cpp":
(https://github.com/bitcoin/bitcoin/pull/30397#issuecomment-2213261630)
ACK e233ec036dc972a5847ab769ad22d418fd9404d1

Previously clang-tidy couldn't understand the named args, because of list-initialization. (Even if it did, catching them right in the any C++ compiler is always better than only in clang-tidy)

✅ maflcko closed an issue: "Slow sync "
(https://github.com/bitcoin/bitcoin/issues/30360)

💬 maflcko commented on issue "Slow sync ":
(https://github.com/bitcoin/bitcoin/issues/30360#issuecomment-2213281103)
Closing for now, due to lack of details, but please leave a comment if you get a chance.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668158370)
>> I think it is going to result in multiple mappings on the same external IPv6 address but on different ports.

> i don't think so? We only request one mapping per IPv6 address at most.

Afaik PCP always uses a pinhole, which does not involve assinging a (different) port.

We do end up with our node being announced multiple times with different IP addresses, which could cause some other node to connect to use twice. That can already happen when you have IPv4 and IPv6 (and Tor and I2P) ena
...

💬 S3RK commented on pull request "psbt: Check non witness utxo outpoint early":
(https://github.com/bitcoin/bitcoin/pull/29855#issuecomment-2213286137)
tACK 9e13ccc50eec9d2efe0f472e6d50dc822df70d84

Reviewed code and verified new test vectors. Reproduced a crash on an older version with the new test vector.

💬 Sjors commented on pull request "net: Replace libnatpmp with built-in PCP+NATPMP implementation":
(https://github.com/bitcoin/bitcoin/pull/30043#discussion_r1668161856)
Indeed. Maybe just add a comment that this is intentional.

💬 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