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

💬 sr-gi commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#issuecomment-2152685201)
> Concept ACK. Mind sharing how you see this fitting in with Erlay work?
>
> Will review in more depth soon.

Sure. For Erlay purposes, some transactions need to be sent via fanout even to Erlay-enabled peers. In order to know to how many peers to send this via fanout, we need to count the amount of tx-relaying peers that we have, both inbound and outbound. This is done, in the current patch, by going over all peers when constructing their corresponding INV message and deciding whether to a
...

💬 ryanofsky commented on pull request "validation: Make ReplayBlocks interruptible":
(https://github.com/bitcoin/bitcoin/pull/30155#discussion_r1629646400)
In commit "validation: Make ReplayBlocks interruptible" (26e78f2ee4a325f20c3b3bfaac5be532ab9e7bac)

> I switched to the current approach of detecting this implicitly inside `BatchWrite()` because it seemed more elegant and led to less boilerplate code, but it's still an option.

I would definitely be curious to see the other approach. Not just because it might be better at detecting inconsistent states like Luke is suggesting, but also because it seems like it would make the code more clear.
...

💬 laanwj commented on pull request "depends: Remove Qt build-time dependencies":
(https://github.com/bitcoin/bitcoin/pull/29923#issuecomment-2152711889)
i was a bit tired of seeing "you need to rebase" messages, will get around to this 😄

💬 maflcko commented on pull request "refactor: move m_is_inbound out of CNodeState":
(https://github.com/bitcoin/bitcoin/pull/30233#discussion_r1629711039)
If the return value is wrong or unused, it could make sense to remove it? Also, it could make sense to update the pattern `if (peer && ...` in the other cases as well for consistency, if this is correct.

💬 maflcko commented on pull request "build: warn on self-assignment":
(https://github.com/bitcoin/bitcoin/pull/30235#issuecomment-2152770109)
ACK 15796d4b61342f75548b20a18c670ed21d102ba8

💬 maflcko commented on pull request "test: Remove struct.pack from almost all places":
(https://github.com/bitcoin/bitcoin/pull/29401#discussion_r1629726335)
I agree, but for consistency with the other `def ser_compact_size` and the other code, I'll leave as-is for now.

👍 dergoegge approved a pull request: "test: Add Compact Block Encoding test `ReceiveWithExtraTransactions` covering non-empty `extra_txn`"
(https://github.com/bitcoin/bitcoin/pull/30237#pullrequestreview-2102290054)
Code review ACK 4c99301220ab44e98d0d0e1cc8d774d96a25b7aa

I've verified that this can reproduce the nullptr deref bug by reverting a8203e94123b6ea6e4f4a6320e3ad20457f44a28 and making short id collisions more likely (see top commits on: https://github.com/dergoegge/bitcoin/commits/2024-06-cmpctblk-extra-txn-test/).
```
$ n=0; while (( n++ < 100 )); do src/test/test_bitcoin --run_test=blockencodings_tests/ReceiveWithExtraTransactions ; done
...
Running 1 test case...
unknown location(0): fa
...