💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417532341)
Indeed!
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417532341)
Indeed!
💬 achow101 commented on pull request "[30.x] Finalise v30.0":
(https://github.com/bitcoin/bitcoin/pull/33559#issuecomment-3386945202)
ACK d615eb6998eeccb9106854dffa95f36b319177e1
(https://github.com/bitcoin/bitcoin/pull/33559#issuecomment-3386945202)
ACK d615eb6998eeccb9106854dffa95f36b319177e1
📝 Aathish101 opened a pull request: "newly Update funcs.mk"
(https://github.com/bitcoin/bitcoin/pull/33590)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
(https://github.com/bitcoin/bitcoin/pull/33590)
<!--
*** Please remove the following help text before submitting: ***
Pull requests without a rationale and clear improvement may be closed
immediately.
GUI-related pull requests should be opened against
https://github.com/bitcoin-core/gui
first. See CONTRIBUTING.md
-->
<!--
Please provide clear motivation for your patch and explain how it improves
Bitcoin Core user experience or Bitcoin Core developer experience
significantly:
* Any test improvements or new tests that improv
...
💬 Aathish101 commented on pull request "newly Update funcs.mk":
(https://github.com/bitcoin/bitcoin/pull/33590#issuecomment-3386948673)
updated
(https://github.com/bitcoin/bitcoin/pull/33590#issuecomment-3386948673)
updated
💬 achow101 commented on pull request "doc: how to update a subtree":
(https://github.com/bitcoin/bitcoin/pull/33568#issuecomment-3386959793)
ACK a1226bc760c70a22ef4a197d5690aca4d83cb74c
(https://github.com/bitcoin/bitcoin/pull/33568#issuecomment-3386959793)
ACK a1226bc760c70a22ef4a197d5690aca4d83cb74c
🚀 achow101 merged a pull request: "doc: how to update a subtree"
(https://github.com/bitcoin/bitcoin/pull/33568)
(https://github.com/bitcoin/bitcoin/pull/33568)
💬 l0rinc commented on pull request "net: make m_nodes_mutex non-recursive":
(https://github.com/bitcoin/bitcoin/pull/32394#discussion_r2417567225)
there are a lot of changes in a single commit that seem not strictly related. Could you maybe extract low-risk refactorings to separate commits so that the more riskier ones are as simple as possible? I prefer the commits telling me a story for why we're doing the change, but here I see a lot of changes that aren't guiding me.
(https://github.com/bitcoin/bitcoin/pull/32394#discussion_r2417567225)
there are a lot of changes in a single commit that seem not strictly related. Could you maybe extract low-risk refactorings to separate commits so that the more riskier ones are as simple as possible? I prefer the commits telling me a story for why we're doing the change, but here I see a lot of changes that aren't guiding me.
💬 kinetickinesthetic commented on issue "v30.0 Testing":
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3386978846)
This patch is a highly contentious update. I implore Core to acknowledge that the network is signaling strong resistance to the proposed changes to op_return / datacarriersize.
We — the users and node runners — do not want the filter to be relaxed. The burden of proof is on Core developers to convince us why this change is beneficial. We understand the technical arguments better than you think. I’ve heard Core’s reasoning for the change, and I remain unconvinced. Furthermore, I will pursue othe
...
(https://github.com/bitcoin/bitcoin/issues/33368#issuecomment-3386978846)
This patch is a highly contentious update. I implore Core to acknowledge that the network is signaling strong resistance to the proposed changes to op_return / datacarriersize.
We — the users and node runners — do not want the filter to be relaxed. The burden of proof is on Core developers to convince us why this change is beneficial. We understand the technical arguments better than you think. I’ve heard Core’s reasoning for the change, and I remain unconvinced. Furthermore, I will pursue othe
...
✅ maflcko closed a pull request: "newly Update funcs.mk"
(https://github.com/bitcoin/bitcoin/pull/33590)
(https://github.com/bitcoin/bitcoin/pull/33590)
🤔 janb84 reviewed a pull request: "[30.x] Finalise v30.0"
(https://github.com/bitcoin/bitcoin/pull/33559#pullrequestreview-3320106531)
ACK d615eb6998eeccb9106854dffa95f36b319177e1
- code review ✅
- checked release notes, they are the same except some grammar (line 184), style changes (" => ` etc) and removal of unused chapters / sections. ✅
- cannot generate list of authors, must be doing something wrong with the script.
(https://github.com/bitcoin/bitcoin/pull/33559#pullrequestreview-3320106531)
ACK d615eb6998eeccb9106854dffa95f36b319177e1
- code review ✅
- checked release notes, they are the same except some grammar (line 184), style changes (" => ` etc) and removal of unused chapters / sections. ✅
- cannot generate list of authors, must be doing something wrong with the script.
🤔 l0rinc reviewed a pull request: "build: Bump clang minimum supported version to 17"
(https://github.com/bitcoin/bitcoin/pull/33555#pullrequestreview-3320154679)
Concept ACK
It seems to me there are a few more clang 16 workarounds that I would prefer addressing here.
Do we still need the build workaround in https://github.com/bitcoin/bitcoin/blob/a4ee70e5b69c67c9caea1e97cb5b08f80536c7d3/src/ipc/libmultiprocess/CMakeLists.txt#L56
Can we simplify these tests now
* https://github.com/bitcoin/bitcoin/blob/43cd83b0c7ba436b8ffc83d8a65e935714dffe70/src/test/arith_uint256_tests.cpp#L583-L592
* https://github.com/bitcoin/bitcoin/blob/e50312eab0b54f338d6e08bea
...
(https://github.com/bitcoin/bitcoin/pull/33555#pullrequestreview-3320154679)
Concept ACK
It seems to me there are a few more clang 16 workarounds that I would prefer addressing here.
Do we still need the build workaround in https://github.com/bitcoin/bitcoin/blob/a4ee70e5b69c67c9caea1e97cb5b08f80536c7d3/src/ipc/libmultiprocess/CMakeLists.txt#L56
Can we simplify these tests now
* https://github.com/bitcoin/bitcoin/blob/43cd83b0c7ba436b8ffc83d8a65e935714dffe70/src/test/arith_uint256_tests.cpp#L583-L592
* https://github.com/bitcoin/bitcoin/blob/e50312eab0b54f338d6e08bea
...
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#issuecomment-3387039466)
Addressed many comments, and made some bigger changes:
* Restructured the commits to first make all abstractions, and then do the abstract class `Cluster` change cleanly.
* Use `GetTxCount()` in more places, instead of `m_graph_index != GraphIndex(-1)`
* Made `Compact()` and memory accounting work at the `TxGraphImpl::Merge` level rather than `Cluster::Merge`: the old code could be compacting O(n) times when merging n clusters together simultaneously.
* Made the dealing with which implementa
...
(https://github.com/bitcoin/bitcoin/pull/33157#issuecomment-3387039466)
Addressed many comments, and made some bigger changes:
* Restructured the commits to first make all abstractions, and then do the abstract class `Cluster` change cleanly.
* Use `GetTxCount()` in more places, instead of `m_graph_index != GraphIndex(-1)`
* Made `Compact()` and memory accounting work at the `TxGraphImpl::Merge` level rather than `Cluster::Merge`: the old code could be compacting O(n) times when merging n clusters together simultaneously.
* Made the dealing with which implementa
...
💬 andrewtoth commented on pull request "net_processing: rename RelayTransaction to better describe what it does":
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2417636066)
> The next send will only include the highest feerate transactions in the queue
Ah, right, but then can we say it is scheduled for the next periodic send which will include it as one of the highest feerate transactions?
> If we keep receiving higher feerate transactions, it won't be inv'd for some time
Sure, but this is I believe an implementation detail. The tx has been scheduled when we call this method, and newly received txs can alter the broadcast schedule after the fact. The inten
...
(https://github.com/bitcoin/bitcoin/pull/33565#discussion_r2417636066)
> The next send will only include the highest feerate transactions in the queue
Ah, right, but then can we say it is scheduled for the next periodic send which will include it as one of the highest feerate transactions?
> If we keep receiving higher feerate transactions, it won't be inv'd for some time
Sure, but this is I believe an implementation detail. The tx has been scheduled when we call this method, and newly received txs can alter the broadcast schedule after the fact. The inten
...
💬 furszy commented on pull request "index: initial sync speedup, parallelize process":
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-3387099398)
Updated with a fuzz test case from @TheCharlatan. Thanks!
(https://github.com/bitcoin/bitcoin/pull/26966#issuecomment-3387099398)
Updated with a fuzz test case from @TheCharlatan. Thanks!
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417685776)
There are certainly possibilities, like storing the clusters in two vectors (one for each implementation), and using positions within that vector + some marker to identify the implementation as "identifier" for the cluster. The difficulty is that you now need to deal with holes in those vectors, or swapping to compact them, meaning you need to go find and update all places where those cluster identifiers are used.
Absent something like that, I think the current approach is close to ideal. It in
...
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417685776)
There are certainly possibilities, like storing the clusters in two vectors (one for each implementation), and using positions within that vector + some marker to identify the implementation as "identifier" for the cluster. The difficulty is that you now need to deal with holes in those vectors, or swapping to compact them, meaning you need to go find and update all places where those cluster identifiers are used.
Absent something like that, I think the current approach is close to ideal. It in
...
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417686261)
Fixed.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417686261)
Fixed.
💬 maflcko commented on pull request "build: Bump clang minimum supported version to 17":
(https://github.com/bitcoin/bitcoin/pull/33555#issuecomment-3387122253)
> Do we still need the build specialization in
Seems unrelated, because the check is to be disabled for clang-15 and less. Not sure why it is there in the first place, but if you want to remove it, it could be removed already today, unrelated to this pull.
> Do we need a build release notes for this?
Probably not, but I've added one. Thx.
> Can we simplify these tests now
Nice. Reverted that commit.
(https://github.com/bitcoin/bitcoin/pull/33555#issuecomment-3387122253)
> Do we still need the build specialization in
Seems unrelated, because the check is to be disabled for clang-15 and less. Not sure why it is there in the first place, but if you want to remove it, it could be removed already today, unrelated to this pull.
> Do we need a build release notes for this?
Probably not, but I've added one. Thx.
> Can we simplify these tests now
Nice. Reverted that commit.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417689081)
Addressed by creating separate FindCluster and FindClusterAndLevel functions. In most call sites, FindCluster can remain unchanged.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417689081)
Addressed by creating separate FindCluster and FindClusterAndLevel functions. In most call sites, FindCluster can remain unchanged.
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417690581)
I have added a few `/*level=*/0` etc. as arguments. Does that help?
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417690581)
I have added a few `/*level=*/0` etc. as arguments. Does that help?
💬 sipa commented on pull request "cluster mempool: control/optimize TxGraph memory usage":
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417692771)
I have incorporated this, and added you as co-author.
(https://github.com/bitcoin/bitcoin/pull/33157#discussion_r2417692771)
I have incorporated this, and added you as co-author.