💬 VolodymyrBg commented on pull request "test: Add support for mainnet addresses in address_to_scriptpubkey":
(https://github.com/bitcoin/bitcoin/pull/32060#issuecomment-2730157607)
@maflcko @davidgumberg is something wrong?
(https://github.com/bitcoin/bitcoin/pull/32060#issuecomment-2730157607)
@maflcko @davidgumberg is something wrong?
💬 theuni commented on pull request "multiprocess: Add libmultiprocess git subtree":
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2730158691)
> I see mpgen now ends up in `build/src/ipc/libmultiprocess/mpgen`. Perhaps after #31375 it can live in `build/libexec`?
mpgen shouldn't be installed (in makefile-speak it's an intermediate binary, which doesn't seem to exist for CMake), so afaik the libexec question is moot.
(https://github.com/bitcoin/bitcoin/pull/31741#issuecomment-2730158691)
> I see mpgen now ends up in `build/src/ipc/libmultiprocess/mpgen`. Perhaps after #31375 it can live in `build/libexec`?
mpgen shouldn't be installed (in makefile-speak it's an intermediate binary, which doesn't seem to exist for CMake), so afaik the libexec question is moot.
💬 Chand-ra commented on pull request "rpc: add cli examples, update docs":
(https://github.com/bitcoin/bitcoin/pull/31958#issuecomment-2730194601)
Concept ACK [8134a6](https://github.com/bitcoin/bitcoin/pull/31958/commits/8134a6b5d40568dcf32fdb21163cb1792efddc27)
The proposed changes _do_ improve the readability of the documentation.
(https://github.com/bitcoin/bitcoin/pull/31958#issuecomment-2730194601)
Concept ACK [8134a6](https://github.com/bitcoin/bitcoin/pull/31958/commits/8134a6b5d40568dcf32fdb21163cb1792efddc27)
The proposed changes _do_ improve the readability of the documentation.
💬 hodlinator commented on pull request "net: Block v2->v1 transport downgrade if !fNetworkActive":
(https://github.com/bitcoin/bitcoin/pull/32073#issuecomment-2730200956)
[Windows CI failure](https://github.com/bitcoin/bitcoin/actions/runs/13904008032/job/38902487830?pr=32073) is unrelated:
```
Downloading https://gitlab.freedesktop.org//freetype/freetype/-/archive/VER-2-13-3/freetype-VER-2-13-3.tar.gz -> freetype-freetype-VER-2-13-3.tar.gz
error: https://gitlab.freedesktop.org//freetype/freetype/-/archive/VER-2-13-3/freetype-VER-2-13-3.tar.gz: failed: status code 503
```
Currently tracked as #32085.
(https://github.com/bitcoin/bitcoin/pull/32073#issuecomment-2730200956)
[Windows CI failure](https://github.com/bitcoin/bitcoin/actions/runs/13904008032/job/38902487830?pr=32073) is unrelated:
```
Downloading https://gitlab.freedesktop.org//freetype/freetype/-/archive/VER-2-13-3/freetype-VER-2-13-3.tar.gz -> freetype-freetype-VER-2-13-3.tar.gz
error: https://gitlab.freedesktop.org//freetype/freetype/-/archive/VER-2-13-3/freetype-VER-2-13-3.tar.gz: failed: status code 503
```
Currently tracked as #32085.
💬 willcl-ark commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r1999216364)
Right, I meant those two only and would agree with adding them to the incantation here, if you agreed.
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r1999216364)
Right, I meant those two only and would agree with adding them to the incantation here, if you agreed.
👍 hodlinator approved a pull request: "test: Update coverage.cpp to drop linux restriction"
(https://github.com/bitcoin/bitcoin/pull/32059#pullrequestreview-2691382483)
re-ACK 54e6eacc1fccd602897d9e3025c62f83194ffd5b
Changes since [first ACK](https://github.com/bitcoin/bitcoin/pull/32059#pullrequestreview-2686973172):
- Moved `weak` attribute on the original declarations so they line up with the fallback definitions and mirror [Clang docs](https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1997328341). Thanks!
- Improved commit message.
Retested using Rust utility on coverage-enabled version of the code with successful determinism result.
(https://github.com/bitcoin/bitcoin/pull/32059#pullrequestreview-2691382483)
re-ACK 54e6eacc1fccd602897d9e3025c62f83194ffd5b
Changes since [first ACK](https://github.com/bitcoin/bitcoin/pull/32059#pullrequestreview-2686973172):
- Moved `weak` attribute on the original declarations so they line up with the fallback definitions and mirror [Clang docs](https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1997328341). Thanks!
- Improved commit message.
Retested using Rust utility on coverage-enabled version of the code with successful determinism result.
💬 hodlinator commented on pull request "test: Update coverage.cpp to drop linux restriction":
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1999246344)
Thanks for lining up the attributes!
(https://github.com/bitcoin/bitcoin/pull/32059#discussion_r1999246344)
Thanks for lining up the attributes!
💬 Sjors commented on pull request "Shuffle depends instructions and recommend modern make for macOS":
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r1999256826)
Added
(https://github.com/bitcoin/bitcoin/pull/32086#discussion_r1999256826)
Added
🤔 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
...