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

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1818548647)
50c19dd21f6e437cd8e3d9047a89d793d097c5e5
Not sure it's worth exposing this external method if the only reason to have it is testing.

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1818553463)
50c19dd21f6e437cd8e3d9047a89d793d097c5e5

worth adding a comment on what would happen if there are 3 colliding transactions (probably fine but some text would be helpful) :)

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1818568903)
50c19dd21f6e437cd8e3d9047a89d793d097c5e5
could *this* be used to game collisions (for *wtxid* you have here)?

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1818583090)
392facfce1be6d0395abecc2e049ff39635bd1a5

you can technically feed it arbitrary transactions. I think the comment should say it, and then later say "we normally pass a list of parents to determine whether [... ... ...]".

💬 naumenkogs commented on pull request "p2p: Fill reconciliation sets (Erlay) attempt 2":
(https://github.com/bitcoin/bitcoin/pull/30116#discussion_r1818575030)
392facfce1be6d0395abecc2e049ff39635bd1a5

why do you think force relay should influence whether we reconcile or flood? The transaction is relayed in either case (the original meaning of force relay)