🚀 fanquake merged a pull request: "doc: fixed inconsistencies in documentation between autotools to cmake change"
(https://github.com/bitcoin/bitcoin/pull/30875)
(https://github.com/bitcoin/bitcoin/pull/30875)
💬 hebasto commented on pull request "Fix linking when configured with `-DENABLE_WALLET=OFF` (Qt 6)":
(https://github.com/bitcoin-core/gui/pull/837#issuecomment-2359064599)
> What is the error?
I put it to the PR description.
> Why does it only happen with Qt6?
Differences in Qt implementation between Qt 5 and Qt 6? (did not dig into the exact code change though)
(https://github.com/bitcoin-core/gui/pull/837#issuecomment-2359064599)
> What is the error?
I put it to the PR description.
> Why does it only happen with Qt6?
Differences in Qt implementation between Qt 5 and Qt 6? (did not dig into the exact code change though)
💬 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.