🤔 ajtowns reviewed a pull request: "cluster mempool: introduce TxGraph"
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2684192006)
This seems fairly difficult to review -- there's a lot of mostly implicit background in what TxGraph does and why it does it that way, as evidenced by both the description in this PR and in the [review club](https://bitcoincore.reviews/31363).
When I started typing this comment, I was wondering if maybe it would make sense to make the commits here into more of a gentle introduction to TxGraph, rather than one commit that does everything, then optimisations. But I don't really think that appro
...
(https://github.com/bitcoin/bitcoin/pull/31363#pullrequestreview-2684192006)
This seems fairly difficult to review -- there's a lot of mostly implicit background in what TxGraph does and why it does it that way, as evidenced by both the description in this PR and in the [review club](https://bitcoincore.reviews/31363).
When I started typing this comment, I was wondering if maybe it would make sense to make the commits here into more of a gentle introduction to TxGraph, rather than one commit that does everything, then optimisations. But I don't really think that appro
...
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994918766)
`std::array<std::vector<std::unique_ptr<Cluster>>,int{QualityLevel::NONE}> m_clusters;` might read clearer?
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994918766)
`std::array<std::vector<std::unique_ptr<Cluster>>,int{QualityLevel::NONE}> m_clusters;` might read clearer?
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994939901)
"A class of objects" doesn't seem like a very useful description? "Internal information about each transaction in a TxGraphImpl" ?
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994939901)
"A class of objects" doesn't seem like a very useful description? "Internal information about each transaction in a TxGraphImpl" ?
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994757992)
This commit could be a scripted diff.
```
-BEGIN VERIFY SCRIPT-
sed -i 's/Data type to represent transaction indices in clusters./Data type to represent transaction indices in DepGraphs and the clusters they represent./' $(git grep -l 'using ClusterIndex')
sed -i 's|\<ClusterIndex\>|DepGraphIndex|g' $(git grep -l 'ClusterIndex')
-END VERIFY SCRIPT-
```
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994757992)
This commit could be a scripted diff.
```
-BEGIN VERIFY SCRIPT-
sed -i 's/Data type to represent transaction indices in clusters./Data type to represent transaction indices in DepGraphs and the clusters they represent./' $(git grep -l 'using ClusterIndex')
sed -i 's|\<ClusterIndex\>|DepGraphIndex|g' $(git grep -l 'ClusterIndex')
-END VERIFY SCRIPT-
```
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994854097)
The invalidation behaviour for Refs seems unclear to me? I think the idea is that if you hold onto a `Ref` you can continue to uniquely refer to a removed transaction, up until the Ref is destructed, at which point the TxGraph is compacted and memory might be reused at that point. This delay then also allows for reordering to occur which can result in optimisations.
I think the intended behaviour could be made a bit clearer in the header file though? It's not immediately obvious to me under w
...
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994854097)
The invalidation behaviour for Refs seems unclear to me? I think the idea is that if you hold onto a `Ref` you can continue to uniquely refer to a removed transaction, up until the Ref is destructed, at which point the TxGraph is compacted and memory might be reused at that point. This delay then also allows for reordering to occur which can result in optimisations.
I think the intended behaviour could be made a bit clearer in the header file though? It's not immediately obvious to me under w
...
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1996715147)
Might be worth pointing out that the api is designed towards allowing for an implementation that stores the transitive closure of dependencies -- when querying, if B spends C, then you can't differentiate between A spends B and A spends B and C.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1996715147)
Might be worth pointing out that the api is designed towards allowing for an implementation that stores the transitive closure of dependencies -- when querying, if B spends C, then you can't differentiate between A spends B and A spends B and C.
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994970731)
Having each Ref point to the TxGraph and the TxGraph point to (and potentially modify via SwapIndexes) every Ref seems very self-referential, which is probably a bit confusing and not very cache optimal and so forth...
I think you could reduce this to a `const Ref*` (so that Refs wouldn't get modified via `entry.m_ref`) by replacing the use of SwapIndexes on Compact() with a free list/heap of no-longer used entries. Probably not worthwhile though.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994970731)
Having each Ref point to the TxGraph and the TxGraph point to (and potentially modify via SwapIndexes) every Ref seems very self-referential, which is probably a bit confusing and not very cache optimal and so forth...
I think you could reduce this to a `const Ref*` (so that Refs wouldn't get modified via `entry.m_ref`) by replacing the use of SwapIndexes on Compact() with a free list/heap of no-longer used entries. Probably not worthwhile though.
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1999180674)
I think this gives you a segfault when aborting the program if the TxGraph is destructed prior to some of its outstanding Ref's being destructed. It might be worthwhile having the TxGraph destructor iterate through any txs that haven't already been unlinked and clear the `m_graph` pointers back to itself. Alternatively, the TxGraph destructor could issue an assertion failure if there are any linked Refs remaining.
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1999180674)
I think this gives you a segfault when aborting the program if the TxGraph is destructed prior to some of its outstanding Ref's being destructed. It might be worthwhile having the TxGraph destructor iterate through any txs that haven't already been unlinked and clear the `m_graph` pointers back to itself. Alternatively, the TxGraph destructor could issue an assertion failure if there are any linked Refs remaining.
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994762659)
("and so forth" alternatively)
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994762659)
("and so forth" alternatively)
💬 ajtowns commented on pull request "cluster mempool: introduce TxGraph":
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994903294)
Doesn't "topologically-valid order" already cover what "acceptable quality" means here, as far as the interface is concerned?
(https://github.com/bitcoin/bitcoin/pull/31363#discussion_r1994903294)
Doesn't "topologically-valid order" already cover what "acceptable quality" means here, as far as the interface is concerned?
💬 m3dwards commented on pull request "qa: Verify clean shutdown on startup failure":
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1999309420)
The inclusion of an assert suggests to me that we expect that all OSErrors without errno are TimeoutErrors? Is this actually what we expect?
If not should the if statement be changed to `if error_num is None and isinstance(e, TimeoutError):`
(https://github.com/bitcoin/bitcoin/pull/30660#discussion_r1999309420)
The inclusion of an assert suggests to me that we expect that all OSErrors without errno are TimeoutErrors? Is this actually what we expect?
If not should the if statement be changed to `if error_num is None and isinstance(e, TimeoutError):`
🤔 mzumsande reviewed a pull request: "p2p: protect addnode peers during IBD"
(https://github.com/bitcoin/bitcoin/pull/32051#pullrequestreview-2691505746)
I think the main conceptual question is what should happen in the case of a stalling situation where the addnode peer is actually slow and causing congestion?
While we have more trust in manual peers than automatic ones, I don't think that means the trust should be without bounds. Even trusted nodes can become unresponsive temporarily (e.g. very slow internet). I'm not sure if we should give one addnode peer the power to let our entire IBD come to a halt completely.
An alternative would be
...
(https://github.com/bitcoin/bitcoin/pull/32051#pullrequestreview-2691505746)
I think the main conceptual question is what should happen in the case of a stalling situation where the addnode peer is actually slow and causing congestion?
While we have more trust in manual peers than automatic ones, I don't think that means the trust should be without bounds. Even trusted nodes can become unresponsive temporarily (e.g. very slow internet). I'm not sure if we should give one addnode peer the power to let our entire IBD come to a halt completely.
An alternative would be
...
💬 mzumsande commented on pull request "p2p: protect addnode peers during IBD":
(https://github.com/bitcoin/bitcoin/pull/32051#discussion_r1999313868)
This is not the timeout specific for IBD stalling but the general one (which is generally longer). So we'd still disconnect for stalling (see log message "Peer is stalling block download") - is that on purpose / can you explain a bit more the reasoning behind this?
(https://github.com/bitcoin/bitcoin/pull/32051#discussion_r1999313868)
This is not the timeout specific for IBD stalling but the general one (which is generally longer). So we'd still disconnect for stalling (see log message "Peer is stalling block download") - is that on purpose / can you explain a bit more the reasoning behind this?
💬 mzumsande commented on pull request "validation: stricter internal handling of invalid blocks":
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2730598301)
> Update: Just saw the PR https://github.com/bitcoin/bitcoin/pull/31835 which fixes this issue. :)
Heh, looks like you are at least the third person who found that bug independently, first one was in #16856!
> Since both branches behave similarly (even on master the m_best_header and BLOCK_FAILED_CHILD statuses are set correctly because of the final call to InvalidChainFound - except for the ones not set as BLOCK_FAILED_CHILD because of the potential bug), is it correct that the main goal
...
(https://github.com/bitcoin/bitcoin/pull/31405#issuecomment-2730598301)
> Update: Just saw the PR https://github.com/bitcoin/bitcoin/pull/31835 which fixes this issue. :)
Heh, looks like you are at least the third person who found that bug independently, first one was in #16856!
> Since both branches behave similarly (even on master the m_best_header and BLOCK_FAILED_CHILD statuses are set correctly because of the final call to InvalidChainFound - except for the ones not set as BLOCK_FAILED_CHILD because of the potential bug), is it correct that the main goal
...
🤔 marcofleon reviewed a pull request: "fuzz: Use serial task runner to increase fuzz stability"
(https://github.com/bitcoin/bitcoin/pull/31841#pullrequestreview-2691764413)
ACK fa4fb6a8f15c295a2a3d3ffd737e115b8be46c1f
Seems fine to me to use `ImmediateTaskRunner` for fuzz tests. I checked stability for `partially_downloaded_block` and saw it go from 90% to 94%.
Am I correct in saying that this change makes calling something like `SyncWithValidationInterfaceQueue()` in fuzz targets effectively a no-op?
(https://github.com/bitcoin/bitcoin/pull/31841#pullrequestreview-2691764413)
ACK fa4fb6a8f15c295a2a3d3ffd737e115b8be46c1f
Seems fine to me to use `ImmediateTaskRunner` for fuzz tests. I checked stability for `partially_downloaded_block` and saw it go from 90% to 94%.
Am I correct in saying that this change makes calling something like `SyncWithValidationInterfaceQueue()` in fuzz targets effectively a no-op?
💬 janb84 commented on pull request "doc: Update documentation to include Clang/llvm based coverage report generation":
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1999479349)
The extra `-g` just gives a lot of `Unexecuted instantiation:` messages and a lot lower coverage. No sure why but don't think this gives a good image of the actual coverage.
(https://github.com/bitcoin/bitcoin/pull/31933#discussion_r1999479349)
The extra `-g` just gives a lot of `Unexecuted instantiation:` messages and a lot lower coverage. No sure why but don't think this gives a good image of the actual coverage.
💬 maflcko commented on pull request "fuzz: Use immediate task runner to increase fuzz stability":
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2730623476)
> Am I correct in saying that this change makes calling something like `SyncWithValidationInterfaceQueue()` in fuzz targets effectively a no-op?
Yes, they could be removed now. Happy to include that here, but they should also be harmless.
(https://github.com/bitcoin/bitcoin/pull/31841#issuecomment-2730623476)
> Am I correct in saying that this change makes calling something like `SyncWithValidationInterfaceQueue()` in fuzz targets effectively a no-op?
Yes, they could be removed now. Happy to include that here, but they should also be harmless.
💬 maflcko commented on pull request "Feature: Use different datadirs for different signets":
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r1999497982)
stray leftover debug statement?
(https://github.com/bitcoin/bitcoin/pull/29838#discussion_r1999497982)
stray leftover debug statement?
🤔 maflcko reviewed a pull request: "Feature: Use different datadirs for different signets"
(https://github.com/bitcoin/bitcoin/pull/29838#pullrequestreview-2691801719)
are you still working on this?
(https://github.com/bitcoin/bitcoin/pull/29838#pullrequestreview-2691801719)
are you still working on this?
👋 naiyoma's pull request is ready for review: "test: Add test coverage for rpcwhitelistdefault when unset"
(https://github.com/bitcoin/bitcoin/pull/32079)
(https://github.com/bitcoin/bitcoin/pull/32079)