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

👍 hebasto approved a pull request: "util: add VecDeque"
(https://github.com/bitcoin/bitcoin/pull/30161#pullrequestreview-2102299643)
re-ACK e4ecb8217ada3dae1c1645a8d0a12e14b0f935da, I agree with changes since my recent [review](https://github.com/bitcoin/bitcoin/pull/30161#pullrequestreview-2088736908).

💬 hebasto commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1629750737)
This copy assignment can be amended to handle the self-assignment as well to do not conflict with https://github.com/bitcoin/bitcoin/pull/30234.

💬 hebasto commented on pull request "util: add VecDeque":
(https://github.com/bitcoin/bitcoin/pull/30161#discussion_r1629747141)
Does it make sense to label branches with the `[[likely]]` and `[[unlikely]]` attributes here?

💬 murchandamus commented on pull request "policy: bump TX_MAX_STANDARD_VERSION to 3":
(https://github.com/bitcoin/bitcoin/pull/29496#discussion_r1629749583)
The file name could also be renamed to refer to TRUC transactions instead of v3_policy.

🤔 murchandamus reviewed a pull request: "policy: bump TX_MAX_STANDARD_VERSION to 3"
(https://github.com/bitcoin/bitcoin/pull/29496#pullrequestreview-2102303518)
ACK 30a01134cdec37e7467fcd6eee8b0ae3890a131c

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1629758949)
Yes, interesting edge case! I extended the comment.

💬 fjahr commented on pull request "assumeutxo: Add dumptxoutset height param, remove shell scripts":
(https://github.com/bitcoin/bitcoin/pull/29553#discussion_r1629759103)
Done