💬 andrewtoth commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1765470983)
Hmm looks like you moved the `&` now too, along with the brace on the newline.
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1765470983)
Hmm looks like you moved the `&` now too, along with the brace on the newline.
🤔 sipa reviewed a pull request: "cluster mempool: extend DepGraph functionality"
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2313371167)
I addressed most of the feedback received. Thanks @instagibbs and @glozow!
In addition, the `DepGraphFormatter::Unser` logic was changed significantly. The result is simpler to follow I think (doesn't involve adding a transaction once and then undoing the insertion and redoing it once the position is known), and reduces the complexity from $\mathcal{O}(n^3)$ to $\mathcal{O}(n^2)$ in the process. For this, the commits were reordered, and their commit messages expanded to explain the rationale.
...
(https://github.com/bitcoin/bitcoin/pull/30857#pullrequestreview-2313371167)
I addressed most of the feedback received. Thanks @instagibbs and @glozow!
In addition, the `DepGraphFormatter::Unser` logic was changed significantly. The result is simpler to follow I think (doesn't involve adding a transaction once and then undoing the insertion and redoing it once the position is known), and reduces the complexity from $\mathcal{O}(n^3)$ to $\mathcal{O}(n^2)$ in the process. For this, the commits were reordered, and their commit messages expanded to explain the rationale.
...
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765474071)
Done. I've expanded the commit message of all commits.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765474071)
Done. I've expanded the commit message of all commits.
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765470244)
Done. Made it "Fill in dependencies by remapping direct parents.".
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765470244)
Done. Made it "Fill in dependencies by remapping direct parents.".
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765472587)
I went a lot further and dropped `AddDependency` entirely.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765472587)
I went a lot further and dropped `AddDependency` entirely.
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765479597)
Done.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765479597)
Done.
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765475972)
Same reason; won't work for cyclic graphs.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765475972)
Same reason; won't work for cyclic graphs.
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765469878)
Fixed. I initially intended to introduce this in multiple commits, but then didn't go back to editing the commit message after everything ended up in one commit.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765469878)
Fixed. I initially intended to introduce this in multiple commits, but then didn't go back to editing the commit message after everything ended up in one commit.
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765478370)
This is adding transactions to `depgraph_tree` corresponding to the holes in `depgraph_gen`, which are then deleted below. The point is making sure that the non-holes have the same positions in both. I have added a comment to explain.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765478370)
This is adding transactions to `depgraph_tree` corresponding to the holes in `depgraph_gen`, which are then deleted below. The point is making sure that the non-holes have the same positions in both. I have added a comment to explain.
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765479695)
Done.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765479695)
Done.
💬 sipa commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765475761)
Sadly that won't work when there are cyclic dependencies. Of course, this test could be moved down to the "if acyclic" branch, but there is a stronger test there already.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765475761)
Sadly that won't work when there are cyclic dependencies. Of course, this test could be moved down to the "if acyclic" branch, but there is a stronger test there already.
💬 l0rinc commented on pull request "refactor: migrate `bool GetCoin` to return `optional<Coin>`":
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1765489041)
Yes, formatted the cpp, since it was modified, reverted the header, since it wasn't.
Resolving this comment, as the developer notes state:
> However, we're now trying to converge to a single style, which is specified below. When writing patches, favor the new style over attempting to mimic the surrounding style, except for move-only commits.
But let me know if it was correct before, and my modification is incorrect, maybe I misunderstood what you meant.
(https://github.com/bitcoin/bitcoin/pull/30849#discussion_r1765489041)
Yes, formatted the cpp, since it was modified, reverted the header, since it wasn't.
Resolving this comment, as the developer notes state:
> However, we're now trying to converge to a single style, which is specified below. When writing patches, favor the new style over attempting to mimic the surrounding style, except for move-only commits.
But let me know if it was correct before, and my modification is incorrect, maybe I misunderstood what you meant.
💬 instagibbs commented on pull request "p2p: When close to the tip, download blocks in parallel from additional peers to prevent stalling":
(https://github.com/bitcoin/bitcoin/pull/29664#issuecomment-2359136626)
IIUC, this could also fire at tip if there are no hb peers and the first peer requested stalls out for 30s? Once parallel compact blocks is warmed up, the slots will likely be taken by those efforts.
If so, I wonder if this could allow us to f.e., not require a parallel compact slot be taken by an outbound, if we know we will try an outbound connection 30s later to defeat stalling behavior?
Having tests for the interplay would be interesting too.
(https://github.com/bitcoin/bitcoin/pull/29664#issuecomment-2359136626)
IIUC, this could also fire at tip if there are no hb peers and the first peer requested stalls out for 30s? Once parallel compact blocks is warmed up, the slots will likely be taken by those efforts.
If so, I wonder if this could allow us to f.e., not require a parallel compact slot be taken by an outbound, if we know we will try an outbound connection 30s later to defeat stalling behavior?
Having tests for the interplay would be interesting too.
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765524349)
even better
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765524349)
even better
✅ mzumsande closed a pull request: "p2p: attempt to fill full outbound connection slots with peers that support tx relay"
(https://github.com/bitcoin/bitcoin/pull/28538)
(https://github.com/bitcoin/bitcoin/pull/28538)
💬 mzumsande commented on pull request "p2p: attempt to fill full outbound connection slots with peers that support tx relay":
(https://github.com/bitcoin/bitcoin/pull/28538#issuecomment-2359152545)
Not working on this currently, I might open a new PR at a later time, or up for grabs if someone else wants to.
(https://github.com/bitcoin/bitcoin/pull/28538#issuecomment-2359152545)
Not working on this currently, I might open a new PR at a later time, or up for grabs if someone else wants to.
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765529514)
If we're allowing it, let's make sure there's overage for it in the fuzzer
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765529514)
If we're allowing it, let's make sure there's overage for it in the fuzzer
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765537433)
ok, seems clear to me now, not sure why I was so confused.
(https://github.com/bitcoin/bitcoin/pull/30857#discussion_r1765537433)
ok, seems clear to me now, not sure why I was so confused.
💬 instagibbs commented on pull request "cluster mempool: extend DepGraph functionality":
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2359202120)
looked at `git range-diff master 9f67d6dcdcf7e9c75a93ad78f8902537599f4f6d 4476915bc76b48eff9b5e67fd6bd7647cc12793f`
LGTM, though I haven't re-validated the serializer, trusting the tests of tests. I want to think more about the fuzz targets to see if there are any other easy pickups in coverage.
(https://github.com/bitcoin/bitcoin/pull/30857#issuecomment-2359202120)
looked at `git range-diff master 9f67d6dcdcf7e9c75a93ad78f8902537599f4f6d 4476915bc76b48eff9b5e67fd6bd7647cc12793f`
LGTM, though I haven't re-validated the serializer, trusting the tests of tests. I want to think more about the fuzz targets to see if there are any other easy pickups in coverage.
📝 LuizWT opened a pull request: "Optimize: convert trusted keys list to a set for better performance"
(https://github.com/bitcoin/bitcoin/pull/30925)
# Motivation
This pull request optimizes the handling of trusted keys by converting lists to sets. This change improves performance for membership checks, making them faster.
Changes Made
Changed trusted keys storage from lists to sets for faster access.
# Tests
No new tests were added, as this is a refactor that does not change the business logic.
(https://github.com/bitcoin/bitcoin/pull/30925)
# Motivation
This pull request optimizes the handling of trusted keys by converting lists to sets. This change improves performance for membership checks, making them faster.
Changes Made
Changed trusted keys storage from lists to sets for faster access.
# Tests
No new tests were added, as this is a refactor that does not change the business logic.