💬 darosior commented on pull request "fuzz: bound some miniscript operations to avoid fuzz timeouts":
(https://github.com/bitcoin/bitcoin/pull/30197#discussion_r1629427388)
Changed to use our own `reverse_iterator`. Opted to instanciate the `std::span` from an iterator instead of adding a dependency on https://github.com/bitcoin/bitcoin/pull/30229. Happy to rebase on top of it if it's merged first.

💬 glozow commented on pull request "refactor: TxDownloadManager":
(https://github.com/bitcoin/bitcoin/pull/30110#issuecomment-2152299577)
rebased

💬 glozow commented on pull request "rpc: distinguish between vsize and sigop-adjusted mempool vsize":
(https://github.com/bitcoin/bitcoin/pull/27591#issuecomment-2152302281)
Sorry, I kept thinking I would get back to this but haven't. Closing + marking up for grabs.
Note for anybody who wants to pick this up, note you'll want to write the doc described in https://github.com/bitcoin/bitcoin/pull/27591#discussion_r1378636536

✅ glozow closed a pull request: "rpc: distinguish between vsize and sigop-adjusted mempool vsize"
(https://github.com/bitcoin/bitcoin/pull/27591)

💬 glozow commented on pull request "refactor: policy: Pass kernel::MemPoolOptions to IsStandard[Tx] rather than long list of individual options":
(https://github.com/bitcoin/bitcoin/pull/30232#issuecomment-2152305711)
Mind providing some motivation for the refactor? PR description is empty

💬 fanquake commented on pull request "Sanitizing ports of -rpcconnect and -rpcport.":
(https://github.com/bitcoin/bitcoin/pull/27820#issuecomment-2152305956)
Picked up in #29521.

💬 theStack commented on pull request "test: Remove struct.pack from almost all places":
(https://github.com/bitcoin/bitcoin/pull/29401#discussion_r1629496043)
nit: for serializing single bytes I personally prefer `bytes([value])` over the `.to_bytes` method, e.g.
```suggestion
r = bytes([253]) + l.to_bytes(2, "little")
```
(but no blocker, feel free to ignore)

👍 theStack approved a pull request: "test: Remove struct.pack from almost all places"
(https://github.com/bitcoin/bitcoin/pull/29401#pullrequestreview-2101884249)
Code-review ACK fa52e13ee81fcc7543890dbd6986fcb55168583f

🤔 instagibbs reviewed a pull request: "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`"
(https://github.com/bitcoin/bitcoin/pull/30237#pullrequestreview-2102010028)
concept ACK!

💬 instagibbs commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#discussion_r1629573202)
I think another useful case is a transaction that's *also* in your mempool

💬 theuni commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2152614416)
Closed on purpose?

💬 AngusP commented on pull request "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`":
(https://github.com/bitcoin/bitcoin/pull/30237#discussion_r1629620484)
L319 has `pool.addUnchecked(entry.FromTx(block.vtx[2]));` which adds a tx from the block to the mempool, giving `.IsTxAvailable(2)` (`.IsTxAvailable(0)` should always be true as that's the coinbase [which is in the compact block header](https://github.com/bitcoin/bitcoin/blob/1040a1fc807ed984020eeaa6e90b5bf070b61b05/src/blockencodings.cpp#L25), IIUC).

`SimpleRoundTripTest` and a couple others in here also already covered the add to mempool case.

💬 theuni commented on pull request "Enable clang-tidy checks for self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30234#issuecomment-2152629880)
> > Hmm, how best to ignore the tidy false-positive in a leveldb header?
>
> FWIW, in the CMake staging branch, all subtrees are already excluded from being analyzed by clang-tidy.

I believe (based on our `.bear-tidy-config`) that this is the case in current master as well. That's not the issue here, though.

What we're running into is a leveldb header that's included by `dbwrapper.cpp`, so excluding the leveldb build itself doesn't help.

💬 sr-gi commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1629635922)
You're right. This was an oversimplification. However, `CNodeState` and `Peer` should be tightly coupled, shouldn't they? Both are wiped in `PeerManagerImpl::FinalizeNode` (synchronously I assume), so I think there shouldn't have been a way to trigger that `true` when `peer` was a `nullptr`.

Zooming out a little bit, `MaybePunishNodeForBlock` return value is never used in the codebase (AFAICT at least), plus the docs also seem to be confusing when true needs to be returned:

https://github.
...

💬 instagibbs commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1622515654)
For this operator are `S ancestors` and `S descendants` ordered? Is it just compared via `m_val` for `BitSets`? Is ordering by these elements meaningful?

🤔 instagibbs reviewed a pull request: "Low-level cluster linearization code"
(https://github.com/bitcoin/bitcoin/pull/30126#pullrequestreview-2077166958)
glanced in the context of checking what of VecDeque/BitSet actually required

💬 instagibbs commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1613617080)
in/out args for iterations is kinda spooky and made these few lines really hard to read, could `FindCandidateSet` just return a `std::pair` or similar?

💬 instagibbs commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1622564714)
it crashes once it hits `S::Singleton` if violated but maybe an earlier crash is more immediately informative
```suggestion
Assume(ntx <= S::Size());
entries.resize(ntx);
```

💬 instagibbs commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1626377280)
is this for ensuring no allocations are going to take place? How do we know this?

💬 instagibbs commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1622574793)
to avoid an OOB access regression
```suggestion
Assert(mapping.size() == depgraph.TxCount());
// Fill in fee, size, ancestors.
```