💬 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)
💬 VolodymyrBg commented on pull request "qt: Add addressList field to SendCoinsRecipient for multiple addresses":
(https://github.com/bitcoin-core/gui/pull/857#issuecomment-2730662322)
@maflcko done
(https://github.com/bitcoin-core/gui/pull/857#issuecomment-2730662322)
@maflcko done
👍 willcl-ark approved a pull request: "Shuffle depends instructions and recommend modern make for macOS"
(https://github.com/bitcoin/bitcoin/pull/32086#pullrequestreview-2691813509)
reACK 31a0c5f3caa80c8e6baf4e8f909a673442a852d5
(https://github.com/bitcoin/bitcoin/pull/32086#pullrequestreview-2691813509)
reACK 31a0c5f3caa80c8e6baf4e8f909a673442a852d5
💬 tnndbtc commented on pull request "test: fix intermittent failure in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/pull/32063#issuecomment-2730664318)
Hi @mzumsande , Followed your hint that the node needs to successfully disconnect the peer before bumping mocktime, I added one line of "debugging" code to reproduce the issue 100%: in `src/net.cpp:CConnman::DisconnectNodes()`, add a one second sleep before calling `DeleteNode(pnode);`
```
{
// Delete disconnected nodes
std::list<CNode*> nodes_disconnected_copy = m_nodes_disconnected;
for (CNode* pnode : nodes_disconnected_copy)
{
// Destroy
...
(https://github.com/bitcoin/bitcoin/pull/32063#issuecomment-2730664318)
Hi @mzumsande , Followed your hint that the node needs to successfully disconnect the peer before bumping mocktime, I added one line of "debugging" code to reproduce the issue 100%: in `src/net.cpp:CConnman::DisconnectNodes()`, add a one second sleep before calling `DeleteNode(pnode);`
```
{
// Delete disconnected nodes
std::list<CNode*> nodes_disconnected_copy = m_nodes_disconnected;
for (CNode* pnode : nodes_disconnected_copy)
{
// Destroy
...
💬 naiyoma commented on pull request "test: Add test coverage for rpcwhitelistdefault when unset":
(https://github.com/bitcoin/bitcoin/pull/32079#issuecomment-2730670316)
> Concept ACK, nice follow-up! Why is it in draft?
had to force push some changes, but its now open
(https://github.com/bitcoin/bitcoin/pull/32079#issuecomment-2730670316)
> Concept ACK, nice follow-up! Why is it in draft?
had to force push some changes, but its now open
💬 tnndbtc commented on issue "intermittent issue in p2p_orphan_handling.py":
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2730670414)
@maflcko Even with @mzumsande 's [fix](https://github.com/bitcoin/bitcoin/pull/32063) in , I am able to add a one second sleep in src/net.cpp:CConnman::DisconnectNodes() to 100% reproduce the issue. For more details, please checkout https://github.com/bitcoin/bitcoin/pull/32063#issuecomment-2730664318
(https://github.com/bitcoin/bitcoin/issues/31700#issuecomment-2730670414)
@maflcko Even with @mzumsande 's [fix](https://github.com/bitcoin/bitcoin/pull/32063) in , I am able to add a one second sleep in src/net.cpp:CConnman::DisconnectNodes() to 100% reproduce the issue. For more details, please checkout https://github.com/bitcoin/bitcoin/pull/32063#issuecomment-2730664318
💬 maflcko commented on pull request "depends: Avoid using helper variables in toolchain file":
(https://github.com/bitcoin/bitcoin/pull/31360#issuecomment-2730672659)
CI fails with https://cirrus-ci.com/task/6136289713455104?logs=ci#L531 :
```
copying packages: boost libevent qt expat libxcb xcb_proto libXau xproto freetype fontconfig libxkbcommon libxcb_util libxcb_util_render libxcb_util_keysyms libxcb_util_image libxcb_util_wm qrencode systemtap zeromq
to: /ci_container_base/depends/x86_64-pc-linux-gnu
To build Bitcoin Core with these packages, pass '--toolchain /ci_container_base/depends/x86_64-pc-linux-gnu/toolchain.cmake' to the first CMake invo
...
(https://github.com/bitcoin/bitcoin/pull/31360#issuecomment-2730672659)
CI fails with https://cirrus-ci.com/task/6136289713455104?logs=ci#L531 :
```
copying packages: boost libevent qt expat libxcb xcb_proto libXau xproto freetype fontconfig libxkbcommon libxcb_util libxcb_util_render libxcb_util_keysyms libxcb_util_image libxcb_util_wm qrencode systemtap zeromq
to: /ci_container_base/depends/x86_64-pc-linux-gnu
To build Bitcoin Core with these packages, pass '--toolchain /ci_container_base/depends/x86_64-pc-linux-gnu/toolchain.cmake' to the first CMake invo
...