💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1667979626)
Fixed.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1667980741)
From the perspective of a `DepGraph`, transactions are identified by their position in the entry vector (which matches their position in the `Cluster` they came from). `DepGraph`'s equality tests whether every transaction in every position matches; there is nothing else it knows about.

I've added a comment to `DepGraph::operator==` to indicate it's primarily for testing.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1667980882)
Added an `Assume` for that.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1667980947)
Fixed.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1667981357)
I've pushed this simplification; it works a lot different now, but `CanAddDependency`, `GetReducedParents`, and `GetReducedChildren` are gone.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1667981485)
It's gone.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1667981720)
With the format change, I've documented every byte of the encoding in the tests here.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1667981849)
Good catch, fixed.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1667981943)
Fixed.

💬 sipa commented on pull request "cluster mempool: cluster linearization algorithm":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1667982100)
Added a comment.

💬 maflcko commented on pull request "wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs":
(https://github.com/bitcoin/bitcoin/pull/24128#issuecomment-2213093829)
rebased

💬 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.