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

💬 Sjors commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1399137900)
0b284b5fd29c06d46c1ec60ea7e1bcd07f36feb1: it would be more practical to (also) have a lookup by transaction hash

⚠️ dergoegge opened an issue: "fuzz, parse_iso8601: attempt to dereference an end-of-stream istreambuf_iterator"
(https://github.com/bitcoin/bitcoin/issues/28917)
Ran into this crash on my own infra, not sure why oss-fuzz doesn't find it.

```
$ echo "MjIyMw0NDQ0NDQ0NDQ0NDQ0NDcIn" | base64 --decode > parse_iso8601-46463936b8a32173e167a89aad1ddc9a81f24bef.crash
$ FUZZ=parse_iso8601 ./src/test/fuzz/fuzz parse_iso8601-46463936b8a32173e167a89aad1ddc9a81f24bef.crash
INFO: Running with entropic power schedule (0xFF, 100).
INFO: Seed: 3937133750
INFO: Loaded 1 modules (568922 inline 8-bit counters): 568922 [0x5624a4a983f0, 0x5624a4b2324a),
INFO: Loade
...

💬 sipa commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1399160874)
To check `a/b > c/d` you can instead check `a*d > c*b`, which avoids divisions (which are an order of magnitude slower than multiplication).

💬 sipa commented on pull request "[WIP] Cluster mempool implementation":
(https://github.com/bitcoin/bitcoin/pull/28676#discussion_r1399161977)
I believe that's an idea we've toyed with (calling it "sibling eviction"), but so far it isn't clear how to align that with DoS prevention.

💬 hebasto commented on pull request "[26.x] Final Changes":
(https://github.com/bitcoin/bitcoin/pull/28872#discussion_r1399162674)
I'm sorry for that late comment. Mind adding the following stuff:
```
- The transaction list in the GUI to no longer provide a special category for “payment to yourself”. Now transactions that have both inputs and outputs that affect the wallet are displayed on separate lines for spending and receiving. (gui#119)

- A new menu option allows migrating a legacy wallet based on keys and implied output script types stored in BerkeleyDB (BDB) to a modern wallet that uses descriptors stored in SQL
...

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1399162733)
Fixed

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1399163164)
I updated the commit message 👍🏾

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1399163551)
Updated with your suggestion

💬 ismaelsadeeq commented on pull request "Fee Estimator updates from Validation Interface/CScheduler thread":
(https://github.com/bitcoin/bitcoin/pull/28368#discussion_r1399165583)
This will change the input of the `CBlockPolicyEstimator` fuzzing test, I added a docstring `/*inBlock*/`.