🤔 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!
(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
(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?
(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.
(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.
(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.
...
(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?
(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
(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?
(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);
```
(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?
(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.
```
(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:
(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.
(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.
(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`).
...
(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
...
(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.
...
(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 😄
(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.
(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.