💬 maflcko commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2021302172)
thx, done

💬 maflcko commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2021302250)
thx, pushed something similar with emojis

💬 maflcko commented on pull request "fuzz: Make partially_downloaded_block more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32158#discussion_r2021302350)
thx, pushed something similar

💬 laanwj commented on pull request "torcontrol: Define tor reply code as const to improve our maintainability":
(https://github.com/bitcoin/bitcoin/pull/32166#discussion_r2021344127)
i considered commenting this, but didn't because it's a bit much for defining two response values. Seems the benefit of enum class would be to make sure they're grouped by name `TorReply::OK` `TorReply::UNRECOGNIZED`?

📝 brunoerg opened a pull request: "fuzz: doc: add info about `afl-system-config` for macOS"
(https://github.com/bitcoin/bitcoin/pull/32175)
`afl-system-config` adjusts the shared memory segment size limits and configures kernel parameters for better fuzzing performance. Since macOS has more conservative values on shared memory, it's necessary to run `afl-system-config`, or manually adjust the values to fuzz with AFL++.

e.g.:
```sh
kern.sysv.shmmax: 524288000
kern.sysv.shmmin: 1
kern.sysv.shmseg: 48
kern.sysv.shmall: 131072000
```

📝 laanwj opened a pull request: "net: Prevent accidental circuit sharing when using Tor stream isolation"
(https://github.com/bitcoin/bitcoin/pull/32176)
Add a class TorsStreamIsolationCredentialsGenerator that generates
unique credentials based on a randomly generated session prefix
and an atomic counter.

This makes sure that different launches of the application won't share
the same credentials, and thus circuits, even in edge cases.

Example with `-debug=proxy`:
```
2025-03-31T15:51:20Z [proxy] SOCKS5 sending proxy authentication d91418ab1b2fc8d7b672a6eb3ac96f8f-0:d91418ab1b2fc8d7b672a6eb3ac96f8f-0
2025-03-31T15:51:21Z [proxy] SOCKS
...

💬 darosior commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-2766799625)
Concept ACK.

I don't buy that the discontinuity would cause any more confusion than making harmless transactions non-standard. The upgrade hook argument made sense back when this PR was closed, but i don't think it holds anymore now that no proposal includes invalidating <64 bytes transactions.

> OTOH, it might be plausible to just not worry about small-tx's at all, in which case the check could just be removed entirely, avoiding any discontinuities...

This goes completely against your
...

💬 glozow commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r2021216973)
concept ack introducing this constant. I think it would be best to have it in a separate commit and replace the magic number in policy.cpp with this

💬 glozow commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r2021218605)
Should use `CURRENT_VERSION` instead of magic number

💬 glozow commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r2021201880)
Yes, this seems to be redundant with `TX_MAX_STANDARD_VERSION`. It also appears to be unused, did you maybe forget to delete it?

💬 glozow commented on pull request "rpc: Support v3 raw transactions creation":
(https://github.com/bitcoin/bitcoin/pull/31936#discussion_r2021361765)
This is needed as the arg is non-string type. My understanding is that @luke-jr is concept nacking the PR. I don't really understand the comments about the positionalness of this arg?

📝 instagibbs opened a pull request: "TxGraph: Increase fuzz coverage"
(https://github.com/bitcoin/bitcoin/pull/32177)
Was looking at my local coverage report, and noticed a few spots that will not or cannot be hit.

CountDistinctClusters, GetAncestorsUnion, and GetDescendantsUnion accept nullptrs, but the test harness never employs them. Disallow them.

We never call PullIn whenever there isn't staging, so just enforce that invariant via assertion.

Remaining places that are not covered:

1) Relinearize: Currently we seem to always start with a cold (not known to be optimal) cluster, and after one attem
...

👍 instagibbs approved a pull request: "cluster mempool: add txgraph diagrams/mining/eviction"
(https://github.com/bitcoin/bitcoin/pull/31444#pullrequestreview-2729193453)
ACK d8e4b92fb4aea16e18285d25c4d5bba3b3b51c98

non-blocking comments

`git range-diff master 0630995fee22990402771547be1480b8706c76ce d8e4b92fb4aea16e18285d25c4d5bba3b3b51c98`

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2021185973)
Yeah I think no comment at all is better than as-is, but maybe we can just point exactly to what case this is ignoring as safe?

"Sort by feerate only, since violating topological constraints within same-feerate chunks won't affect diagram comparisons."

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2021048633)
hindsight nit: `chunking` could be `const` to make it clear loop end condition isn't being modified

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2020997878)
```Suggestion
/** Get feerate diagrams for both main and staging respectively (which must both exist and not be
```
or be even more pedantic about ordering

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2021186944)
not worth the nit, mark as resolved please

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2020995996)
Actually we need to define exactly what a feerate diagram is here given it could mean a couple things. i.e., it's not the aggregated form, but a series of monotonically decreasing in feerate chunks

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2021150749)
f01ce7f2c9d50a95bb7c694028525c8cc3112a56
```Suggestion
Assume(m_cur_cluster);
m_cur_cluster->GetClusterRefs(*m_graph, ret->first, start_pos);
```

💬 instagibbs commented on pull request "cluster mempool: add txgraph diagrams/mining/eviction":
(https://github.com/bitcoin/bitcoin/pull/31444#discussion_r2021344681)
both cases could use fuzz coverage :pray:

💬 ajtowns commented on pull request "Replace MIN_STANDARD_TX_NONWITNESS_SIZE to preclude 64 non-witness bytes only":
(https://github.com/bitcoin/bitcoin/pull/26398#issuecomment-2766949251)
> > OTOH, it might be plausible to just not worry about small-tx's at all, in which case the check could just be removed entirely, avoiding any discontinuities...
>
> This goes completely against your earlier argument of keeping our options open with regard to upgrade hooks! There is a current proposal to make those transactions invalid, so making them standard now seems unnecessarily risky.

If there is a need to worry about small transactions, then the conservative thing to do in consensu
...