💬 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.
```

💬 instagibbs commented on pull request "Low-level cluster linearization code":
(https://github.com/bitcoin/bitcoin/pull/30126#discussion_r1622667577)
not super important but can we use a more descriptive typename since our use-case is pretty narrow? I'm mentally substituting `S` everywhere with `bitset` which is a fair mental leap and it's relatively difficult to grep for being so short.

`S_bitset`? I don't know why the conventions are the way they are :sweat_smile:

💬 theuni commented on pull request "build: warn on self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30235#discussion_r1629639372)
@maflcko I'd rather not quietly work around it, as it's pretty clearly a compiler bug that has since been fixed.

💬 theuni commented on pull request "build: warn on self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30235#issuecomment-2152658170)
Fixed the other test (added another pragma and comment) and rebased after #30236.

💬 sr-gi commented on pull request "net_processing: make any misbehavior trigger immediate discouragement":
(https://github.com/bitcoin/bitcoin/pull/29575#issuecomment-2152662655)
ACK [ae60d48](https://github.com/bitcoin/bitcoin/pull/29575/commits/ae60d485da33f238ed2186799da4e109d4edd3a1)

Something orthogonal to this, but that I realized working on the same part of the codebase and I wonder if it could be added here:

Looks like the result of `MaybePunishNodeForBlock` is never consumed in the codebase, plus the docs regarding what to return seem not to follow what the code is doing (if `peer` is not found, `Misbehaving` is not called, but the method returns `true`).
...