🤔 hodlinator reviewed a pull request: "rpc: Distinguish between vsize and sigop adjusted mempool vsize"
(https://github.com/bitcoin/bitcoin/pull/32800#pullrequestreview-3026725085)
Reviewed 8158ca70a7b6b492607918d860947497efd72cd3

Found some final things before A-C-K.

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2215809980)
Realized the comment is now out of date:
```diff
- // Add sigopsize if the transaction exists in the mempool and its size is affected by it.
+ // Add sigopsize if the transaction exists in the mempool.
```

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2213198113)
Documentation of `getrawtransaction` says:
> By default, this call only returns a transaction if it is in the mempool.

So not sure what you mean by "not yet in the mempool".

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2215831224)
When building block templates, I'm not sure we want to recommend using the sigop-adjusted vsize? That might leave space over in the block if some transactions have a lot of sigops. This would make a miner earn less fees due to not filling up the block.

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2213166401)
meganit: This newline was logically separating the two commented code blocks and should probably be brought back.

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2213183113)
nit: It would be good to hyphen-prefix to make it stand out visually (from fields like `vsize` etc) and also call it "setting" or "argument" rather than "parameter" as those are the terms used in the code.
```suggestion
calculation (sigop count multiplied by a configurable `-bytesPerSigOp` setting, which defaults to
```

💬 hodlinator commented on pull request "rpc: Distinguish between vsize and sigop adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/32800#discussion_r2211498969)
Side note: I think it's weird that we would not include the sizes when exceeding the max fee rate - one would think that it would be interesting to know then too. But it seems to have been the same since 2233a93a109b10b6fe0f5f26c2bb529c8de3dde7 when `vsize` was added. Not suggesting it be changed in this PR, but possible follow-up if others agree.

💬 maflcko commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#discussion_r2215907801)
the diff is already at more than 100 lines, so i wanted to keep it minimal for now

💬 maflcko commented on pull request "log: [refactor] Use info level for init logs":
(https://github.com/bitcoin/bitcoin/pull/32967#discussion_r2215916022)
I disagree, because:


* All parameter interaction is (and was) info level (see also `LogInfo("parameter interaction: -blocksonly=1 -> setting -maxmempool=%d\n", DEFAULT_BLOCKSONLY_MAX_MEMPOOL_SIZE_MB);` in init cpp)
* A warning is for the node operator to investigate and possibly fix. See https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#logging
* The `-blocksonly` docs already explain the interaction, so if someone ignores the official docs, hiding a warning in the debug l
...

💬 willcl-ark commented on pull request "tests: speed up coins_tests by parallelizing":
(https://github.com/bitcoin/bitcoin/pull/32945#issuecomment-3089316864)
On my machine I see this go from master:

```bash
135/139 Test #28: coins_tests .......................... Passed 7.61 sec
136/139 Test #5: secp256k1_exhaustive_tests ........... Passed 8.13 sec
137/139 Test #126: coinselector_tests ................... Passed 11.85 sec
138/139 Test #3: secp256k1_noverify_tests ............. Passed 14.59 sec
139/139 Test #4: secp256k1_tests ...................... Passed 29.11 sec
```

This branch (with a small patch):
<deta
...

💬 maflcko commented on pull request "improve MallocUsage() accuracy":
(https://github.com/bitcoin/bitcoin/pull/28531#issuecomment-3089357219)
Could turn into draft while CI is red?

💬 maflcko commented on pull request "docs: adds correct updated documentation links":
(https://github.com/bitcoin/bitcoin/pull/32699#issuecomment-3089360188)
Can this be drafted/closed, or are you still working on this?

💬 sipa commented on pull request "Cluster linearization: separate tests from tests-of-tests":
(https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2215991768)
How about

```c++
if (mask != uint64_t(-1)) mask += nonzero;
```

That avoids overflow, makes the conment accurate, but also keeps the existing (and intended) behavior of effectively letting the range of mask start at 1 rather than 0 (shifting everything up, rather than mapping both 0 and 1 to 1).

🤔 danielabrozzoni reviewed a pull request: "test: refactor: overhaul block hash determination for `CBlock{,Header}` objects"
(https://github.com/bitcoin/bitcoin/pull/32868#pullrequestreview-3033499281)
reACK 5fa34951ead2eebcced919537f5e27526f61d909

📝 LarryRuane converted_to_draft a pull request: "improve MallocUsage() accuracy"
(https://github.com/bitcoin/bitcoin/pull/28531)
The `MallocUsage()` function takes an allocation size as an argument and returns the amount of physical memory consumed, which is greater due to memory allocator overhead and alignment. It was first added in 2015 (first commit of #6102), but its accuracy has degraded as memory allocation libraries have evolved. It's used when it's important that large data structures, such as the coins cache and mempool, should use a predictable, configurable (limited) amount of physical memory (see the `-dbcach
...

💬 Crypt-iQ commented on pull request "log: Mitigate disk filling attacks by rate limiting LogPrintf, LogInfo, LogWarning, LogError":
(https://github.com/bitcoin/bitcoin/pull/32604#discussion_r2216025047)
I have opted not to do this in the follow-up PR.

🚀 fanquake merged a pull request: "test: refactor: overhaul block hash determination for `CBlock{,Header}` objects"
(https://github.com/bitcoin/bitcoin/pull/32868)

💬 glozow commented on pull request "refactor: GenTxid type safety followups":
(https://github.com/bitcoin/bitcoin/pull/33005#discussion_r2216047704)
67837bb426049918004fe48525db71b515940e6f seems like an unnecessarily complex approach? We can just keep a set of `Wtxid`s and, if the peer does not support wtxidrelay, grab the txid from `info.tx` (which we have to look up anyway) to construct the inv message. It's also going to be Wtxids pretty much 100% of the time.

💬 maflcko commented on pull request "fuzz: Make process_message(s) more deterministic":
(https://github.com/bitcoin/bitcoin/pull/32822#issuecomment-3089499548)
(rebased)

📝 ismaelsadeeq opened a pull request: "test: fix `ReadTopologicalSet` unsigned integer overflow"
(https://github.com/bitcoin/bitcoin/pull/33007)
This PR is a simple fix for a potential unsigned integer overflow in ReadTopologicalSet.
We obtain the value of `mask` from fuzz input, which can be the maximum representable value.
Adding 1 to it would then cause an overflow.

The fix skips the addition when the read value is already the maximum.

See https://github.com/bitcoin/bitcoin/pull/30605#discussion_r2215338569 for more context