💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1818172329)
No longer relevant after rewriting this function.

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1818173833)
I reworked your suggestion to capture the caching idea, and now invoke this function in `Apply`. However, there's still an ugly special case in `Apply()` in that we can only used a cached ancestor set calculation for the first transaction in a package (because the calculation is no longer correct once transactions start getting added to the mempool).

Note that in the cluster mempool implementation, this ugliness in `Apply()` goes away, because we will no longer need to have ancestor sets cal
...

👋 l0rinc's pull request is ready for review: "optimization: change XOR obfuscation key from `std::vector<std::byte>(8)` to `uint64_t`"
(https://github.com/bitcoin/bitcoin/pull/31144)

⚠️ Bahramgho opened an issue: "BTC"
(https://github.com/bitcoin/bitcoin/issues/31170)
1A1zP1eP5QGefi2DMPTfTL5SLmv7DivfNa

✅ pinheadmz closed an issue: "BTC"
(https://github.com/bitcoin/bitcoin/issues/31170)

💬 jarolrod commented on pull request "Drop miniupnp dependency":
(https://github.com/bitcoin/bitcoin/pull/31130#issuecomment-2440499252)
ACK https://github.com/bitcoin/bitcoin/commit/40e5f26a3ff77e50df808f6f850c617aec2df203

Tested GUI changes, and GUI changes are sufficient. Error message appears in GUI as well when moving from master with UPNP having been set in the gui, then to this PR branch with same settings.json file.

💬 andremralves commented on pull request "contrib: skip missing binaries in gen-manpages":
(https://github.com/bitcoin/bitcoin/pull/30986#issuecomment-2440525890)
I probably don’t have enough understanding of the release process to know exactly how this change could affect it, but I assume that having gen-manpages.py exit with an error is important somewhere during the process.

I implemented your suggestion by adding the --skip-missing-binaries option to the command.

Thanks for the feedback! What do you think of it now?

💬 stratospher commented on pull request "net: option to disallow v1 connection on ipv4 and ipv6 peers":
(https://github.com/bitcoin/bitcoin/pull/30951#issuecomment-2440608399)
I've rebased and changed the approach in the PR to have v2 only outbound connections. So there's null partition risk because of the option now.

> Right. Would it maybe make sense to have an option to restrict transaction broadcast and relay to v2-only, for IPv4 and IPv6 connections? Instead of not connect at all? Eg consider v1 connections blocks-only (except those over Tor/I2P). This alleviates the risk of network partition of consensus. While still providing a bit of added privacy for where
...

💬 maflcko commented on issue "Stop at header":
(https://github.com/bitcoin/bitcoin/issues/31162#issuecomment-2440755070)
Does the hidden debug-only `-minimumchainwork=0` not work for this use case?

:lock: fanquake locked an issue: "BTC"
(https://github.com/bitcoin/bitcoin/issues/31170)

💬 maflcko commented on pull request "test: Don't enforce BIP94 on regtest unless specified by arg":
(https://github.com/bitcoin/bitcoin/pull/31156#discussion_r1818507761)
nit: Why add a new option at all? I wonder if using the existing `-test=<option>` hook would work.

💬 maflcko commented on pull request "guix: Use DOS newlines for SHA256SUMS files":
(https://github.com/bitcoin/bitcoin/pull/29147#issuecomment-2440840378)
Closing for now due to lack of progress. Leave a comment if you want this reopened.

✅ maflcko closed a pull request: "guix: Use DOS newlines for SHA256SUMS files"
(https://github.com/bitcoin/bitcoin/pull/29147)

💬 Muaytie23 commented on issue "MIN_STANDARD_TX_NONWITNESS_SIZE prevents efficient spending of P2A outputs":
(https://github.com/bitcoin/bitcoin/issues/31155#issuecomment-2440891137)
[{"inputs":[{"internalType":"address","name":"_SY","type":"address"},{"internalType":"string","name":"_name","type":"string"},{"internalType":"string","name":"_symbol","type":"string"},{"internalType":"uint8","name":"__decimals","type":"uint8"},{"internalType":"uint256","name":"_expiry","type":"uint256"}],"stateMutability":"nonpayable","type":"constructor"},{"inputs":[],"name":"InvalidShortString","type":"error"},{"inputs":[],"name":"OnlyYCFactory","type":"error"},{"inputs":[],"name":"OnlyYT","t
...

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1818596465)
I think the issue is if we `quit_early`; I recall the fuzzer hitting the new `Assume()` calls via `LimitMempoolSize()` at the line just below, before I added this.

💬 sdaftuar commented on pull request "cluster mempool: Implement changeset interface for mempool":
(https://github.com/bitcoin/bitcoin/pull/31122#discussion_r1818600499)
Update: I realized that at least for now, there is a requirement that any added transactions are added in a topological order, with parents coming before children -- this is so that we can add things to the mempool's multi_index at `Apply()` time without creating any missing dependencies. But removals can happen in any order and can be interleaved with the additions.

💬 maflcko commented on pull request "bench: Remove various extraneous benchmarks":
(https://github.com/bitcoin/bitcoin/pull/31153#issuecomment-2440916567)
Hex parsing is used in mining via `DecodeHexBlk`, no?

Also, while some of the benchmarks do not exist to be optimized, but merely to give a feeling and to avoid a regression where an algorithm is swapped to go from `O(n)` to `O(n^2)`. So in a sense, they are a bit of a unit test.



In any case, updating the docs should be uncontroversial and useful going forward.

💬 maflcko commented on pull request "Introduce `g_fuzzing` global for fuzzing checks":
(https://github.com/bitcoin/bitcoin/pull/31093#issuecomment-2440960117)
review ACK 9f243cd7fa6654e3b71ba6bff82cceed547c5d53 🗜

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

Signature:

```
untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK 9f243cd7fa66
...

💬 maflcko commented on pull request "bench: add support for custom data directory":
(https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2441018301)
Looks like that fixed it. However, it would be good to understand *why* it fixed it. Looking at the diff that triggered it, I fail to see how (https://github.com/bitcoin/bitcoin/pull/31000#issuecomment-2437357680)

💬 maflcko commented on pull request "Windows bitcoind stall debugging [NOMERGE, DRAFT]":
(https://github.com/bitcoin/bitcoin/pull/30956#issuecomment-2441027190)
Can this be closed, or is it waiting on something?