💬 martinus commented on pull request "coins: make sure PoolAllocator uses the correct alignment":
(https://github.com/bitcoin/bitcoin/pull/28913#issuecomment-1818769524)
@pinheadmz I don't think there's a need to test any any RPC calls, if sync doesn't go OOM it means it is now working :)

💬 TheCharlatan commented on pull request "lint: Report all lint errors instead of early exit":
(https://github.com/bitcoin/bitcoin/pull/28862#discussion_r1398979031)
It's a bit jarring to see `Err("")`, but I guess there is little point in adding a parser for the script output, or duplicating the failure reporting, so this can just be improved if/when the lint checks are converted to Rust.

💬 maflcko commented on pull request "util: Replace std::filesystem with util/fs.h":
(https://github.com/bitcoin/bitcoin/pull/28076#issuecomment-1818774328)
> relying on CIs

This pull request does not change how much CI is relied upon. Previously, CI was running on all patches, and it is still after this pull request. Previously, it was possible to run any CI task locally, and it is still possible, in the same way. I am running the CI locally regularly and it obviously helps if others do the same. If you have issues running anything locally, it would be good to open an issue, so that it can be fixed. When filing an issue, adding context which doc
...

👍 TheCharlatan approved a pull request: "lint: Report all lint errors instead of early exit"
(https://github.com/bitcoin/bitcoin/pull/28862#pullrequestreview-1739454098)
lgtm ACK fa01f884d3ac128f09bfa57ac2648a19a19d854e

I checked that the CI can now correctly report multiple lint failures.

💬 maflcko commented on pull request "lint: Report all lint errors instead of early exit":
(https://github.com/bitcoin/bitcoin/pull/28862#discussion_r1398984491)
yeah, my primary goal is to improve the experience when running the lint stuff (both locally and on CI). Anything else can be discussed and tackled in other threads, if there is a need and interest.

💬 maflcko commented on pull request "refactor: Make CTxMemPoolEntry non-copyable":
(https://github.com/bitcoin/bitcoin/pull/28903#issuecomment-1818789234)
Needs the title adjusted, according to the commit title?

💬 maflcko commented on pull request "refactor: Make CTxMemPoolEntry only explicitly copyable":
(https://github.com/bitcoin/bitcoin/pull/28903#issuecomment-1818809689)
Would be nice to completely remove the need to create copies, but this involves a lot more code changes. This small fixup makes code and the mentioned code changes easier to review, so further improvements can be done in the future, if there is need and interest.

ACK 705e3f1de00bf30d728addd52a790a139d948e32 🌯

<details><summary>Show signature</summary>

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_fi
...

💬 maflcko commented on pull request "refactor: Make CTxMemPoolEntry only explicitly copyable":
(https://github.com/bitcoin/bitcoin/pull/28903#issuecomment-1818810741)
For reference, the move constructor is already deleted, according to a compiler:

```
./kernel/mempool_entry.h:188:27: note: move constructor of 'CTxMemPoolEntry' is implicitly deleted because field 'm_epoch_marker' has a deleted move constructor

💬 TheCharlatan commented on pull request "refactor: Make CTxMemPoolEntry only explicitly copyable":
(https://github.com/bitcoin/bitcoin/pull/28903#issuecomment-1818847461)
Thanks for the review @maflcko

Re https://github.com/bitcoin/bitcoin/pull/28903#issuecomment-1818810741

> For reference, the move constructor is already deleted, according to a compiler:

Removed the explicit move constructor deletion again.

💬 maflcko commented on pull request "fuzz: add target for `DescriptorScriptPubKeyMan`":
(https://github.com/bitcoin/bitcoin/pull/28578#issuecomment-1818847784)
I think this can go ahead without having to wait on the unrelated and separately handled pull request to fix a timeout in a function that is called in this target? Seems odd to block this, if it is useful. Better to have some fuzz inputs time out than to have no fuzz coverage?

💬 maflcko commented on pull request "refactor: Make CTxMemPoolEntry only explicitly copyable":
(https://github.com/bitcoin/bitcoin/pull/28903#issuecomment-1818857039)
Ah sorry, didn't mean it as a feedback to change the commit. It seems fine to keep it explicitly deleted, because a move is no better than a copy when either results in a runtime error. Having it deleted also prevents it form accidentally appearing again.

💬 TheCharlatan commented on pull request "refactor: Make CTxMemPoolEntry only explicitly copyable":
(https://github.com/bitcoin/bitcoin/pull/28903#issuecomment-1818861865)
Reverted the last change again.

💬 maflcko commented on pull request "Use serialization parameters for CTransaction":
(https://github.com/bitcoin/bitcoin/pull/28438#issuecomment-1818877296)
> SpanReader

Done in #28912. I guess the `CDataStream` one can be done as part of #https://github.com/bitcoin/bitcoin/pull/28451

💬 maflcko commented on pull request "mempool / rpc: followup to getprioritisedtransactions and delete a mapDeltas entry when delta==0":
(https://github.com/bitcoin/bitcoin/pull/28885#discussion_r1399066691)
I don't understand the firs commit message:

```
test: Directly constructing 2 entry map for getprioritisedtransactions

Directly constructing the map so that we wouldn't miss extraneous
entries in future regressions.
```

What does it mean and why is the code changed? Can you give an example or explanation?

💬 ajtowns commented on pull request "refactor: VectorWriter and SpanReader without nVersion":
(https://github.com/bitcoin/bitcoin/pull/28912#issuecomment-1818885200)
Shares its first commit with #28892

💬 ajtowns commented on pull request "refactor: P2P transport without serialize version and type":
(https://github.com/bitcoin/bitcoin/pull/28892#discussion_r1399077883)
Could give it a distinct name like `void MakeAndPushMessage(node, msg_type, args)` to make that clear, I guess?

💬 Sjors commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1399053865)
065e18fff30a91c94c47d5c2fc65b13ddc38aa47: do you plan to relax this assumption so that a transaction in (a) full cluster(s) can be replaced?

👍 ryanofsky approved a pull request: "depends: fix libmultiprocess build on aarch64"
(https://github.com/bitcoin/bitcoin/pull/28846#pullrequestreview-1739571746)
Code review ACK c3a962be10a98192a93518b0222c21fc7f115404. These changes seems safe, and the first commit seems like a clear bugfix. But I did have some minor suggestions to improve the first commit, and some questions about the second commit below.

💬 ryanofsky commented on pull request "depends: fix libmultiprocess build on aarch64":
(https://github.com/bitcoin/bitcoin/pull/28846#discussion_r1399057303)
In commit "depends: always install libmultiprocess to /lib" (156366f10aa38c612e26a1f93d8503786f3d3a34)

Thanks for this fix. It seems like a regression after https://github.com/chaincodelabs/libmultiprocess/pull/79#discussion_r1399049836

Would suggest a comment here like "# Hardcode library install path to "lib" to be compatible with the PKG_CONFIG_PATH setting in depends/config.site.in which also hardcodes "lib". Without this setting, cmake by default would use the OS library directory, w
...

💬 ryanofsky commented on pull request "depends: fix libmultiprocess build on aarch64":
(https://github.com/bitcoin/bitcoin/pull/28846#discussion_r1399123100)
In commit "depends: build libmultiprocess with position independant code" (c3a962be10a98192a93518b0222c21fc7f115404)

I don't think I really understand what's going on here, but this seems like a reasonable change given that `--with-pic` seems to be used on so many other depends builds [^1]. I guess it is a little unclear why in the other depends builds, the `--with-pic` option seems to be selectively applied for individual platforms like linux, freebsd, netbsd, openbsd (and never darwin), whi
...